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

Add plugin template registration API #7125

Closed

Conversation

Aljullu
Copy link

@Aljullu Aljullu commented Aug 1, 2024

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.

Copy link

github-actions bot commented Aug 1, 2024

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props aljullu, peterwilsoncc, antonvlasenko, azaozz, youknowriad, noisysocks.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@Aljullu Aljullu force-pushed the add/plugin-template-registration-api branch from 56b304f to 1cf666a Compare August 1, 2024 14:19
@Aljullu
Copy link
Author

Aljullu commented Aug 1, 2024

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

@youknowriad
Copy link
Contributor

@Aljullu yes, that's fine, it means this PR can only be merged after the package update on Core.

Copy link

github-actions bot commented Aug 9, 2024

Test using WordPress Playground

The 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

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

peterwilsoncc
peterwilsoncc previously approved these changes Aug 27, 2024
Copy link
Contributor

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a few notes for the wp-dev version.

src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved
src/wp-includes/block-template.php Outdated Show resolved Hide resolved
src/wp-includes/block-template.php Outdated Show resolved Hide resolved
src/wp-includes/class-wp-block-templates-registry.php Outdated Show resolved Hide resolved
@peterwilsoncc peterwilsoncc dismissed their stale review August 27, 2024 23:36

Meant to comment, not approve.

@Aljullu
Copy link
Author

Aljullu commented Aug 28, 2024

Thanks for the review @peterwilsoncc! I have applied all your feedback.

Copy link

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had a chance to do a thorough review, but this caught my attention.
Thank you for considering my notes.

Comment on lines +881 to +895
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;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@azaozz azaozz Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +881 to +895
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;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@noisysocks
Copy link
Member

Although this needs a little work before landing in the final release, I am a little tempted to merge the code in it's current form while performance is figured out so we can merge in the inital package update. What do you think @azaozz @noisysocks?

If there's no outstanding blocking comments I'm ok with this.

@anton-vlasenko
Copy link

anton-vlasenko commented Sep 9, 2024

Although this needs a little work before landing in the final release, I am a little tempted to merge the code in it's current form while performance is figured out so we can merge in the inital package update. What do you think @azaozz @noisysocks?

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 (WP_Block_Template::$plugin).
I also have concerns regarding #7125 (comment) (might not work as expected).
In my opinion, this PR may not be ready to be merged in its current state.

@Aljullu
Copy link
Author

Aljullu commented Sep 9, 2024

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.

@azaozz
Copy link
Contributor

azaozz commented Sep 9, 2024

Although this needs a little work before landing in the final release, I am a little tempted to merge the code in it's current form while performance is figured out

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.

@peterwilsoncc
Copy link
Contributor

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.

Copy link

@anton-vlasenko anton-vlasenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Aljullu Please find another review below. Apologies for not including it with the previous one, as I just noticed it.

src/wp-includes/block-template-utils.php Outdated Show resolved Hide resolved
@anton-vlasenko
Copy link

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.

@peterwilsoncc I don't see any blockers at the moment. Thanks!

@peterwilsoncc
Copy link
Contributor

I've merged in trunk and pushed a couple of test nitpicks in e8b2b2c and some docs standards fixes in 9a18a25

@peterwilsoncc
Copy link
Contributor

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?

@noisysocks
Copy link
Member

Looks OK to me. We'll have to wait until packages are synced before we commit.

@noisysocks
Copy link
Member

noisysocks commented Sep 20, 2024

Committed in r59073.

@noisysocks noisysocks closed this Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants