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 dependency support for activating performance features #1184

Merged
merged 11 commits into from
May 3, 2024

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Apr 29, 2024

Summary

This is needed for the Image Prioritizer plugin which is to be a dependent of Optimization Detective. See #1088.

  • Check for whether dependency plugins can be installed and activated when determining whether to show a feature can be activated or not.
  • Install and activate dependencies when activating a feature.

Relevant technical choices

The existing logic to check if a plugin is compatible with the current version of PHP and WP and whether the user can install or activate plugins is moved into a perflab_get_plugin_availability() helper function. This function is recursive to check not only the provided plugin slug but all of its dependencies and their dependencies. In the end, the compatible_php and compatible_wp array items returned will be true if all of the plugin's dependencies and the plugin itself are true. The same is true for the checks for whether the user can install or activate each plugin.

Additionally, the current logic to install and activate a plugin in perflab_install_activate_plugin_callback() is extracted into a new helper function perflab_install_and_activate_plugin() which is also recursive. Given the provided plugin slug, it will activate and install the provided plugin's dependencies recursively before attempting to install and activate the provided plugin.

Testing

Given a perflab_get_standalone_plugin_data() which is modified to include the woocommerce-square plugin, which is dependent on woocommerce:

--- a/load.php
+++ b/load.php
@@ -93,6 +93,10 @@ function perflab_get_standalone_plugin_data() {
 	 * - 'experimental' (boolean, optional)
 	 */
 	return array(
+		'woocommerce-square' => array(
+			'constant' => 'WOOCOMMERCE',
+			'experimental' => false,
+		),
 		'auto-sizes'              => array(
 			'constant'     => 'IMAGE_AUTO_SIZES_VERSION',
 			'experimental' => true,

Here is the behavior with this PR:

Screen.recording.2024-04-29.14.00.43.webm

@westonruter westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only labels Apr 29, 2024
@westonruter westonruter added this to the performance-lab 3.1.0 milestone Apr 29, 2024
@westonruter westonruter changed the title Implement initial dependency plugin checking for feature activation Add plugin dependency support for activating performance features Apr 29, 2024
@westonruter
Copy link
Member Author

@mukeshpanchal27 FYI: Still a work in progress, but I wanted to get your eyes on this early.

@westonruter westonruter marked this pull request as ready for review April 29, 2024 21:06
Copy link

github-actions bot commented Apr 29, 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.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: mukeshpanchal27 <mukesh27@git.wordpress.org>
Co-authored-by: adamsilverstein <adamsilverstein@git.wordpress.org>

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

@westonruter
Copy link
Member Author

The need for a progress spinner (#1139) is made even more evident here.

Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @westonruter for the PR!

Overall look good to me. I'm enable to tested old PHP/WP version plugins with dependancy as script get data from plugin repo.

Could we add notice in plugin card similar to WP core? Check screenshot.

Screenshot 2024-05-01 at 2 53 58 PM

@westonruter
Copy link
Member Author

westonruter commented May 1, 2024

@mukeshpanchal27 how did you get into that state where the dependent plugin is active but the dependency plugin is not? Normally this wouldn't be possible, right?

* @param string $plugin_slug Plugin slug.
* @return WP_Error|null WP_Error on failure.
*/
function perflab_install_and_activate_plugin( string $plugin_slug ): ?WP_Error {
Copy link
Member

@adamsilverstein adamsilverstein May 1, 2024

Choose a reason for hiding this comment

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

Should we add protection against infinite recursion here by ensuring we only handle a given plugin slug once at most. In theory if you had a plugin dependency chain that went back to the top plugin, this function would never exit (A->B->C->A). Track which slug has been handled and return early if a handled slug is called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. I was thinking core didn't account for this, but I stand corrected:

image

Since our plugin has to support 6.4 still we can't reuse the validity-checking logic that core has, but it is overkill for our purposes here since we are managing all of the plugins that we list here. For that matter, checking for recursion may also be overkill since again we're managing all of these plugins, so we hopefully won't make such a recursive error when we list out the Requires Plugins field 😄

Copy link
Member

Choose a reason for hiding this comment

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

Right, it was mostly a reflexive comment as a general rule for recursive functions - make sure they don't infinitely recurse.

I agree since we control all the plugins here we are unlikely to introduce a circular dependency. Still the code change would be relatively small so maybe worth including anyway? I don't feel strongly so I'll leave it up to you.

Copy link
Member

Choose a reason for hiding this comment

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

not sure how core does it, maybe something like https://github.com/WordPress/performance/pull/1197/files

Copy link
Member Author

Choose a reason for hiding this comment

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

@adamsilverstein I had a great conversation with Gemini about this: https://g.co/gemini/share/100242fb5096

I asked about how to address the circular dependency problem and it suggested a dependency tracker as an argument to the function, passed by reference. After some back and forth, I've come up with this: 1362cef.

Copy link
Member

Choose a reason for hiding this comment

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

had a great conversation with Gemini about this: g.co/gemini/share/100242fb5096

haha, that conversation is quite funny, thanks for sharing. The tips and feedback provided seem pretty through and insightful, I especially like the table with pros and cons of various approaches. Then at then end where it turns out it got some stuff wrong and you admonish it, pure gold.

I've come up with this: 1362cef.

Thats perfect, thanks. Passing the processes list through as a parameter by reference is smart.

@westonruter
Copy link
Member Author

Could we add notice in plugin card similar to WP core? Check screenshot.

@mukeshpanchal27 I've accounted for this in 7a5c9b0. If somehow a site gets into a state where the dependency is deactivated but the dependent is active, then the feature will be listed as if it was not active in the first place. The "Activate" button will then be shown:

Before After
Screenshot 2024-05-01 11 18 13 image

Copy link
Member

@adamsilverstein adamsilverstein left a comment

Choose a reason for hiding this comment

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

Terrific!

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Awesome ❤️

@mukeshpanchal27 mukeshpanchal27 merged commit 9224f29 into trunk May 3, 2024
17 checks passed
@mukeshpanchal27 mukeshpanchal27 deleted the add/feature-dependencies branch May 3, 2024 04:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants