Skip to content
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

Merged
merged 19 commits into from
Mar 17, 2022

Conversation

mcsf
Copy link
Contributor

@mcsf mcsf commented Nov 22, 2021

Part of: #36548.

This PR explores ways for themes to define and implicitly register patterns by declaring them as either PHP or HTML files under 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, or go to the active theme's source directory, then create a patterns directory. In it, drop the following file and name it hello.php:

<?php
/**
 * Title: My heading
 * Slug: my-theme/my-heading
 * Categories: text
 */

?>
<!-- wp:heading -->
<h2>Hello!</h2>
<!-- /wp:heading -->
<!-- wp:paragraph -->
<p>2 + 2 = <?php echo 2 + 2; ?></p>
<!-- /wp:paragraph -->

At the same time, try adding an HTML-only file, naming it hello2.html:

<!--
	Pattern Name: My other heading
	Categories: text
-->
<!-- wp:heading -->
<h2>Another heading</h2>
<!-- /wp:heading -->

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

@scruffian
Copy link
Contributor

What do you think about allowing patterns to also be declared in html files?

@carolinan
Copy link
Contributor

carolinan commented Nov 23, 2021

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.
In other words, not allowing page templates to be created the "traditional way" but using that same way to create patterns is what is confusing.
(If you add that type of comment to a HTML template file, the editor thinks it is a classic block)

@carolinan
Copy link
Contributor

carolinan commented Nov 23, 2021

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.

@kjellr
Copy link
Contributor

kjellr commented Nov 23, 2021

Theoretically we could define the slug/title for patterns in theme.json somewhere, and then use plain html files for the patterns? We already do essentially the same thing with templates.

@mcsf
Copy link
Contributor Author

mcsf commented Nov 23, 2021

I think that bringing back a mix of PHP files together with the HTML template files will be confusing.

@carolinan: I agree, but I wanted to imagine file-based registration of patterns and templates.

What do you think about allowing patterns to also be declared in html files?

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 <?php and ?> tokens — and 2) WordPressy in that it uses get_file_data and conventional file headers.

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 -->

@mcsf
Copy link
Contributor Author

mcsf commented Nov 23, 2021

@mtias also suggested something as simple as opening HTML files with:

<!--
    Template: Name
-->

I love the simplicity of it, and get_file_data works just as well on this. The downside is that we lose the ability to neatly separate metadata from content which we got by calling include $file and capturing that output. It's nothing insurmountable, of course: we could either manually discard any content preceding the first occurrence of -->, or pipe the entire file contents through the low-level block parser if we want more power.

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 <p>2 + 2 = <?php echo 2 + 2; ?></p>. This ties into past conversations around dynamic inline content, inline tokens, i18n, etc.

mcsf added a commit that referenced this pull request Nov 23, 2021
@mcsf
Copy link
Contributor Author

mcsf commented Nov 23, 2021

I just pushed d592853 and edited the instructions in the PR's description: you can now try adding .php and .html templates side by side in the /patterns directory.

Let me know how that feels. But personally I think the ability to pipe strings through __ with PHP is pretty important.

@youknowriad
Copy link
Contributor

youknowriad commented Nov 23, 2021

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.

@mcsf
Copy link
Contributor Author

mcsf commented Nov 23, 2021

Theoretically we could define the slug/title for patterns in theme.json somewhere, and then use plain html files for the patterns? We already do essentially the same thing with templates.

@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.

@kjellr
Copy link
Contributor

kjellr commented Nov 23, 2021

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.

@scruffian
Copy link
Contributor

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.

Does it have to be one or the other? Could it not be both?

@carolinan
Copy link
Contributor

do we miss out by not allowing PHP to generate templates and patterns?

Do they really need to be mutually exclusive?
I agree that we need the ability to register patterns with PHP content.
I would like for the following to be possible:

  • Use the current PHP functions that are available
  • Use the patterns folder with either PHP files or HTML files.

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)

@carolinan
Copy link
Contributor

I worry about blurring the lines between pattern and templates too much if they are registered the same way.
And I can worry that if we, in lack of better terms, keep moving back to PHP, block themes will loose what made them easy to create.

@mcsf
Copy link
Contributor Author

mcsf commented Nov 24, 2021

do we miss out by not allowing PHP to generate templates and patterns?

Do they really need to be mutually exclusive? I agree that we need the ability to register patterns with PHP content. I would like for the following to be possible:

  • Use the current PHP functions that are available
  • Use the patterns folder with either PHP files or HTML files.

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.

@carolinan
Copy link
Contributor

carolinan commented Nov 24, 2021

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.

@mcsf
Copy link
Contributor Author

mcsf commented Nov 24, 2021

We passed that when there was a split between classic editor and block editor.

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.

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.

Could you summarise these differences for me?

@scruffian
Copy link
Contributor

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?

@carolinan
Copy link
Contributor

carolinan commented Nov 24, 2021

Could you summarise these differences for me?

Based on the large amount of broken theme submissions I have seen as a theme reviewer,
simplicity reduces the risk of making mistakes.
If we can reduce the risk of the developer making mistakes, then we can take that burden away from that developer, and the end product may in many cases have a higher quality.
That is why I would choose a non PHP - and non JS - solution when possible.

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.

@scruffian
Copy link
Contributor

It's definitely easier when reviewing themes to not have to worry about the security issues that come with JS and PHP files.

@carolinan
Copy link
Contributor

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".

@carolinan
Copy link
Contributor

The discussion about whether block themes should use PHP or HTML is larger than patterns, and -disclaimer, - If I understood @mtias correctly, as of #36548, the decision to support both has already been made.

I'll do my part to try to move the conversation back to the PR and test the update.

@carolinan
Copy link
Contributor

What is the expected format for adding more than one blockType?

@carolinan
Copy link
Contributor

carolinan commented Nov 25, 2021

Emptytheme with test files: emptytheme.zip
Twenty Twenty with test files (for testing a classic theme)
twentytwenty.zip

@mtias
Copy link
Member

mtias commented Nov 25, 2021

@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.

@mtias mtias added the [Status] In Progress Tracking issues with work in progress label Nov 25, 2021
@mcsf
Copy link
Contributor Author

mcsf commented Nov 25, 2021

@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?

@carolinan
Copy link
Contributor

carolinan commented Nov 29, 2021

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
@mcsf mcsf force-pushed the try/block-patterns-as-theme-files branch from ee5159e to 1f06b8a Compare March 17, 2022 17:51
@mcsf mcsf merged commit ae02122 into trunk Mar 17, 2022
@mcsf mcsf deleted the try/block-patterns-as-theme-files branch March 17, 2022 19:59
@github-actions github-actions bot added this to the Gutenberg 12.9 milestone Mar 17, 2022
}
}
}
add_action( 'init', 'gutenberg_register_theme_block_patterns' );
Copy link
Member

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.

Copy link
Contributor

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)

Copy link
Member

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.

Copy link
Contributor

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)

Copy link
Member

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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 and WP_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.

Copy link
Member

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 and WP_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.

jostnes pushed a commit to jostnes/gutenberg that referenced this pull request Mar 23, 2022
…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
@priethor priethor removed the [Status] In Progress Tracking issues with work in progress label Mar 28, 2022
@bph bph assigned mcsf Apr 6, 2022
@mcsf mcsf added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Apr 7, 2022
@mcsf
Copy link
Contributor Author

mcsf commented Apr 7, 2022

Removing my own backport labelling, as I believe these changes are already batched in https://core.trac.wordpress.org/ticket/55505 — correct, @hellofromtonya?

@mcsf mcsf removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Apr 7, 2022
@hellofromtonya
Copy link
Contributor

Removing my own backport labelling, as I believe these changes are already batched in https://core.trac.wordpress.org/ticket/55505 — correct, @hellofromtonya?

Yes, this PR is captured in WordPress/wordpress-develop#2488 which is part of that Trac ticket.

@nextgenthemes
Copy link

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.

@mcsf
Copy link
Contributor Author

mcsf commented Sep 23, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Pattern Affects the Patterns Block Needs Dev Note Requires a developer note for a major WordPress release cycle [Type] New API New API to be used by plugin developers or package users.
Projects
No open projects
Status: Ready for Review
Archived in project
Development

Successfully merging this pull request may close these issues.