Patterns, Templates: Try conditional theme attribute injection

[ockham]

Not planning to land this for 6.4, it's been too much of a 🎢

WIP, early stages.

Since we had to revert our previous attempt at this, I'm starting a new draft PR based on the notes I posted here.

TODO:

• Move hooked blocks insertion to the patterns controller so we can remove it from WP_Block_Patterns_Registry
• Make sure hooked block insertion is run on the frontend. We can probably achieve that through running it in the Pattern block's render_block.
• Apply the same changes to Templates?
• Add test coverage to avoid the previous regression. If we manage to move all the REST API specific logic into the respective controllers (and get rid of all REST_REQUEST conditionals), it's probably enough to add unit test coverage for those controllers

When testing, make sure that this doesn't cause this regression (it currently still does; needs more tweaking!)

Trac ticket: TBD

---

This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

[ockham]

cc/ @gziolo @felixarntz

[felixarntz]

Thanks for the ping @ockham. Do we know why the revert in #5509 fixed the problem? Why did the ( defined( 'REST_REQUEST' ) && REST_REQUEST ) not work? Is it because the relevant REST endpoint that needs this is preloaded via PHP (in which case REST_REQUEST would not be defined)? Or why was that insufficient / unreliable as a solution?

[gziolo]

Why did the ( defined( 'REST_REQUEST' ) && REST_REQUEST ) not work? Is it because the relevant REST endpoint that needs this is preloaded via PHP (in which case REST_REQUEST would not be defined)?

I haven't had a chance to have a closer look, but it would be my understanding that this check won't work with the preloading feature. I'm wondering if it would be possible to create a helper function like wp_is_rest_request which would account for preloading requests with some magic global, too.

However, I believe that in this case, @ockham is planning to move the necessary computation to the REST API handler which will remove the need for using this check altogether.

[ockham]

Yeah, my best guess is that REST_REQUEST isn't set to true when preloading. I didn't verify 100% but did notice that at the time, the theme attribute was missing for a number of template parts on trunk but was correctly injected on the revert branch.

[ockham]

BTW if that's indeed the reason, it raises the question why we're not setting REST_REQUEST to true right before preloading in the first place (and back to false right after -- if that's possible in PHP with define? 🤔 )

It seems like we'd like to mimic a REST API request context as closely as possible when preloading routes (not just in this case, but for all routes really 🤔 )

[ockham]

setting REST_REQUEST to true right before preloading in the first place (and back to false right after -- if that's possible in PHP with define? 🤔 )

Ah nevermind, can't redefine a constant. Fair enough 😬

[felixarntz]

@ockham That's why there is this ticket: https://core.trac.wordpress.org/ticket/42061

If it was moving forward, it would allow us to more properly test REST API specific logic, as there would be a filter to use instead of being limited to the constant.

[ockham]

@ockham That's why there is this ticket: https://core.trac.wordpress.org/ticket/42061

Ah, TIL! Thank you for the pointer :)

If it was moving forward, it would allow us to more properly test REST API specific logic, as there would be a filter to use instead of being limited to the constant.

👍