Add plugin template registration API #7125
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Aljullu
force-pushed
the
add/plugin-template-registration-api
branch
from
56b304f
to
1cf666a
Compare
|
@youknowriad I've tested the PHP APIs in this PR and they are working as expected. Unfortunately, I didn't find a way to reproduce all the testing steps from WordPress/gutenberg#61577. This PR doesn't include the JS changes from the original PR so template revert/removal doesn't work. Is it expected that this kind of PRs can't be tested right away, or am I missing something? |
|
@Aljullu yes, that's fine, it means this PR can only be merged after the package update on Core. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
…plugin-registered title and description
Co-authored-by: Peter Wilson <519727+peterwilsoncc@users.noreply.github.com>
|
Thanks for the review @peterwilsoncc! I have applied all your feedback. |
src/wp-includes/rest-api/endpoints/class-wp-rest-templates-controller.php
Outdated
Show resolved
Hide resolved
| foreach ( $plugins as $plugin_file ) { | ||
| $plugin_basename = plugin_basename( $plugin_file ); | ||
| // Split basename by '/' to get the plugin slug. | ||
| list( $plugin_slug, ) = explode( '/', $plugin_basename ); | ||
|
|
||
| if ( $plugin_slug === $template_object->plugin ) { | ||
| $plugin_data = get_plugin_data( $plugin_file ); | ||
|
|
||
| if ( ! empty( $plugin_data['Name'] ) ) { | ||
| return $plugin_data['Name']; | ||
| } | ||
|
|
||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems somewhat slow, mostly the get_plugin_data(). May be missing something but seems this is supposed to run on every page load for all templates that have a "plugin" in the settings. If that's the case, perhaps some caching would be nice here?
Or even better, is the readable plugin author name always necessary? Would it make sense to retrieve it only when it's needed, otherwise just return whatever is in $template_object->plugin?
There was a problem hiding this comment.
I agree with Ozz that it would be good to avoid the performance impact here, the function is in the admin folder section because it's very slow.
There was a problem hiding this comment.
Thanks for the comments!
Or even better, is the readable plugin author name always necessary? Would it make sense to retrieve it only when it's needed, otherwise just return whatever is in $template_object->plugin?
This code runs only when querying the templates endpoint. That means the code does not run on the frontend or on most wp-admin pages. It runs in the Site Editor, where the template author text is displayed in several UI elements, so I'm not sure how it can be optimized in that sense.
When making a call to the templates endpoint, this specific code path can be deactivated if the author_text attribute is not requested. That could be done in some specific cases, like when loading templates for the Swap template popup in the Post Editor, as AFAIK we don't display the author text anywhere in the UI, but I might be wrong. 🤔
If that's the case, perhaps some caching would be nice here?
I agree that caching could help. Since this is my first PR of this size in WP core, I'd appreciate any specific recommendations on implementing caching in this context. My initial thought is to store a transient that maps plugin slugs to plugin names, which would be invalidated whenever a plugin is activated, deactivated, or updated. But wondering if you have any better suggestions. 🙂
There was a problem hiding this comment.
My initial thought is to store a transient that maps plugin slugs to plugin names, which would be invalidated whenever a plugin is activated, deactivated, or updated.
The various hooks for when a plugin is updated only run if the plugin is updated via the Dashboard. In the case of sites deployed by git, rsync or good old FTP this isn't the case which makes caching these functions persistently via a transient or the object cache unworkable.
There was a problem hiding this comment.
This code runs only when querying the templates endpoint. That means the code does not run on the frontend
Thanks for the clarification. As peterwilsoncc pointed out above when plugins are updated by FTP (uploading) or from version control, WP doesn't know it unless an admin visits the Plugins screen. This makes caching of the plugins meta somewhat impractical as that cache will always be stale in some cases/on some sites.
On the other hand mapping the plugin's name to the plugin's slug may be acceptable even if a bit stale. It is very rare that a plugin is renamed, so probably can use a transient that expires every week or so? That may be an acceptable compromise as this information doesn't seem critical.
| foreach ( $plugins as $plugin_file ) { | ||
| $plugin_basename = plugin_basename( $plugin_file ); | ||
| // Split basename by '/' to get the plugin slug. | ||
| list( $plugin_slug, ) = explode( '/', $plugin_basename ); | ||
|
|
||
| if ( $plugin_slug === $template_object->plugin ) { | ||
| $plugin_data = get_plugin_data( $plugin_file ); | ||
|
|
||
| if ( ! empty( $plugin_data['Name'] ) ) { | ||
| return $plugin_data['Name']; | ||
| } | ||
|
|
||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
I agree with Ozz that it would be good to avoid the performance impact here, the function is in the admin folder section because it's very slow.
| * Fall back to the theme name if the plugin is not defined. That's needed to keep backwards | ||
| * compatibility with templates that were registered before the plugin attribute was added. | ||
| */ | ||
| $plugins = get_plugins(); |
There was a problem hiding this comment.
As above with get_plugins(), it gets a file listing of wp-content/plugins/*, filters for PHP files and opens each of them to look for a plugin header. In the case of Jetpack, this is 38 files.
get_plugins() is cached but the plugins group isn't persistent so the code runs on each http request calling the function.
There was a problem hiding this comment.
In cases where a plugin is using the API and setting the namespace to match its slug, we will have already returned in this function, so this code would never run.
The main reason to keep it is for backwards compatibility, as this code already exists in WP 6.6 and some plugins rely on it: they don't use the new template registration API but they still set the template source to plugin.
…troller.php Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
If there's no outstanding blocking comments I'm ok with this. |
@noisysocks Speaking of blocking comments, the code currently has some compatibility issues with PHP 8.2, as it introduces a dynamic class property ( |
Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
|
Thanks for the review @anton-vlasenko! I have addressed all your feedback, I would appreciate it if you can take another look. There is also this open discussion about finding a more performant way to retrieve the plugin author text, but I might need some assistance on how to proceed. Also I don't know if it's a blocker. |
Sounds good to me too after all of @anton-vlasenko's suggestions and fixes were added. Would be nice to address/fix the performance improvements from above but as that affect only the site editor don't think it is a blocker. |
|
I've merged trunk in and renamed a class in the tests folder (ie, it won't effect your source code). If @anton-vlasenko is happy with the change a few hours ago then I think it's good to go. |
There was a problem hiding this comment.
@Aljullu Please find another review below. Apologies for not including it with the previous one, as I just noticed it.
tests/phpunit/tests/block-templates/WpBlockTemplatesRegistry.php
Outdated
Show resolved
Hide resolved
Co-authored-by: Anton Vlasenko <43744263+anton-vlasenko@users.noreply.github.com>
@peterwilsoncc I don't see any blockers at the moment. Thanks! |
|
I think this is good to go in, @noisysocks are you able to take a look to make sure I am not missing something obvious that will effect the editor? |
|
Looks OK to me. We'll have to wait until packages are synced before we commit. |
|
Committed in r59073. |
This PR includes the changes that need to be backported from WordPress/gutenberg#61577 and WordPress/gutenberg#64610 into WC core.
Trac ticket: https://core.trac.wordpress.org/ticket/61804
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.