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

Notice of plugins disabled in WP upgrade. #1431

Conversation

peterwilsoncc
Copy link
Contributor

Trac ticket: https://core.trac.wordpress.org/ticket/53432


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.

Comment on lines 2499 to 2503
if ( false === $deactivated_plugins ) {
// Option not in database, add an empty array to avoid extra DB queries on subsequent loads.
update_option( 'wp_deactivated_plugins', array() );
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

May be better to add an upgrade_580() to https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/includes/upgrade.php#L689. Something like:

function upgrade_580() {
    // Do add_option() to not overwrite existing option.
    add_option( 'wp_deactivated_plugins', array() );
}

@azaozz
Copy link
Contributor

azaozz commented Jun 25, 2021

Looks good imho. Just trying to think if there might be something unexpected on multisite installs, or if this should be disabled there.

Copy link
Contributor

@desrosj desrosj left a comment

Choose a reason for hiding this comment

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

Added some questions and suggestions that I had!

src/wp-admin/includes/plugin.php Outdated Show resolved Hide resolved
src/wp-admin/includes/plugin.php Outdated Show resolved Hide resolved
src/wp-admin/includes/plugin.php Outdated Show resolved Hide resolved
src/wp-admin/includes/update-core.php Show resolved Hide resolved
@peterwilsoncc
Copy link
Contributor Author

Docs fix and renaming the option pushed in e693d51 and ba0d814 respectively.

Moving back to figuring out how to deactivate on MS sub-site installs with cleaner code than https://gist.github.com/peterwilsoncc/689be020e5aa6561b0189cba59090c26

@peterwilsoncc
Copy link
Contributor Author

In 7fb3635 I've added a check to disable the plugin if it's active when the DB upgrade script runs, this is mainly for use on multisite it should be disabled by the time the code runs on a single site install.

I'm not very happy with the code but haven't been able to think of an alternative approach that might be neater.

A question to ask is whether this is even worth doing given the code has to be so ugly. The maintenance of a multisite install is considered advanced WordPress so maybe it's best handled by a dev-note and falling back to site-health pausing the plugin if the site's maintainer fails to take action.

'version_deactivated' => $gutenberg_version,
'version_compatible' => '10.8',
);
if ( is_plugin_active_for_network( 'gutenberg/gutenberg.php' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Something about this blog feels weird to me. I'm wondering if it's possible for a site to have activated a plugin and then for the plugin to have been activated network-wide and if so, what would happen here. This might be a nonissue ( that network activating a plugin causes it to be deactivated on individual sites) or this might be such an edge case that we don't need to worry about.

I think it also might make sense to abstract this out a bit into something like a wp_set_deactivated_plugin function

* @since 5.8.0
* @access private
*
* @global string $pagenow
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the @global $wp_version is also being used in the function.

@desrosj
Copy link
Contributor

desrosj commented Jun 29, 2021

A question to ask is whether this is even worth doing given the code has to be so ugly. The maintenance of a multisite install is considered advanced WordPress so maybe it's best handled by a dev-note and falling back to site-health pausing the plugin if the site's maintainer fails to take action.

I originally had pictured this as a one off check/disable that we could remove after the plugin raised its minimum WP requirement. But, I think this is much better for reuse long term.

With that in mind, I think it would be fine to include what we have here and refine later, or to circle back to multisite in 5.8.1 or 5.9 to try and come up with a more elegant solution. The function is marked as @access private, so folks shouldn't be using it (allowing us to change as needed later), but once it's released it's out in the wild in some way.

@peterwilsoncc
Copy link
Contributor Author

To get something in to RC 1, I've just pushed the following:

  • Revert changes in upgrade.php accounting for multi-site to give some additional time as to whether it ought to be accounted for or presented as a dev-note
  • Document use of $wp_version global in notice function

@peterwilsoncc
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants