Remove plugin option network-wide for Multisite during uninstall by merkys7 · Pull Request #458 · WordPress/performance · GitHub

Summary

Fixes #382

Relevant technical choices

Remove plugin option for Multisite during uninstall execution

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

@merkys7 Left some feedback.

@merkys7 Thanks for the updates. The approach here now looks good, Left some more feedback.

As per the plugin function name we should use the perflab_ prefix in name of the function. Something like perflab_delete_option or perflab_delete_plugin_option.

cc. @felixarntz

do we really need a helper function for one line of code?

In my initial approach, I don't use a helper function for a single line: #345 (comment) But I think it was used two times, so Merkys introduced a helper function. If it doesn't make sense, then can we remove it?

Thank you for the feedback, removed it

Not a blocker, but I also prefer to have a function for this. Yes, it's one line, but IMO it makes sense even to have a function for this to clarify that this needs to be exactly the same logic for a single site as well as for every site in a multisite. Plus it avoids a (admittedly tiny) chance of e.g. a typo in the option name in one of the calls, which in a function call would immediately be flagged as fatal error, but in a string isn't.

left some questions/feedback

I really appreciate your feedback @mukeshpanchal27 @adamsilverstein thank you for taking the time.
I have adjusted the PR based on the comments

@merkys7 Thanks for the updates. It looks good. I left some final feedback.

I feel like we should check the count condition inside the loop. Otherwise, it will run else part for multisite with more than 100 sites.

I would argue we don't need that check for the site count here at all. It's another query to run, which we can avoid. I think we should still handle multisites in the right way, and we can just limit the get_sites() query like you do below. That means:

For networks with 100 or fewer sites, it will wipe all sites.
For networks with more than 100 sites, it will at least wipe 100 sites.

The latter is not great, but better than what we currently have, while accounting for scalability issues. Also, deleting the option for 100 sites when there are more is still better than falling back to only deleting the option for the current site.

Also, let's add a comment here explaining this block.

Suggested change

      
            if ( is_multisite() && 100 >= get_sites( array( 'count' => true ) ) ) {
          
            // For a multisite, delete the option for all sites (however limited to 100 sites to avoid memory limit or timeout problems in large scale networks).
          
            if ( is_multisite() ) {

@merkys7 Thank you for the PR! Left some comments with additional suggestions.

I would argue we don't need that check for the site count here at all. It's another query to run, which we can avoid. I think we should still handle multisites in the right way, and we can just limit the get_sites() query like you do below. That means:

For networks with 100 or fewer sites, it will wipe all sites.
For networks with more than 100 sites, it will at least wipe 100 sites.

The latter is not great, but better than what we currently have, while accounting for scalability issues. Also, deleting the option for 100 sites when there are more is still better than falling back to only deleting the option for the current site.

I think it would be best to not have this in an else, but instead always run it. IMO it makes sense to at least always wipe the current site's (main site) option on uninstall. If a multisite has more than 100 sites, we should still ensure the main site's option is deleted.

So I would suggest the following:

Suggested change

      
            } else {
          
            	delete_option( 'perflab_modules_settings' );
          
            }
          
            }
          
            // Delete the current site's option.
          
            delete_option( 'perflab_modules_settings' );

Also, let's add a comment here explaining this block.

Suggested change

      
            if ( is_multisite() && 100 >= get_sites( array( 'count' => true ) ) ) {
          
            // For a multisite, delete the option for all sites (however limited to 100 sites to avoid memory limit or timeout problems in large scale networks).
          
            if ( is_multisite() ) {

Thank you for the feedback guys I have improved the PR based on the comments

@merkys7 Thank you for the updates, looks great now! I found one more issue that needs to be addressed.

Small aside: Please avoid force-pushing. Preferably you would simply add commits with your changes to this PR. Otherwise it messes up the git history.

@merkys7 Made the final mini updates here, so this is good to go into 1.4.0 now. Thanks again!

@felixarntz Thank you for the help and feedback!

merkys7 added [Type] Bug An existing feature is broken Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release labels Jul 28, 2022

merkys7 requested a review from felixarntz July 29, 2022 09:32

mxbclang removed the no milestone PRs that do not have a defined milestone for release label Jul 29, 2022

mxbclang added this to the 1.4.0 milestone Jul 29, 2022

mxbclang requested review from adamsilverstein, jjgrainger and mukeshpanchal27 July 29, 2022 13:30

mukeshpanchal27 requested changes Aug 1, 2022

View reviewed changes

Comment thread uninstall.php Outdated

Comment thread uninstall.php Outdated

Comment thread uninstall.php Outdated

Comment thread uninstall.php Outdated

merkys7 force-pushed the fix/382-remove-plugin-option-for-multisite branch from e0ad3d8 to f315074 Compare August 1, 2022 20:34

merkys7 requested a review from mukeshpanchal27 August 1, 2022 20:40

mukeshpanchal27 reviewed Aug 2, 2022

View reviewed changes

adamsilverstein reviewed Aug 2, 2022

View reviewed changes

merkys7 force-pushed the fix/382-remove-plugin-option-for-multisite branch from f315074 to 6fcc9c6 Compare August 5, 2022 16:29

merkys7 requested review from adamsilverstein and mukeshpanchal27 August 5, 2022 16:33

mukeshpanchal27 reviewed Aug 8, 2022

View reviewed changes

felixarntz reviewed Aug 8, 2022

View reviewed changes

merkys7 force-pushed the fix/382-remove-plugin-option-for-multisite branch from 6fcc9c6 to 1d7486b Compare August 8, 2022 17:33

mukeshpanchal27 approved these changes Aug 9, 2022

View reviewed changes

Comment thread uninstall.php Outdated

Remove plugin option for Multisite during uninstall

5aae8ad

merkys7 force-pushed the fix/382-remove-plugin-option-for-multisite branch from 1d7486b to 5aae8ad Compare August 9, 2022 07:39

felixarntz reviewed Aug 9, 2022

View reviewed changes

Comment thread uninstall.php Outdated

Comment thread uninstall.php Outdated

Fix sites loop and address final CR feedback.

a3db3a3

felixarntz approved these changes Aug 10, 2022

View reviewed changes

felixarntz merged commit 204479f into trunk Aug 10, 2022

felixarntz deleted the fix/382-remove-plugin-option-for-multisite branch August 10, 2022 14:19

felixarntz changed the title Remove plugin option for Multisite during uninstall Remove plugin option network-wide for Multisite during uninstall Aug 10, 2022

+               *
+               * @since n.e.x.t
+               */
+              function delete_data() {

	if ( is_multisite() && 100 >= get_sites( array( 'count' => true ) ) ) {
	// For a multisite, delete the option for all sites (however limited to 100 sites to avoid memory limit or timeout problems in large scale networks).
	if ( is_multisite() ) {

Remove plugin option network-wide for Multisite during uninstall by merkys7 · Pull Request #458 · WordPress/performance · GitHub

Uh oh!

Remove plugin option network-wide for Multisite during uninstall - #458

Remove plugin option network-wide for Multisite during uninstall#458felixarntz merged 2 commits intotrunkWordPress/performance:trunkfrom fix/382-remove-plugin-option-for-multisiteWordPress/performance:fix/382-remove-plugin-option-for-multisiteCopy head branch name to clipboard

Conversation

merkys7 commented Jul 28, 2022

Summary

Relevant technical choices

Checklist

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adamsilverstein left a comment

Choose a reason for hiding this comment

Uh oh!

merkys7 commented Aug 5, 2022

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merkys7 commented Aug 8, 2022

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

felixarntz left a comment

Choose a reason for hiding this comment

Uh oh!

merkys7 commented Aug 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Remove plugin option network-wide for Multisite during uninstall#458
felixarntz merged 2 commits into
trunkfrom
fix/382-remove-plugin-option-for-multisite