Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block Patterns: Automatically load headered files from a theme's
/patterns
directory #36751Block Patterns: Automatically load headered files from a theme's
/patterns
directory #36751Changes from all commits
ba1bbc6
98fb903
a585298
fb8cce6
f666088
388e8d1
72ab526
a915083
f06c16b
2e2fa4c
556e1e2
7b5a086
3df71d9
bc8471f
288e7f5
cd97aef
5d4594d
1f06b8a
4c48632
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Registering the patterns on
init
might mean we're registering them on too many pages, even if we don't need them. Only the block editor UI page (post or site) needs block patterns, is that right? Butinit
runs on all pages, like the site frontend and many other.Other pattern registration functions run on
current_screen
and check if$current_screen->is_block_editor()
is true. See #39493 where a similar fix was done recently.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been tried to be solved above but apparently we need the patters also on frontend, so checking current screen doesn't work here #36751 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, a template can contain a
<!-- wp:pattern {"slug":"foo"} /-->
markup and then the server will render the pattern's content. But for which patterns does this work reliably? If the pattern is bundled with Gutenberg itself (likecore/query-standard-posts
) or with the theme, then yes, the pattern block will be rendered.But what about remote patterns? A
theme.json
can declare apatterns
field that contains merely slugs, not the full patterns. These need to be downloaded from api.wordpress.org. Then there are "core" and "featured" patterns that the block editor UI will dynamically download and register for you.When the
slug
refers to such a remote pattern, thewp:pattern
block will either be not rendered at all, or the frontend page rendering will be blocked on an api.wordpress.org REST request that the server needs to perform before it can proceed with rendering. And that sounds like something very undesired, doesn't it?Seems we'll need to draw some sharp line between bundled and remote patterns. One can work with
<!-- wp:pattern /-->
markup, the other is exclusively a block editor UI thing. Remote patterns are ephemeral and references to them should not be persisted anywhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is meant for themes essentially as a way to allow i18n for block templates. So the main use-cases is for the patterns registered by themes (like the current PR).
This is a potential decent solution for the problem mentioned here #36751 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that there's a strong need for the
register_block_patterns
action. It makes it obvious how to register a pattern. But @mcsf's comment doesn't distinguish between bundled an remote patterns. Registering bundled patterns means executing non-controversial PHP code, but registering remote patterns means remote network requests and blocking until they finish. May be need two register actions?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are three kind of remote patterns:
I think for 1 and 2, I'm not sure if there should be any difference in behavior with bundled patterns. The directory there just serves as a shortcut to avoid building these in the theme or core itself.
For 3. We don't need any action or any specific treatment, we just load these on demand from the editor.
So I think in the end, only one action should be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally agree:
gutenberg_register_remote_theme_patterns
andWP_REST_Request
, it doesn't look like there's any caching, but patterns is the sort of thing we don't need to pull more than once daily, IMO. This caching should also apply to theme.json-invoked patterns.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remote patterns, however, are quite different in terms of performance and reliability. When serving a request to render, say, a site homepage, rendering a
wp:pattern
block may involve a request toapi.wordpress.org
which may take a long time (seconds) to execute, and can fail. What ifapi.wordpress.org
is down? Currently the pattern block will be silently not rendered at all, and the incorrect result will likely get cached.When rendering a frontend page, a reasonable expectation is that the server needs to access only reliable "local" resources, like filesystem or the MySQL database?
The pattern directory responses are cached for one hour with
set_site_transient
. That means we refresh the patterns once an hour, while serving a (random) request. Unlessset_site_transient
is broken, like it was on WP.com as @tyxla discovered 🙃, then there's no caching.Ideally, there should be some autoupdate job that would download and sync the patterns to local FS or database, decoupled from serving frontend requests. Then the frontend request processing always works with local resources, and is more consistent and reliable.