Make WordPress Core

Opened 7 months ago

Closed 7 months ago

#59718 closed defect (bug) (wontfix)

Short-term (WP 6.4) hotfix to prevent fatal error in standalone Gutenberg (<16.5)

Reported by: rebasaurus's profile rebasaurus Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: reporter-feedback close has-patch
Focuses: Cc:

Description

WP 6.4 is not compatible with older versions of standalone Gutenberg plugins (< 16.5) per https://core.trac.wordpress.org/changeset/56820. The commit added automatic deactivation for plugins set by UI/DB, however, that doesn't satisfy enterprise use cases where:

  • WP Core files are updated through a version control system and then deployed. So, the actual update process isn't triggered in the usual way, bypassing the function intended to deactivate incompatible plugins.
  • Plugins can be force-enabled directly via code, making the standard deactivation process ineffective. This poses a risk, especially with the Gutenberg plugin, as sites might experience fatal errors after a core update.

The upcoming WP release mandates that users cannot run a Gutenberg version older ~2 mos. This abrupt timeframe is problematic for those building extensive integrations on top of Gutenberg.

Unlike previous instances where compatibility issues between Gutenberg and WP Core were resolved, this release seems to be establishing a stricter stance, which might hinder many from updating WP core.

Is it possible to fix the below fatal and/or add safeguards to it if an older version of standalone Gutenberg plugin is present?

PHP Fatal error: Access level to Gutenberg_REST_Global_Styles_Controller_6_2::validate_custom_css() must be protected (as in class WP_REST_Global_Styles_Controller) or weaker in /../public/wp-content/plugins/gutenberg/lib/compat/wordpress-6.2/class-gutenberg-rest-global-styles-controller-6-2.php on line 217

Attachments (1)

patch.patch (781 bytes) - added by rebasaurus 7 months ago.

Download all attachments as: .zip

Change History (26)

#1 @hellofromTonya
7 months ago

  • Milestone changed from Awaiting Review to 6.4

Hello @rebasaurus,

Welcome back to WordPress Core's Trac :)

This ticket is the next step in a discussion that started in Make/Core #6-4-release-leads channel (found it here) following feedback from WordPress VIP folks.

Context for plugin deactivation during Core upgrade

_upgrade_core_deactivate_incompatible_plugins() was added in 5.8.0 to safeguard users when they upgrade to a newer version of WordPress Core.

Why was it needed?
Some older versions of the Gutenberg plugin shipped without enough protection to avoid loading the same named class(es) and/or function(s) that were (possibly later) merged into Core. It aligned to Core files should load by default. By deactivating the known older versions of the plugin, it avoids a naming collision fatal error. In other words, it seeks to maintain users' trust that it's safe to upgrade to the newest version of WordPress.

Why is Gutenberg special? Why not enforce that it must fix its incompatibilities like all other plugins? Because Gutenberg is not just a plugin and its code is intended to land in Core. I think of as Core's R&D, an incubator for new ideas that, when ready for the masses, come to Core. Gutenberg and Core are intertwined.

What is the ask that was discussed in Make/Core slack?

Is there a short-term way (for 6.4) to possibly prevent the fatal errors for all ways WordPress Core itself gets upgraded? This includes those upgrade processes that do not use Core's update_core().

What was discussed in the slack thread and thus here in this ticket:

  1. Option 1: Could Core not load its class files and/or functions that will be incompatible with different versions of Gutenberg that have the same named classes and/or functions?
  1. Option 2: Is there a way to trigger _upgrade_core_deactivate_incompatible_plugins() as part of the upgrade processes not using Core's update_core()?
  1. Option 3: Could the Gutenberg team fix the incompatibilities in each older version and then ship .x release for each? Note: These upgrade processes will first upgrade the plugin before upgrading WordPress Core.

I'm bringing this ticket into 6.4 for discussion. That said, any changes in Core's source code must be minimal and with very high confidence given how late it is in the cycle.

Last edited 7 months ago by hellofromTonya (previous) (diff)

#2 @hellofromTonya
7 months ago

Sharing what I shared in the Make/Core slack channel about Option 1:

Could Core not load its class files and/or functions that will be incompatible with different versions of Gutenberg that have the same named classes and/or functions?

Core is loaded by default. It does not guard itself to protect against naming collisions outside of Core. Why? Its files are loaded into memory very very early in the boot up process before plugins and themes.

Not loading Core class files or functions if the Gutenberg plugin version is incompatible likely will require:

  • Waiting until later in the boot up cycle to load files that have naming conflicts with known older versions of Gutenberg.

This is risky. Why? Other code (such as within Core itself) may depend upon these classes/functions being loaded into memory. Identifying each of these is risky this late in the release cycle. This would be better done early in alpha IMO.

  • Need a list of which Gutenberg versions are incompatible for each PHP file or function.
  • Add guards to either not load the class file or function if incompatible with Gutenberg version.
  • Is Gutenberg activated? Is it an incompatible version? For Core to load these files/functions, it would need to look ahead (as waiting until Gutenberg is loaded might be too late):
    • Look ahead to to determine (a) is it activated and (b) get the version number from file's header.

#3 @hellofromTonya
7 months ago

Another consideration for Option 1:

Should Core's code version be considered the newest? Or should Gutenberg's code?

Changes also originate and get committed within Core's source code that may or may not be merged back into Gutenberg. This means the code in Core may be the newest version.

#4 @hellofromTonya
7 months ago

Option 3

Could the Gutenberg team fix the incompatibilities in each older version and then ship .x release for each? Note: These upgrade processes will first upgrade the plugin before upgrading WordPress Core.

Pinging the 6.4 Editor Tech/Triage Leads for their input on if this is possible @mikachan @siobhyb @karmatosed @annezazu @bph.

#5 follow-up: @jorbin
7 months ago

How is this different from any of the previous times? _upgrade_core_deactivate_incompatible_plugins has existed since 5.8

There have also been 4 major versions of Gutenberg released since 16.4 (the last incompatible version) so any site that is still running that should have upgraded by now.

#6 in reply to: ↑ 5 @hellofromTonya
7 months ago

Replying to jorbin:

How is this different from any of the previous times? _upgrade_core_deactivate_incompatible_plugins has existed since 5.8

Great question. Thanks @jorbin. My originally thinking too, especially as it relates to the current release.

I think the ask flows from this assertion:

Unlike previous instances where compatibility issues between Gutenberg and WP Core were resolved, this release seems to be establishing a stricter stance, which might hinder many from updating WP core.

But I'm not sure this is true. Were the naming conflicts resolved in the older released Gutenberg versions?

In past releases I was involved in, identification of the minimum Gutenberg version used the same process I used for 6.4: one-by-one, activate each past Gutenberg plugin version until the first throws fatal errors.

There have also been 4 major versions of Gutenberg released since 16.4 (the last incompatible version) so any site that is still running that should have upgraded by now.

@azaozz and I were recently discussing the following:

  • At what point do older versions of Gutenberg features become incompatible with the newest code shipped in a WordPress major release?
  • As Gutenberg plugin is rapid development and bleeding edge, shouldn't users keep upgrading to the newer versions? Assuming this is why older released versions of the plugin are not maintained in its repo.
Last edited 7 months ago by hellofromTonya (previous) (diff)

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


7 months ago

This ticket was mentioned in Slack in #core-upgrade-install by hellofromtonya. View the logs.


7 months ago

#9 follow-up: @jorbin
7 months ago

Looking at some previus usage of _upgrade_core_deactivate_incompatible_plugins and the timing between the most recent compatible Gutenberg Version and the WP release:

for 5.9, it was 75 days.
For 6.1.1 it was 60 days.
For 6.4 it will be 76 days (Assuming 6.4 remains on schedule)

#10 in reply to: ↑ 9 @hellofromTonya
7 months ago

  • Keywords reporter-feedback added

Thank you @jorbin for tracking down the timing for each of these releases:

Looking at some previus usage of _upgrade_core_deactivate_incompatible_plugins and the timing between the most recent compatible Gutenberg Version and the WP release:

for 5.9, it was 75 days.
For 6.1.1 it was 60 days.
For 6.4 it will be 76 days (Assuming 6.4 remains on schedule)

With this data, @rebasaurus @icaleb @annezazu why is 6.4 being flagged as more strict?

this release seems to be establishing a stricter stance

#11 @jorbin
7 months ago

  • Keywords close added

This was discussed in the #core-upgrade-install room amongst myself, @hellofromTonya, @pbiron, @costdev, @icaleb, and others. The consensus that we came to is that the protections in core feel sufficient for 6.4, but that the Gutenberg plugins should be enhanced to help future versions.

The Guteneberg Readme states:

Are you a tech-savvy early adopter who likes testing bleeding-edge and experimental features, and isn’t afraid to tinker with features that are still in active development? If so, this beta plugin gives you access to the latest Gutenberg features for block and full site editing, as well as a peek into what’s to come.

When a user opts into using bleeding edge features that are still in active development, they are accepting a certain risk. In the Gutenberg plugin, some additional checks can be added so that future versions of WP will have less of an issue around compatibility. GB is designed to be an experimental platform and the desire isn't to reign in that experimentation by forcing strict backwards compatible guidelines on it. A PR is being worked on that it is believed will help prevent fatal errors in future versions.

@icaleb mentioned doing some research to see if reverting [56575] should be considered to prevent the fatal error. Going to leave this open for that.

#12 @rebasaurus
7 months ago

Hi @jorbin, thanks for taking the time to discuss this. In my testing, https://core.trac.wordpress.org/changeset/56575 simply changing the property from protected to private seems to resolve the fatal. Is that something that can be considered for the next RC?

@rebasaurus
7 months ago

#13 @huzaifaalmesbah
7 months ago

  • Keywords has-patch added

#14 @jorbin
7 months ago

@rebasaurus The testing for that would need to make sure that no other versions of Gutenberg released since 16.5 have an issue due to the change. I believe this was the research Caleb mentioned doing. Is that the testing you did or was it just GB 16.4 and the patch vs trunk and/or 6.4? We also likely need to make sure that no public code is expecting the protected visibility. I've started a search of the plugins directory to help with that.

The change would also need to be made in Gutenberg and testing would need to be done to ensure trunk there has no issue.

I also think @ramonopoly, @isabel_brison, and @spacedmonkey might want to chime in as [56575] is something they worked on.

#15 @ramonopoly
7 months ago

Hi folks!

If I recall correctly, changing the access on the validate_custom_css method was done to make it consistent with other REST API classes, namely, so that any classes that extend them will have access to these methods internally.

Gutenberg itself has a minimum requirement of WordPress 6.2, which is why the Gutenberg_REST_Global_Styles_Controller_6_2 classes and other 6.2 compatibility-specific code was removed from the plugin.

WordPress 6.4 should contain all the non-experimental functionality of Gutenberg 16.8, so running a previous version such as 16.5, I believe, aside from the bespoke control workflows mentioned, has little pragmatic benefit.

It's a tough one: reverting validate_custom_css access back to "private" works yes, but in my mind it would be undesirable due to the extensibility issue mentioned above.

There are, however, understandable backwards compatibility concerns as this ticket explains.

If folks think reverting is acceptable, the main thing I'd be worried about is whether plugins etc are extending WP_REST_Global_Styles_Controller with the expectation of having access to this method. Given that the change was in 6.4 its seems that it's unlikely. A quick search confirms this: https://wpdirectory.net/search/01HDK32TW3W7NGTCBPMRZ2RVEY

The following PR, when merged, seems like it would help prevent such situations in the future (?)

https://github.com/WordPress/gutenberg/pull/35194

#16 follow-up: @hellofromTonya
7 months ago

Heads up:
The class WP_REST_Global_Styles_Controller_Gutenberg (in lib/class-wp-rest-global-styles-controller-gutenberg.php) is overloading and invoking the validate_custom_css() method

Reverting [56575] will impact not only those past releases, but also current and future development. Why? Reverting the method back to private means the method cannot be overloaded or invoked within Gutenberg.

#17 in reply to: ↑ 16 @hellofromTonya
7 months ago

Clarifying my comments:

Reverting [56575] will impact not only those past releases, but also current and future development. Why? Reverting the method back to private means the method cannot be overloaded or invoked within Gutenberg.

Reverting [56575] will not cause a fatal error on the past releases or currently in Gutenberg trunk.

Reverting this change needs broader consideration beyond this ticket as it can have impacts such as creating risks and additional development effort due to how PHP handles private access.

Let me explain:
Private methods are not overloadable and cannot be invoked outside of the class. See it in action https://3v4l.org/J5u29.

As a private method can only be run from within the class that has the method, care must be given to avoid fatal errors and invoking the wrong method.

From the PHP manual:

Private methods of a parent class are not accessible to a child class. As a result, child classes may reimplement a private method themselves without regard for normal inheritance rules.

A fatal error can happen if a child class invokes the private method, but does not have a copy of that method within its class.

  • The risk is minor in the wild as the visibility change has not shipped yet.
  • The risk can be managed in Gutenberg through testing and CI jobs.

Code may not behave as intended when invoking the parent's code that then invokes the private method (see it in action https://3v4l.org/TtXpK).

  • Extra burden: This means when extending a class, all of the code that invokes a private method(s) must be copied into the extended class, even if it's not customized.

Extra burden happens when merging code between Gutenberg and Core.

#18 follow-up: @hellofromTonya
7 months ago

IMO This ticket should be closed as wontfix.

tl;dr

  • The problems / reasons given do not directly relate to only 6.4.0.
  • This is not a 6.4.0 problem to solve this late in the release cycle.

Why not revert 56575?

I think the benefits of [56575] outweigh reverting it just to make 6.4 compatible with older Gutenberg versions.

The changeset is not a BC break within Core itself.

Changing it from private to protected helps Gutenberg development, as previously noted.

Yes, I do understand that a fatal error will happen in older Gutenberg versions without reverting this change. Fatal errors from older Gutenberg versions have happened in WP 5.8, 5.9, 6.1, and 6.4 (see the history of _upgrade_core_deactivate_incompatible_plugins()).

The assertions / reasons do not directly relate to only 6.4

I still don't understand these assertions / reasons for this ticket as to how they apply to only the 6.4 release:

The commit added automatic deactivation for plugins set by UI/DB, however, that doesn't satisfy enterprise use cases where:

This is not directly related to 6.4, as it's been this way since 5.8.0.

The upcoming WP release mandates that users cannot run a Gutenberg version older ~2 mos. This abrupt timeframe is problematic for those building extensive integrations on top of Gutenberg.

Why is it problematic?

Gutenberg is built on the premise of rapid experimentation and development. As @jorbin noted in comment:11:

The Guteneberg Readme states:

Are you a tech-savvy early adopter who likes testing bleeding-edge and experimental features, and isn’t afraid to tinker with features that are still in active development? If so, this beta plugin gives you access to the latest Gutenberg features for block and full site editing, as well as a peek into what’s to come.

When a user opts into using bleeding edge features that are still in active development, they are accepting a certain risk.

Given that Gutenberg iterates and ships quickly, shouldn't users of it be constantly upgrading the plugin?

My mind also goes to the feature and functionality compatibility between 6.4 (and any new version of WordPress) and older versions of Gutenberg.

Unlike previous instances where compatibility issues between Gutenberg and WP Core were resolved, this release seems to be establishing a stricter stance,

6.4 is not more strict than past releases:

for 5.9, it was 75 days.
For 6.1.1 it was 60 days.
For 6.4 it will be 76 days (Assuming 6.4 remains on schedule)

which might hinder many from updating WP core.

Why? Has this been a problem since WordPress 5.8.0, when this minimum version check was first introduced?

Future

Yes, it's worthwhile (and encouraged) to explore how to mitigate these issues between Gutenberg and Core versions.

If Core code changes are needed, they could happen in the next or future major release during the alpha cycle.

#19 @hellofromTonya
7 months ago

  • Severity changed from critical to normal

This ticket was mentioned in Slack in #core by jorbin. View the logs.


7 months ago

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


7 months ago

#22 @hellofromTonya
7 months ago

Been thinking more about this ...

I was wrong to say this:

The changeset is not a BC break within Core itself.

Changing a method's visibility is (strictly speaking) a BC break. That break will only impact instances where an extender has a child class with a copy of that method that has the original visibility, i.e. private.

Given this visibility change has not yet been released and is not in the Field Guide and does not have a dev note, the risk seems low, except for older released versions of the Gutenberg plugin.

For extensibility concerns, promoting the visibility of a method works without error.

  • The code since Gutenberg 16.5.0 will continue work (the code that uses the protected visibility).
  • Any code extending Gutenberg's class should also be okay.
  • Any child classes in the wild that is invoking the parent's method will get a fatal error. As previously noted, the searches do not show instances, but it is possible as extenders prepare for the new release.

I'm not currently aware of coming changes in PHP which may impact that, but will check.

Should the change be reverted?

So yes, the visibility change in r56575 could be reverted. But there are risks, including risks in comment:17.

Thinking out loud of the revert impacts:

  1. May help some hosts or service providers

who do upgrades without using update_core() functionality. But only helps their customers/users do not have incompatible previous versions, such as GB 16.0.0.

Thus, reverting does not completely resolve the concerns or efforts.

  1. Sets precedence that Core's BC promise also applies to past, non-maintained released versions of Gutenberg.

Does Core need to keep the BC promise for past, non-maintained released versions of the Gutenberg plugin? Huh, an interesting question.

Given GB's bleeding edge stance, I don't think it does.

If it does, then:

  • Gutenberg must also adopt Core's BC promise for code that is or will (some day) be in Core.

That would be a difficult to achieve when experimenting with new ideas and refining features that one day may go into Core. It would cause too much churn and technical debt.

  • Doing so would cause development problems in the Gutenberg cycles. For example, in a new class or method design, visibility is set to private until a need arises to open it.
  1. The minimum version set in [56820] would need to change.

Process: start with GB 16.4, test it, if okay, go back 1 older release version. Repeat until a fatal error happens. Then that's the minimum GB versions.

A quick check of file unzip and refresh frontend: GB 16.0.0 is the first version that causes a fatal error:

Fatal error: Cannot declare class WP_REST_Navigation_Fallback_Controller
  1. GB 16.8 is the version for WP 6.4

As @ramonopoly noted in comment:15:

WordPress 6.4 should contain all the non-experimental functionality of Gutenberg 16.8, so running a previous version such as 16.5, I believe, aside from the bespoke control workflows mentioned, has little pragmatic benefit.

As I noted in comment:18:

My mind also goes to the feature and functionality compatibility between 6.4 (and any new version of WordPress) and older versions of Gutenberg.

Summary

Reverting does not completely eliminate the concerns raised.

Reverting brings more concerns.

I'm still leaning towards not reverting and closing the ticket.

#23 in reply to: ↑ 18 @kirasong
7 months ago

Replying to hellofromTonya:

IMO This ticket should be closed as wontfix.

tl;dr

<snip>

Thanks so much for the synopsis. I agree with this assessment, and your follow up comment.

In particular (along with the notes that this has been the case since 5.8), I agree that it's important for it to remain safe for Gutenberg to be experimental, and that this includes being able to change class visibility as features evolve.

#24 @ramonopoly
7 months ago

Thanks so much for all the helpful information and time spent testing @hellofromTonya

#25 @jorbin
7 months ago

  • Milestone 6.4 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Based on the comments from both 6.4 core tech leads, the developer behind the original change, the original consensus reached in slack, and no new rationale presented in the contrary, I am going to close this ticket as wontfix. Discussion can continue so feel free to add new research or rationale.

Note: See TracTickets for help on using tickets.