Fatal error during update to 5.8 of a site with an active Gutenberg plugin (version less than 10.7)

[oglekler]

Versions of Gutenberg less than 10.7.4 have WP_Block_Template which is now in Core in WordPress 5.8 and without a checkup that this block is already active on a site (or some other compatibility precautions), it crushes with Fatal Error.

[oglekler]

Fatal error during update to 5.8 of a site with an active Gutenberg plugin (version 10.3.1):

[hellofromTonya]

I was able to reproduce the problem with Gutenberg plugin versions less than 10.7, but not with 10.7+.

Why does this happen?

The plugin already has the class loaded into memory. With 5.8, the class has been merged into core and is loaded by core. When updating to 5.8, there's a naming conflict as PHP does not allow overwriting a class (or function, trait, constant, etc.) once it's in memory. In this case, it's already in memory (via the plugin) when core tries to load it. Fatal error.

Different solutions were discussed in slack (​see the thread here) including guarding the load circuit in core. But a simple solution was proposed using ​a similar approach as this to deactivate the Gutenberg plugin when the version is not compatible. In this case, < 10.7 is not compatible with WP 5.8. This solution would prevent other potential fatal errors of other files loaded later that might also be incompatible.

[hellofromTonya]

Adds plugin deactivation on core update when Gutenberg plugin is a version <= 10.7

[hellofromTonya]

The patch implements the suggestion from @jorbin (props to him) to deactivate the plugin. After further discussions in slack, marking this for commit.

[pbiron]

@hellofromTonya

don't you need to add a call to the new _upgrade_580_force_deactivate_incompatible_plugins() function (e.g., on/about line 1390)?

[hellofromTonya]

Replying to pbiron:

@hellofromTonya

don't you need to add a call to the new _upgrade_580_force_deactivate_incompatible_plugins() function (e.g., on/about line 1390)?

@pbiron You know that moment when you realize "duh, I forgot to invoke it"? LOL You're right. Thanks for catching that. Updating now.

[hellofromTonya]

Invokes the upgrade function during update.

[hellofromTonya]

Patch 53432.1.diff​ adds and invokes the function that deactivates the Gutenberg plugin if it's version 10.7 or earlier.

I debated renaming the function to be more descriptive but left it generic in case there are other incompatible plugins.

[azaozz]

53432.1.diff looks good.

There's quite a bit of related discussion in Slack, see: ​https://wordpress.slack.com/archives/C02RQBWTW/p1623965403246200. Also related #53285.

Thinking it may be good to show a warning that the plugin has been disabled. Looking at how that may work, seems it would need to get triggered on the next run, so perhaps saving something in the DB and having a function to show it once. Can possibly add a bit to the Plugins list table to show a notice when a plugin has been auto-disabled, but perhaps consider it for 5.9 :)

For the future seems the best UX would be to interrupt the core upgrade when there is an incompatible plugin. Then tell the user (and/or send email) to either update or uninstall the plugin before upgrading core.

[SergeyBiryukov]

In 51180:

Upgrade/Install: Deactivate the Gutenberg plugin if its version is 10.7 or lower.

This avoids a fatal error due to WP_Block_Template class redeclaration when updating to WordPress 5.8 with an older version of Gutenberg activated.

Follow-up to [35582] for the REST API plugin.

Props hellofromTonya, oglekler, azaozz, desrosj, pbiron, jorbin, youknowriad, TimothyBlynJacobs, Clorith, markparnell.
See #53432.

[hellofromTonya]

Replying to azaozz:

Thinking it may be good to show a warning that the plugin has been disabled. Looking at how that may work, seems it would need to get triggered on the next run, so perhaps saving something in the DB and having a function to show it once. Can possibly add a bit to the Plugins list table to show a notice when a plugin has been auto-disabled, but perhaps consider it for 5.9 :)

For the future seems the best UX would be to interrupt the core upgrade when there is an incompatible plugin. Then tell the user (and/or send email) to either update or uninstall the plugin before upgrading core.

I agree it would be a better user experience than silently deactivating the plugin. How about if that work is moved to a new ticket?

[desrosj]

Replying to hellofromTonya:

I agree it would be a better user experience than silently deactivating the plugin. How about if that work is moved to a new ticket?

I think that a new ticket is beneficial for solving this longer term. However, I think we should solve this near term for this specific instance on this ticket.

Adding needs-patch so someone can take a pass at this.

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

[peterwilsoncc]

In the ​linked pull request:

• The warning displays once and once only to the first person who logs in and has the activate_plugins capability
• Introduces a new option wp_deactivated_plugins to store details about plugins that have been deactivated
• Introduces a new function wp_deactivated_plugins_notice which displays notices about a plugin that has been disabled during the upgrade
• Question: I've got an auto-loading option to avoid an additional db query on every admin screen but it adds a row to the options table that will be infrequently used. Is this the better approach or should WP just make the extra query?

To test the effect, run this wpcli command: wp option update wp_deactivated_plugins --format=json "{ \"gutenberg\": {\"plugin_name\": \"Gutenberg\", \"version_deactivated\": \"1.2\", \"version_compatible\": \"10.8\" }}"

[SergeyBiryukov]

Thanks for the PR!

Replying to peterwilsoncc:

• Introduces a new option wp_deactivated_plugins to store details about plugins that have been deactivated
• Introduces a new function wp_deactivated_plugins_notice which displays notices about a plugin that has been disabled during the upgrade

1. Could the function be named deactivated_plugins_notice() (without the wp_ prefix), for consistency with paused_plugins_notice() and paused_themes_notice()?
2. I think the option does not need the wp_ prefix either, as none of the other core options have it, with wp_page_for_privacy_policy being a single exception.
3. Should the option be added to populate_options() for new installs?

• Question: I've got an auto-loading option to avoid an additional db query on every admin screen but it adds a row to the options table that will be infrequently used. Is this the better approach or should WP just make the extra query?

If I'm reading correctly, the option would be empty most of the time? In that case, I think it should not have a significant effect on memory footprint, and autoloading is probably fine.

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

[peterwilsoncc]

Replying to SergeyBiryukov:

1. Could the function be named deactivated_plugins_notice() (without the wp_ prefix), for consistency with paused_plugins_notice() and paused_themes_notice()?

OK, I've marked the function private, should I prefix with an underscore to indicate that?

2. I think the option does not need the wp_ prefix either, as none of the other core options have it, with wp_page_for_privacy_policy being a single exception.

I'd rather prefix more for things in the table. The roles option uses the database prefix so I'd really prefer to keep it even though WP hasn't been great in the past.

3. Should the option be added to populate_options() for new installs?

Yes, I'll add that.

Replying to prbot:

​azaozz commented on ​PR #1431:

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

I'll test on a multisite install today both with the plugin site and network activated.

[peterwilsoncc]

For multisite installs that have Gutenberg enabled on individual sites (rather than network activated), _upgrade_580_force_deactivate_incompatible_plugins() will need to run as part of the database upgrade.

I've started work on fixing the notification PR I started last week to better account for multisite.

I'll try to get both of these fixes in the follow-up PR within a few hours time. It should be ready for review by around Monday 8am UTC.

[peterwilsoncc]

Replying to peterwilsoncc:

For multisite installs that have Gutenberg enabled on individual sites (rather than network activated), _upgrade_580_force_deactivate_incompatible_plugins() will need to run as part of the database upgrade.

Testing this proved a little ugly.

To determine the version of Gutenberg enabled during the database upgrade requires copying _upgrade_580_force_deactivate_incompatible_plugins() in to src/wp-admin/includes/upgrade.php and modifying it to get the plugin data via get_plugin_data( WP_PLUGIN_DIR . '/gutenberg/gutenberg.php' ) as plugins are not loaded for the DB upgrade screen.

The existing check during upgrade_core() would need to be retained as waiting until the database upgrade runs is too late as by then the plugin has triggered the fatal error.

As it's a little complex, I couldn't work out anything I was super happy with have put my test code in a gist for you to see my thinking ​https://gist.github.com/peterwilsoncc/689be020e5aa6561b0189cba59090c26 -- as you'll see with the missing conditions and triggering of logs, I didn't get beyond testing a few ideas.

---

I've pushed a few changes to the linked pull request for the notification side of things:

• renamed function to drop the wp_ prefix
• use a network wide option for deactivating network activated Gutenberg
• populating the option in the database upgrade routine

[desrosj]

I've added some feedback and questions on the PR.

Docs fix and renaming the option pushed in e693d51dddb503fbcc259c44d7e3a587f72ee97c and ba0d81488ce1069e8e2fe0661c252e1cc0c726ba respectively.

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

In 7fb3635bb679e9b81c95bfe54a6b09c6269471b7 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.

[peterwilsoncc]

I've addressed the feedback on the pull request.

I've also pushed some functional but not beautiful code to deactivate the plugin during the database upgrade but as I mention on the pull request, this may best be left for site owners to manage.

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.

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]

I've pushed some changes to the ​linked pull request, in it's final form:

• modifies function to disable the plugin in update_core() to account for network enabled plugins on multisite
• Displays a notification in the admin to indicate the plugin has been disabled
• Disabling within the DB upgrade still needs some work and a little more discussion on whether it's needed so I've left that out for now.

I think this is in a good form for commit, but given I wrote the code it would be good to get another committer to sign-off on the code.

[pbiron]

I haven't applied the patch and tested, but looks good here.

[peterwilsoncc]

In 51266:

Upgrade/Install: Notify users of deactivated plugins during upgrade.

This adds a one-off notice to the dashboard in the event WordPress has automatically deactivated a plugin due to incompatibility with the new version of WordPress.

Introduces the new private function deactivated_plugins_notice() to display the notice in the dashboard. Introduces the new auto-loaded option wp_force_deactivated_plugins to store a list of automatically deactivated plugins; the option is used on both a site and network level.

Follow up to [51180].

Props desrosj, jorbin, azaozz, SergeyBiryukov, peterwilsoncc.
See #53432.

Merged in https://core.trac.wordpress.org/changeset/51266

[desrosj]

Going to close this one out. If any issues are discovered it can be reopened. Any related work should be performed attached to new tickets.