-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
#36751
Conversation
What do you think about allowing patterns to also be declared in html files? |
I think that bringing back a mix of PHP files together with the HTML template files will be confusing. Specifically, the way the name of the pattern is assigned, is how we used to create page templates. |
Allowing patterns to be registered without using PHP would be a HUGE win for theme development and for every creator, designer and so on who would not have to learn PHP. |
Theoretically we could define the slug/title for patterns in |
@carolinan: I agree, but I wanted to imagine file-based registration of patterns and templates.
The ideal scenario would be to have a correct and WordPressy way of doing that. The benefit of this PR's approach is that it is 1) correct in that it uses native features of the language — specifically, PHP's multiline comments and its interpolation rules around In my mind, any correct solution using only HTML files would have to leverage our conventions around HTML comments for block demarcation. That is, from the top of my head, it could be either: <!-- wp:doctype {"type":"pattern", "title":"My heading", "categories":[...]} /-->
<!-- wp:heading -->
<h2>Hello</h2>
<!-- /wp:heading --> or: <!-- wp:doctype {"type":"pattern", "title":"My heading", "categories":[...]} -->
<!-- wp:heading -->
<h2>Hello</h2>
<!-- wp:heading /-->
<!-- /wp:doctype --> |
@mtias also suggested something as simple as opening HTML files with: <!--
Template: Name
--> I love the simplicity of it, and However, there is an unacknowledged trade-off here: do we miss out by not allowing PHP to generate templates and patterns? In my initial example, I had |
Alternative take based on #36751 (comment)
I just pushed d592853 and edited the instructions in the PR's description: you can now try adding Let me know how that feels. But personally I think the ability to pipe strings through |
While I personally love the "purity" of HTML files, patterns are used right now as the primary way to bring i18n to FSE themes. So we'd be losing that possibility. So I'm on the fence here. That said, I really like the idea of HTML metadata for templates and template parts to replace those weird indirect keys we have right now in theme.json. |
@kjellr: I'd love to move in precisely the opposite direction. :) IMO, theme.json is one of the worst places for templates — a case of "we chose this file to dump this unrelated information because the file was already there". If this patterns-based exploration proves successful, I'd like to apply the same principle to templates. |
🙌 Awesome, I agree fully. I only suggested it as a way to sync up with something that already exists. |
Does it have to be one or the other? Could it not be both? |
Do they really need to be mutually exclusive?
For ease of use, the PHP files and HTML could have the same required and optional content (name, description, category) except you can use PHP if you can and want. (I have also seen requests and questions "in the wild" about why patterns can not be registered using JavaScript) |
I worry about blurring the lines between pattern and templates too much if they are registered the same way. |
Here is the argument that comes to me: if the motivation for HTML-only files is simplicity for the theme developer, then by supporting two parallel methods we are giving up on most of that simplicity, because it conditions us down the line: we'll have to document two methods side by side, people will find tutorials in two similar-but-incompatible dialects, and our docs will have to explain why and when to make the "upgrade" from a bare HTML file to a PHP one. I personally don't know where I stand, but supporting two methods at once "smells", suggesting that we weren't happy enough with our solution or that we couldn't fully commit to it. |
I agree, but isn't WordPress past that already? We passed that when there was a split between classic editor and block editor. I know what I personally want, but it is not what is being asked for by the part of the developer community who participates here on GitHub by opening new feature requests and asking questions. |
I think this is a very different scenario. The editor split is a question of backwards compatibility, and the same applies at a smaller scale to all those APIs that we support long into the future despite their obsolescence. Meanwhile, here we have the opportunity to decide, from the start, between two concurrent APIs — or ship two APIs that we commit to supporting equally.
Could you summarise these differences for me? |
One reason for wanting PHP patterns is to work around the i18n limitations - so we can use the pattern block. In the long term I think we'll solve this problem in a better way, so we won't need PHP patterns (for this reason at least). What about if we automatically load HTML templates as suggested here, but still allow users to register their own PHP templates if they want to? |
Based on the large amount of broken theme submissions I have seen as a theme reviewer, This is not easy to balance when on the other hand, developers are requesting more features, more options, more extensibility because they feel limited and forced. |
It's definitely easier when reviewing themes to not have to worry about the security issues that come with JS and PHP files. |
That is true, I was thinking more in the line of "I have seen 101 too many PHP errors create problems for the developer and user". |
What is the expected format for adding more than one blockType? |
Emptytheme with test files: emptytheme.zip |
@carolinan indeed, I think we should move to patterns being pure html, but right now they are providing an important internationalization affordance without reinventing the wheel. Once we venture into solving localized user-content, then html patterns would be a lot more viable again and they can match the expectations of block templates. |
@carolinan: many thanks for the context on the expectations of themers and reviewers, for setting up emptytheme with mock patterns (I've edited the PR description to point to that), and for humouring my questions. Based on the latest interaction between you and @mtias, does this mean we could roll this out as PHP-only for now? Or are you set on supporting HTML-only from the start? |
It does not seem logical to me that for templates, HTML files are prioritized and the fallback is the PHP file, while for patterns, only PHP is available. What are the negative long-term consequences of supporting both? (And neither file solution can replace a feature where users create patterns in the editor) |
The Pattern block, which block templates can use, is dynamic, sourcing patterns from WP_Block_Patterns_Registry. So we need a much broader hook, such as `init`, to make sure file-based patterns are still registered on the front end.
Alternative to cb99311. In order for child themes to preserve the ability to override parent patterns, this commit also reverts to visiting `wp_get_active_and_valid_themes` in the canonical order (child then parent).
… as opposed to implicitly tying a pattern to the stylesheet as obtained via `get_stylesheet`. This may be heavy-handed, but it leaves it up to pattern and theme authors to define their naming schema. See discussion in #36751
ee5159e
to
1f06b8a
Compare
} | ||
} | ||
} | ||
add_action( 'init', 'gutenberg_register_theme_block_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.
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? But init
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 (like core/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 a patterns
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, the wp: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.
If the pattern is bundled with Gutenberg itself (like core/query-standard-posts) or with the theme, then yes, the pattern block will be rendered
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.
This is a potential decent solution for the problem mentioned here #36751 (comment)
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:
- The default ones that we want to always load (could potentially be bundled in the plugin itself but they are downloaded form the directory as convenience)
- The theme ones: themes can also refer to patterns from the directory to mark them as patterns we want to always load.
- Extra: All the remaining patterns in the directory. For these, we'd need a dedicated UI to search... directly from the directory and these don't need to be registered at all.
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:
- Theme-provided patterns should be available in the front end, both those defined by the theme and those invoked in theme.json.
- General directory patterns should not be available.
- Core patterns sourced from the directory: I would say that yes, they should be present, but we should look into caching them properly if that's not the case already. By the looks of
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.
I think for 1 and 2, I'm not sure if there should be any difference in behavior with bundled patterns.
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 to api.wordpress.org
which may take a long time (seconds) to execute, and can fail. What if api.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?
By the looks of
gutenberg_register_remote_theme_patterns
andWP_REST_Request
, it doesn't look like there's any caching
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. Unless set_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.
…tterns` directory (WordPress#36751) * Try: Block Patterns: Automatically load headered files from a theme's /patterns directory * Bump 'since' version Props gziolo Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl> * Move to compat/wordpress-6.0, prefix with "gutenberg_" Per feedback from oandregal * Support "Inserter" param * Revert to hooking to 'init' The Pattern block, which block templates can use, is dynamic, sourcing patterns from WP_Block_Patterns_Registry. So we need a much broader hook, such as `init`, to make sure file-based patterns are still registered on the front end. * Add notice reporting * Try making Slug mandatory instead * Misc. code quality and style fixes
Removing my own |
Yes, this PR is captured in WordPress/wordpress-develop#2488 which is part of that Trac ticket. |
Has this landed in GP 13? I have tried a few themes and the new pattern format is not getting picked up. Tried latest WP nightly (I think I read something about this being ported already) without and with GB 13 activated. As well as 5.9.3 with GB 13. |
I'm a bit late to the question, but this has since landed in WordPress 6.0. See the dev note. |
Part of: #36548.
This PR explores ways for themes to define and implicitly register patterns by declaring them as
eitherPHPor HTML filesunder a new/patterns
directory at the root of the theme. For the motivations behind this, refer to #36548; in short, the hope is that we could come up with a way for themes to define patterns that would be more conventional, more consistent with how block templates and parts are ultimately defined.To test,
you can use Carolina's test theme attached to this thread, orgo to the active theme's source directory, then create apatterns
directory. In it, drop the following file and name ithello.php
:At the same time, try adding an HTML-only file, naming ithello2.html
:Then load the post editor, open the patterns inserter, and look for the "My heading"
and "My other heading"pattern in the "Text" category.cc @youknowriad @mtias