-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Plugin: Add requirement for minimum and maximum WordPress versions #35194
base: trunk
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed some minor typos
gutenberg.php
Outdated
gutenberg_pre_init(); | ||
// Minimum supported version. Has to match "Requires at least" header above. | ||
if ( ! defined( 'GUTENBERG_MIN_WP_VERSION' ) ) { | ||
define( 'GUTENBERG_MIN_WP_VERSION', '5.7' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could auto-generate this constant. This would be similar to how GUTENBERG_VERSION
gets injected when building the plugin zip:
gutenberg/bin/generate-gutenberg-php.php
Lines 35 to 40 in d55093a
if ( | |
! $plugin_version && | |
preg_match( '@^\s*\*\s*Version:\s*([0-9.]+)@', $line, $matches ) | |
) { | |
$plugin_version = $matches[1]; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, has to match the Requires at least
from the plugin's headers (top of this file). No need to type it twice :)
gutenberg.php
Outdated
// Also note that `version_compare()` considers 1 < 1.0 < 1.0.0 (before suffixes) | ||
// so 6.0 < 6.0.0-alpha and 6.0-beta < 6.0.0-alpha are both true. For that reason | ||
// the constant should be set to major (two digits) WordPress version. | ||
define( 'GUTENBERG_NEXT_UNSUPPORTED_WP_VERSION', '6.0' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be risky, but maybe we could also auto-generate this constant from Tested up to
:
Line 3 in 1d0f3c4
Tested up to: 6.3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, was thinking the same. Generally the Tested up to
is set to the currently released WP version, will need to increment it.
Was wondering, what would be a good rule-of-thumb max WP version? As far as I see fixes and new features that get added to Gutenberg while WP is in alpha are generally synced (merged) with core trunk before WP is released, i.e. in the same WP version. The question is: should it stop loading Gutenberg if WP is newer than the Tested up to
+1, or should it just show a warning to update Gutenberg in Tested up to
+2, and stop loading in Tested up to
+3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be risky, but maybe...
Looking a bit more, actually not sure this would work well. The problem is that if Gutenberg is released during WP alpha or (early?) beta, this should be set to Tested up to
+1. However if Gutenberg is released during WP RC, this should probably be set to Tested up to
+2 as any new features or bugfixes will most likely not be added to the WP RC branch (branching of WP trunk usually happens shortly after RC1). So seems this would have to be set manually. Of course there will be a lengthily explanation in inline comments about how to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it's very difficult to reason how that could work. I need to think a bit more about all the relations between the Gutenberg versions backported to the given major version of WordPress core, Tested up to
, the release process, etc. The goal from my perspective would be to automate as much as possible so people don't have to go through the same painful process again reasoning about all the implications 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
people don't have to go through the same painful process again reasoning about all the implications
Yea, automating the decision would be best. Not sure if that would be possible though, seems too many variables and possible exceptions. The next best thing would be to add as much help and explanations as possible. Thinking it would be great to have "couple pages long" docblock there, in any case.
It would be a very useful addition for sites using the Gutenberg plugin. In particular, it would help avoid the situation when the plugin downgrades the version of the editor that is shipped in WordPress core. |
Flaky tests detected in ddc4ce3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6974358369
|
gutenberg.php
Outdated
/** | ||
* Add a "Gutenberg version is too old" plugins list table notice. | ||
* | ||
* @since 11.7.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR has been open for quite some time. These number will need to be updated before landing 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, still going through it at the moment. Should have a better/cleaned up version by tonight.
Related trac ticket: https://core.trac.wordpress.org/ticket/59720 |
gutenberg.php
Outdated
} elseif ( | ||
version_compare( $wp_version, $max_supported_version, '<' ) && | ||
version_compare( $wp_version, $max_supported_version . '-alpha', '>=' ) | ||
) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To test this I set the WP max version to 6.3 and I have 6.4 (beta or RC) installed locally, and I expected the plugin to disable entirely, but for now I just got that it will be disabled when I upgrade. To be honest, I think that's a bit confusing and I'd just disable Gutenberg in this case as well. I'm not sure we need the two levels of feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and made this change. So now:
- The plugin is also disabled during the beta/RC period. The reasoning is that the Gutenberg features and new code have already been back ported to Core at that time. And you need to update Gutenberg to the next version in order to include the features that were not back ported (targeted for the next WP version).
- It also means that the max version should be updated right before/after beta 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It also means that the max version should be updated right before/after beta 1
Yep, as soon as the version with the latest fixes and features for the current WP development version is released (the latest Gutenberg version scheduled to be synced with core trunk).
As far as I see further fixes to merged features are done in minor (dot) releases. There GUTENBERG_MAX_WP_VERSION
would be same as in the (older) major release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
The display is a bit odd since it reads like the plugin is active while it's not loaded. Not a blocker, but we could add something like |
I tried looking here, I'm not sure there's any filter to that allows us to alter the "name" cell in the plugin table. |
Yea the whole concept of having to install plugins and then activate them is a bit ... different that most other places. Showing "(not loaded)" right next to "Deactivate" sounds good imho. Wondering if "(disabled)" would be even clearer? Alternatively can probably replace the "Deactivate" with "Uninstall", although technically the plugin is still active. Afaik there's no problem uninstalling active plugins (all "must use" plugins are like that). Will do a bit of testing. |
Looks like there's some failing e2e tests preventing the merge of this PR. |
Not sure if they are related, or rather cannot figure out what's actually failing. Seems the only possible thing that might fail is when the WP version during the tests is either missing or incorrect/not supported. Cannot see where that is set though. |
- Add couple more warnings when Gutenberg is too old or too new for the WP installation. - Add warnings in the plugins list table row.
Co-authored-by: Nikhil <Nikschavan@users.noreply.github.com>
Co-authored-by: Nikhil <Nikschavan@users.noreply.github.com>
Co-authored-by: Nikhil <Nikschavan@users.noreply.github.com>
Rename GUTENBERG_NEXT_UNSUPPORTED_WP_VERSION to GUTENBERG_MAX_WP_VERSION, etc.
Also tweak the warnings messages a little and simplify the version compare for newer WP versions.
update `@since` to 17.0.0.
27f1cdf
to
ddc4ce3
Compare
Check if e2e tests are failing because of using too new WP version.
Rebased in an attempt to fix the e2e tests. The timeouts were fixed however "No node found for selector" popped up. |
Seems the e2e tests fail because they are run in an alpha (core trunk) build of WordPress. Tested by increasing GUTENBERG_MAX_WP_VERSION to allow the latest dev WP version, and the e2e tests passed. Just to confirm: the intention here is to auto-disable (not load) the Gutenberg plugin when it is run in a version of WordPress newer than the "Tested up to" version from the plugin's readme, right? In that case why are the e2e tests being run in a newer version of WP, the current development version? Seems they should be run in the currently released WP version? |
Good question 😄 |
Also update some docs.
Yea, that's "the other side of the coin". Generally Gutenberg should not be loaded when it will be "downgrading" code that has already been added to core. This may happen more often in the future as "the push" is to move/sync code from Gutenberg to core as soon as possible, while WP is still in alpha.
That may not be necessary any more as the current Gutenberg version will not be loaded in the next (released) WP version. Using the GUTENBERG_MAX_WP_VERSION will prevent it. In any case, changed it back to still load the plugin in the next WP version while it is in development. For example: If GUTENBERG_MAX_WP_VERSION is 6.4, the plugin will load in 6.5 alpha, beta, and RC, but not in the released 6.5.0. That will prevent fatals and other errors in production while still allowing more flexibility for testing. |
Yeah this is an interesting problem 😄 but given the speed of development in Gutenberg vs core there would only actually be a downgrade if an older version of the plugin were being used. The e2e assume that Gutenberg trunk will always be ahead of core, which I think is correct: trunk should never be behind core. But we also run e2e on release branches when preparing patches for older WP versions, so whatever we go with here should also work in that scenario. I think in the release branches it would make sense to run the tests on whatever WP version they're targeted at. |
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @WPprodigy. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
gutenberg_pre_init(); | ||
// Minimum supported version. Has to match "Requires at least" header above. | ||
if ( ! defined( 'GUTENBERG_MIN_WP_VERSION' ) ) { | ||
define( 'GUTENBERG_MIN_WP_VERSION', '6.2' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the min version always known for each Gutenberg release?
What if the plan changes and thus that version might be different (maybe older such as a previous minor or later)? Later isn't necessarily problematic as the remedy is to upgrade. Older is problematic in that a fatal error might happen.
Wondering if instead, this minimum version should be set by Core and consumed in the plugin. For example, Core could provide a global function such as _get_plugin_wp_min_compatible_version()
where for-core plugins like Gutenberg could invoke. Let Core determine which version is the min compatible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, sorry but I think I'm missing something? Does that mean we're getting rid of the "Requires at least" plugin header?
What if the plan changes and thus that version might be different
That means a plugin will not "know" what is the minimum WP version it supports at release, and want to change/fix it later? This hasn't been a problem... forever, not sure it is a problem now?
Wondering if instead, this minimum version should be set by Core...
...
Let Core determine which version is the min compatible.
Not sure how core would "know" if a plugin is fully compatible or not. Emphasis on "fully compatible" here as some incompatibilities are not obvious or easy to detect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, this isn't correct. Sorry. It's a minimum version in Core, but a max version in the plugin.
Not sure how core would "know" if a plugin is fully compatible or not. Emphasis on "fully compatible" here as some incompatibilities are not obvious or easy to detect.
Historically within Core itself, it views the minimum compatible version of the GB plugin to be the first version that causes a fatal error. That's very different from defining "fully compatible". It's focused on preventing fatal errors for users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically the versions that are compatible with the WordPress version in development aren't known until close to WP Beta 1.
Just before Beta 1 or during beta is when the harded code GB version is updated Core (currently within _upgrade_core_deactivate_incompatible_plugins()
).
What's my point?
The plugin's actual maximum WordPress version might not be known at the plugin release time. It might change as the WordPress dev cycle advances into beta.
For example, a fatal error happened recently which then caused an update in Core to set the 17.6 as the minimum compat version for WP 6.5. I'm wondering if 17.5.x or older would have know it's max WP version is WP 6.4, and not 6.5?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How can released GB plugin versions know if Core itself will deactivate it due to be incompatible?
That's where the plugin could use the proposed Core function _get_plugin_wp_min_compatible_version()
.
Using the previous example, that Core function would return 17.6
. So then released plugin versions could invoke the Core function and take action itself. Then in time once all released versions of the plugin have this new code, Core would no longer need to deactivate the plugin, i.e. because the plugin handles it itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, this isn't correct. Sorry. It's a minimum version in Core, but a max version in the plugin.
Ahh I see. I knew I'm missing something :)
How can released GB plugin versions know if Core itself will deactivate it due to be incompatible?
The same way it "knows" it is not going to be installed on a server with a PHP version lower than the minimum required, and in a "too old" WP? I.e. the plugin sets its own requirements, and WP makes sure these requirements are met. Same as PHP and WP versions, and now plugin dependencies.
Similar logic has existed in core since 2007 (WP 2.0) I think: plugins can specify minimal WP version. This is opt-in and enforced by the plugins repo API and by the core plugins API.
Adding another optional requirement for a maximum supported WP version would be best to follow the same logic/design imho, and should be available for all plugins. I know of 2-3 other plugins that can use this right now, for example the Test jQuery Updates plugin that will probably be used again for jQuery 4.0.
That's where the plugin could use the proposed Core function _get_plugin_wp_min_compatible_version().
Hmm, don't think WP (core) would be able to "know better" which plugin is compatible with which version. This info should be in the plugin, and is set to a constant in this PR. So not sure why a plugin will have to use a WP function to get the info it sets itself? Think it is the opposite: WP should "read" what the plugin requirements are and check if they are satisfied. If not satisfied WP should not load the plugin and should warn the users/site admins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugin's actual maximum WordPress version might not be known at the plugin release time. It might change as the WordPress dev cycle advances into beta.
Right. For that reason the "maximum supported" WP version can only be a major (two digits) version and should (generally) match the "tested up to" plugin header.
Minor version of Gutenberg will still support the same "max WP" version. In addition the "max WP" version will include support for the next "WP Trunk", (alpha, beta, RC). This is needed in order to be able to use the current Gutenberg releases in a WP Trunk installs, like on wp.org. Please see the discussion above.
Just FYI, the reason this PR is not yet merged is that the logic (when to not load newer Gutenberg in older WP) is good, but seems the code would be better off in core, as part of the "PHP fatals prevention" there. We've been thinking/working on that with @hellofromtonya for a while and there will most likely be an initial version of this in WP 6.5. |
I really do think it would be good to have the loader-prevention inside this plugin as well though. It's the only place we can fully prevent a bad outcome. At the end of the day, plugins can be loaded via a IMO, the main goal here should be to make WP upgrades as safe as possible. And at the moment, having the Gutenberg plugin on a site poses a very big threat as fatals after a WP upgrade are fairly common (due to class name re-use for example). I haven't closely followed the other work being done in core, but I can't see this fully be solved without the GB plugin preventing itself from loading if it's unsafe to do so. And as far as development lifecycle goes, the only truly required information (as far as I can reason) would be for a core release to determine it's minimum supported version of GB. Because that's when the breakage is occurring. The sooner we get the loader-prevention code into the GB plugin the better as well, as we won't get the protections right away as not everybody will be using the latest version of the plugin. So starting that timer as soon as possible would be nice. |
Well, there's nothing stopping us from adding it in the plugin too, however that would duplicate core's functionality and might eventually interfere with it. The idea here is to add another type of "plugin requirement": maximum WP version. This will be the forth requirement type, the others are: minimum WP version, minimum PHP version, other plugins (as of 6.5). That way WP will be able to handle these cases in a consistent way which will be easier for the users to follow/understand, and the incompatibility messages/warnings would be a bit better. Also other plugins would be able to set the same requirement.
For now the idea is for WP to be more in control when loading plugins. It will need to have the "plugin meta" (the plugin's headers) either easily readable at plugin loading time or caches (in a DB option/persistent cache). Then the plugin will have an "init" function that WP will call if it is safe to initialize the plugin. This way WP will be able to show all appropriate messages/warnings about incompatible plugins, and prevent loading them when they are updated from FTP or git/svn. Also the warnings can be shown regardless if a plugin is activated or not. See: https://core.trac.wordpress.org/ticket/60491#comment:13 |
Description
Limits loading of Gutenberg when the WordPress version is too old or too new. This will prevent errors and incompatibilities in the future. When the plugin is not loaded it asks the user to either turn plugin auto-updates on, update the plugin, or upgrade WordPress (if they can).
When running a newer development version of WP Gutenberg is loaded and adds a warning that it needs updating and will stop working after WordPress is upgraded to the next release version.
GUTENBERG_MIN_WP_VERSION
andGUTENBERG_MAX_WP_VERSION
constants.To test: override
$wp_version
inwp-includes/version.php
(better) or change the requirements ingutenberg_pre_init()
.(The error/warning strings can probably be enhanced. Edit as you see fit.)
Checklist:
*.native.js
files for terms that need renaming or removal).