Block Patterns: Load patterns from wordpress.org API #28800
Conversation
ryelle
added
the
[Feature] Pattern Directory
|
Size Change: 0 B Total Size: 1.62 MB ℹ️ View Unchanged
|
ryelle
force-pushed
the
try/remote-block-patterns
branch
from
4571c1b
to
c41f48d
Compare
lib/block-patterns.php
Outdated
| 'init', | ||
| 'admin_init', |
There was a problem hiding this comment.
I don't think this should change, but if I leave it as init, I get this error when running the unit tests:
Fatal error: Uncaught Error: Class 'Spy_REST_Server' not found in /var/www/html/wp-includes/rest-api.php:509
This was happening locally and in the PR's unit test action.
There was a problem hiding this comment.
Would it be worth using a more restrictive hook so that the API call isn't being made on admin screens that don't use patterns? In WordPress.com, where we use a similar approach to load patterns from an API endpoint we decided to use the current_screen hook and check for is_block_editor or gutenberg_is_edit_site_page() so that we only call the endpoint on views that need it.
There was a problem hiding this comment.
@andrewserong has a good point on employing a more restrictive hook, Before we used an API call it didn't matter, but now it does. Also, we're prefetching a lot of data in the Gutenberg editors. Shouldn't this be treated the same way?
There was a problem hiding this comment.
This data isn't used the same way as the other prefetched data - it's not used in gutenberg directly, it's only used on the server-side to register patterns. Once the pattern directory is more dynamic, and block patterns are registered in JS, we can switch to something more like the other prefetched endpoints.
In 89045a6, I updated the code here to use the current_screen approach for loading pattern content, but left the unregistration on init, to match what's happening in core now.
lib/block-patterns.php
Outdated
| require __DIR__ . '/block-patterns/' . $core_block_pattern . '.php' | ||
| ); | ||
| $request = new WP_REST_Request( 'GET', '/__experimental/pattern-directory/patterns' ); | ||
| $request->set_param( 'keyword', 11 ); // 11 is the ID for "core". |
There was a problem hiding this comment.
Just noting that without the comment this would be impossible to understand since the keyword ID lives outside of this project. Additionally, if any changes happen in the directory, this would return incorrect patterns. I think for now it's ok, but if core patterns maintain priority in the future, we may want to consider a different approach to loading core patterns that doesn't rely on a hard coded id.
There was a problem hiding this comment.
That's why I left the comment 🙂
This really is a "for now" kind of feature — to move the management of these core patterns into the block pattern directory. The longer-term plan is for a more dynamic search UI that fetches from the API on the client side (like the block directory), any prioritization would happen on the API side, so Gutenberg wouldn't need to be aware of this category at all.
There was a problem hiding this comment.
Can we make that 11 be a variable with a good name like $core_keyword_id = 11 and also have that comment?
There was a problem hiding this comment.
How come the param named keyword is a number? I would expect a string for keyword ...
There was a problem hiding this comment.
It's a taxonomy on wp.org/patterns. Is that a problem? I was trying to make the least changes to the wp.org api, but this could probably be updated if so. cc @iandunn
There was a problem hiding this comment.
It's been awhile, but if I'm reading everything correctly, I think we could rename the keyword param to keyword_id (or maybe plural), and it'd only effect WP_REST_Pattern_Directory_Controller::get_items().
On the API side it's pattern-categories, and get_items() maps between the two.
Using a slug would be nice, but IIRC the REST API only accepts IDs, unless we add custom code. I haven't checked that, though.
Either way, IMO it's best to be practical in this case. As long as variable names and other easily updated things are clear, I think that's good enough. No need to get bogged down over something small.
There was a problem hiding this comment.
Hi there, I hope you don't mind the drive-by commenting, but I thought I'd add a couple of tiny comments based on some experiences building a similar feature into a plugin in WordPress.com, that I thought could be relevant. Feel to disregard, of course! And I'm very excited and curious to see how the patterns directory integration progresses 🙂
lib/block-patterns.php
Outdated
| 'init', | ||
| 'admin_init', |
There was a problem hiding this comment.
Would it be worth using a more restrictive hook so that the API call isn't being made on admin screens that don't use patterns? In WordPress.com, where we use a similar approach to load patterns from an API endpoint we decided to use the current_screen hook and check for is_block_editor or gutenberg_is_edit_site_page() so that we only call the endpoint on views that need it.
| 'core/' . $core_block_pattern, | ||
| require __DIR__ . '/block-patterns/' . $core_block_pattern . '.php' | ||
| ); | ||
| $request = new WP_REST_Request( 'GET', '/__experimental/pattern-directory/patterns' ); |
There was a problem hiding this comment.
Another issue we ran into over in WordPress.com was calling the endpoint on every request for the admin page. We decided to guard the call behind a wp_cache_get check and then wp_cache_set the results from the API endpoint for a period of time so that each admin page request doesn't fire off an external API call.
There was a problem hiding this comment.
I would also recommend using some caching around rest_do_request() in this case, a transient makes sense to me. I don't consider this to be a blocker for merge though.
There was a problem hiding this comment.
Should we do the caching in the actual endpoint around the API call to pattern directory? I think we'd want to avoid this returning different data from the endpoint?
| @@ -84,7 +84,7 @@ public function get_items_permissions_check( $request ) { // phpcs:ignore Variab | |||
| * @return WP_Error|WP_REST_Response Response object on success, or WP_Error object on failure. | |||
| */ | |||
| public function get_items( $request ) { | |||
| $query_args = array(); | |||
| $query_args = array( 'per_page' => 100 ); | |||
There was a problem hiding this comment.
Since Gutenberg is the primary consumer of the api.wordpress.org/patterns/ API, could this be made the default of the API instead of passing it?
There was a problem hiding this comment.
Removed - f57bafa
I haven't updated the wporg api yet.
|
@StevenDufresne The only major concern regarding the API is to ensure that we have proper caching on the endpoints (both on WordPress.org, and within Core for anything that is called on every editor load) before the Gutenberg release is included in a WordPress release. I've left some comments on a few things I'd like to change, but they're not blockers for merge, but would preferably be taken into account. |
|
I tested this and it does not seem to break or make any difference other than for me the patterns' previews loaded slower. |
|
The pattern previews loaded slower for me as well (although I understand they shouldn't as patterns are preloaded server-side), which sometimes results in a visual glitch when there are many patterns in a category and once the previews finish loading: Grabacion.de.pantalla.2021-05-18.a.las.13.49.02.movI can't reproduce it consistently on local environments, but on Gutenberg.run it happens every time, both in Chrome and Firefox. |
|
@youknowriad if this is merged as a Gutenberg experiment until it gets fixed, can it still land in 5.8? Or does it have to be merged as a feature now? |
|
Historically, features need to land as stable and in Core (not just the plugin) before the feature freeze to make it to the WordPress release. While I'm really excited about this feature, late features always make me nervous so I'm wondering whether it's wiser to defer until 5.9 to give it time to mature. How confident are you all about its current state? |
ryelle
force-pushed
the
try/remote-block-patterns
branch
from
dd3e594
to
f57bafa
Compare
Caching still needs to be enabled on wp.org (see WordPress/pattern-directory#99). I added an hour transient cache for the pattern request from the editor, will that work?
Does that happen on trunk? This PR isn't touching any of the query blocks, so I'm not sure why that would happen here. They also shouldn't be loading any slower… the only change here is server-side, it shouldn't affect the loading once the patterns are set into
This was intended to lay some groundwork for more dynamic block patterns, and make it easier to manage the core patterns. There shouldn't be any functionality change in gutenberg, it should just swap out the pattern files for the same data from an API. I think the only outlying issue is the perceived slowness. If @draganescu or @priethor could re-test, maybe the issue was how out-of-date the branch was. |
The API side is solid enough. For this PR it only ever needs to return the core patterns, which it does already. That can easily be cached (or even made static) on the server side for performance. Getting this in to 5.8 means two things: core patterns can then be improved and updated independent of the Gutenberg and WP release cycles. And, since this PR contains the basic core of the necessary editor-side code, adding support for third-party patterns in 5.9 will become easier. Defering this patch to 5.9 means that wide testing of remotely-fetched core patterns doesn't start until the 5.9 beta cycle, and that probably pushes work on third-party patterns to the next release cycle after that. |
|
@ryelle @tellyworth sounds good to me, I'll defer this to you all on this. |
|
Does it mean we need a follow-up trac ticket to remove the bundled patterns from there as well? (probably before next Tuesday) |
youknowriad
force-pushed
the
try/remote-block-patterns
branch
from
e48faf8
to
1ae9b70
Compare
|
The e2e failure here sounds legit. (adding patterns). Anyone to take a look? |
|
yeah, I can fix it. |
This bumps the max patterns returned in a query to 100, which should be enough to return all patterns, since we have a relatively low number for now. See WordPress/gutenberg#28800 (comment) git-svn-id: https://meta.svn.wordpress.org/sites/trunk@10986 74240141-8908-4e6f-9713-ba540dce6ec7
Later we might want to remove this or set it to something smaller, but for now we want to pre-load the set of ~15 core patterns.
noisysocks
force-pushed
the
try/remote-block-patterns
branch
from
0c57f1a
to
bf4af7a
Compare
|
I rebased this to see if it fixes E2E failures. |
This reverts commit 57f7791.
|
I just noticed this introduces some PHP warnings which appear on pages without a block editor. I've fixed them in #32025.
|
… to patterns directory to fetch and then register patterns. This commit provides filters to opt out of: 1. removing core patterns, in case Gutenberg users want to use Core patterns 2. adding Gutenberg patterns 3. loading core patterns from remote source
Description
As part of the WordPress.org pattern directory project, we want to load patterns remotely from WordPress.org — this PR is a first step to get there. It unregisters & removes the existing core patterns, and pulls down the patterns from an API on wordpress.org/patterns. These are the same patterns that were just added in #29973. With this change, patterns can be added or updated without needing to release new WordPress or Gutenberg versions.
Right now this preloads the patterns server-side, so functionally there should be no change to the editing experience. Eventually we'll want to load dynamically, client-side, so that authors can search & filter through all patterns on wordpress.org/patterns. See WordPress/pattern-directory#10
How has this been tested?
Note: The patterns are rendered as a classic blocks for now, that will be fixed with #28799 - leaving this PR in draft until then.This was fixed.Note 2: There are two patterns - heading and quote - which currently add
blockTypessupport for the heading and quote blocks, respectively. we don't have a way of flagging that on the pattern directory yet, so they're registered without it. See WordPress/pattern-directory#33 (comment)