-
Notifications
You must be signed in to change notification settings - Fork 797
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
Blocks: convert to module #39449
base: trunk
Are you sure you want to change the base?
Blocks: convert to module #39449
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
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.
Should we add an upgrade routine to the plugin_upgrade
method in the big Jetpack class, and 1) turn the module on unless the option was set to false 2) delete the function?
This would ensure that blocks continue to work on sites that were using them, regardless of module auto-activation settings that may have been overwritten with the jetpack_get_default_modules
filter.
With
|
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 default state for this option is to not exist, so we want to activate it only one if the option is false and delete the option.
Do we really need to delete the option right away? What if we leave it existing as long as it's true, only deleting it if the module someday gets activated?
In that case we might be able to use the 'jetpack_get_default_modules' filter ourself to exclude the module from the list for as long as that option is set, instead of putting a special case inside of activate_default_modules()
.
$return = true; | ||
|
||
if ( ! ( new Modules() )->is_active( 'blocks' ) ) { | ||
$return = false; |
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 note this is a behavior change, before the 'jetpack_gutenberg' was only able to disable the feature, not enable it. Now it can do both.
If it's intentional, it might be worthwhile updating the filter docs below.
Fixes #39351
Fixes #39441
Supercedes and closes #39422
Proposed changes:
This will reduce the db calls, since this option is typically not set until someone has toggled it the first time, and convert it to a typical entity.
Other information:
Jetpack product discussion
n/a
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
A few different things to test. Each should be tested on a clean site (e.g. Nothing Jetpack on it; either new sites or uninstall Jetpack / delete jetpack_options, etc.)
Default new install state (auto-activation of the blocks module)
Existing site (auto-activation of a newly defined module)
Existing site with the option previously set true (confirms the module does not activate to respect previous setting and the old option has been deleted to clean up after ourselves.
wp option set jetpack_blocks_disabled true
) or visit Jetpack->Settings->Writing and toggle the block option.wp option get jetpack_blocks_disabled
should returnError: Could not get 'jetpack_blocks_disabled' option. Does it exist?
Existing site with the option previously set false. (if someone had previously disabled and re-enabled; verifies that the modules are active to match previous setting and option has been deleted to clean up)
wp option set jetpack_blocks_disabled false
) or visit Jetpack->Settings->Writing and toggle the block option to disable and then re-enable the blocks.wp option get jetpack_blocks_disabled
should returnError: Could not get 'jetpack_blocks_disabled' option. Does it exist?