-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Add handling for plugin-feature incompatibilities #34879
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
Add handling for plugin-feature incompatibilities #34879
Conversation
The methods that return compatibility info now have an extra 'uncertain' part with information regarding plugins that are WooCommerce-aware but haven't declared compatibility.
Test Results SummaryCommit SHA: bd86907
Please address the following issues prior to merging this pull request: |
Includes a link to the list of incompatible plugins.
Plugins that are incompatible with at least one enabled feature will show a warning in the WooCommerce plugins page, and will have their "Activate" link disabled.
3b2fcd0 to
faad9ee
Compare
"All" and "Incompatible with feature X" - Exclude the legacy Analytics features from the feature and plugins activation restrictions - Allow disabling a feature from the settings pages if it's enabled and is incompatible with at least one plugin (it won't be possible to re-enable it once the settings page reloads)
(which uses \ instead of / as directory separator)
- Fix: the incompatible plugins count in the feature settings page now only counts active plugins.
84d12de to
5ed89cf
Compare
…les" - Add an extra parameter to FeaturesController::get_compatible_plugins_for_feature to retrieve all matching plugins or only active plugins.
jorgeatorres
left a comment
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.
Hey @Konamiman! 👋
Great job here 🥇. I left some comments but overall this works and looks great.
My main concern (with the code) has to do with the renaming of custom_order_tables to high_performance_order_storage. I added some notes about this directly as part of the review.
There is another thing we feel should be discussed, which didn't really fit into the code review per se:
- Even though there's support for allowing enabling of features/plugins with incompatible features, there's really no UI for this at the moment. I might've missed whether this is planned or not, but it'd be very useful.
- "Uncertain" compatibility shouldn't prevent activation of plugins, or at least not if the uncertain compatibility is with a legacy feature. For example, "Analytics" is enabled by default but then because all plugins have "uncertain" compatibility with it, it prevents them from being activated. This effectively means no one can activate current version of WC plugins even if they aren't using HPOS or any other experimental feature.
Please let me know what you think.
plugins/woocommerce/src/Internal/Features/FeaturesController.php
Outdated
Show resolved
Hide resolved
| * @return string Printable plugin name, or the plugin id itself if printable name is not available. | ||
| */ | ||
| public function get_plugin_name( string $plugin_id ): string { | ||
| $plugin_data = $this->proxy->call_function( 'get_plugin_data', WP_PLUGIN_DIR . DIRECTORY_SEPARATOR . $plugin_id ); |
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.
Would it make sense to also cache the result of this function call? get_plugin_data() doesn't seem to cache things on its own, so this might result in unnecessary I/O if the function is called a few times during a request.
| ); | ||
| } else { | ||
| /* translators: %1\$s, %2\$s = printable plugin names, %3\$d = plugins count */ | ||
| $message = sprintf( |
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.
Also needs to be passed through _n().
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 is probably not required, the messaging seems correct for both 1 or more cases.
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.
That's true in English, but we don't necessarily know how pluralization works for other languages. This seems like a minor inconvenience though, so I'm not holding off merging for that.
|
@Konamiman We discussed this with @beaulebens now and since HEs are not available during the rollout of 7.1 and if I understand correctly this paragraph
...it means that the default plugins list will show the warnings. Can we disable those until later please (maybe we'll put them back in 7.2 or when a higher % of plugins are compatible)? So that we'll still have the extra view, but the default view won't be flooded by the notices for everyone? |
Addressed in 5a3e897 |
jorgeatorres
left a comment
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.Thanks @Konamiman & @vedanshujain!
|
Hi @jorgeatorres, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:
|
All Submissions:
Changes proposed in this Pull Request:
After the features engine is completed (#34727) it's time to actually do something with the gathered compatibility info. This pull request implements UI changes in two places:
The feature settings page
If there are one or more active plugins that haven't declared positive compatibility with a given feature, a warning notice will be displayed below the checkbox for the feature. Additionally, if the feature is disabled, the checkbox will be disabled too:
The legacy features ("Analytics" and "New navigation") are excluded from this check and don't display any warning: these features have been around for a long time and it doesn't make much sense to start bugging plugin developers about declaring compatibility at this point.
The plugins page
The "Manage incompatible plugins" link in the feature notice targets a special view of the plugins page, with
?plugin_status=incompatible_with_feature&feature_id=<feature_id>:This new view includes all the plugins that haven't declared positive compatibility with the given feature, regardless of whether the plugin is currently active or not. The link to the view will be visible only if
plugin_status=incompatible_with_featureis present in the URL.We are only showing the link to the
Allview together with our new view, because the counters would be wrong anyway (we are filtering the list returned by theall_pluginshook); the counter forAllis explicitly recalculated here.Regardless of the current view, for plugins that haven't declared positive compatibility with at least one of the currently enabled features (again, excluding "Analytics" and "New navigation"), a warning will be shown:
Additionally, the Activate link for the plugin will be disabled.
Enabling/activating via code
It's still possible to activate plugins and enable features via code (
wpcli tool) and that's on purpose: at least initially there must be a mechanism to bypass the UI constraints for the cases where the admins know what they are doing.Additionally, the
FeatureUtilclass gets two new methods that disable the UI restrictions. So the admin can add this code to their site:Then the warning notices will still be displayed, but it will be possible to activate the plugins/enable the features from the respective UI pages.
Closes #34861.
How to test the changes in this Pull Request:
You may want to add or remove features during your testing. To do so, just manually modify the
$featuresarray that is initialized at tthe beginning of the constructor in theFeaturesControllerclass.Testting UI
You need to test that the features page (WooCommerce - Settings - Advanced - Features) and the plugins page work as expected by trying various combinations of enabling/disabling features and activating/deactivation plugins. Thinks to look for:
Testing code
Add the snippet indicated above (in "Enabling/activating via code") and verify that the warnings are still displayed, but features can always be enabled and plugins can always be activated, even with incompatibilities.
Verify that you can still enable and disable features via code, even features with incompatibilities, and even without the above snippet, e.g:
wp option set woocommerce_feature_high_performance_order_storage_enabled yesVerify that you can still activate and deactivate plugins, even plugins with incompatibilities, and even without the above snippet, e.g:
wp plugin activate woocommerce-bookingsOther information:
pnpm --filter=<project> run changelog add?FOR PR REVIEWER ONLY: