Skip to content
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

Default to false when the "network" flag is omitted in the activate command #364

Merged
merged 2 commits into from
Jul 24, 2023

Conversation

raicem
Copy link
Contributor

@raicem raicem commented Jun 8, 2023

Fixes #348

For the activate command, we're defaulting the $network_wide option to NULL if the --network flag is omitted. This causes type errors further down in the code, as shown in issue #348.

I thought about doing the same for the deactivate command as well, but for deactivation, passing null is probably what we should keep since it has its own meaning in the WordPress' deactivate_plugins function.

@raicem raicem changed the title Default to false when the "network" flag is omitted Default to false when the "network" flag is omitted in the activate command Jun 8, 2023
@raicem raicem marked this pull request as ready for review June 8, 2023 11:18
@raicem raicem requested a review from a team as a code owner June 8, 2023 11:18
@danielbachhuber danielbachhuber added this to the 2.1.14 milestone Jun 8, 2023
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add feature tests covering this scenario for both wp plugin activate and wp plugin deactivate ?

@danielbachhuber
Copy link
Member

@raicem Planning to finish this one up?

@raicem
Copy link
Contributor Author

raicem commented Jul 13, 2023

Yes @danielbachhuber. I struggled to figure out tests. Can I get another week? Otherwise I'll close the PR.

@danielbachhuber
Copy link
Member

@raicem Yep, take your time. Feel free to stop by the #cli channel on WordPress.org Slack if you'd like help further.

@raicem raicem force-pushed the main branch 3 times, most recently from 62ea477 to e376a64 Compare July 23, 2023 16:38
@raicem
Copy link
Contributor Author

raicem commented Jul 23, 2023

@danielbachhuber Thank you for your patience with this 🙇 I managed to write a test for this regression now. I hope it makes sense!

While reported in the original issue #348, I can't replicate the type problem with wp plugin deactivate. When deactivating, WP CLI doesn't (and probably can't) cause a type error since the value it passes down to the deactivate_plugins function is not used later in the code.

This is not the case when activating a plugin. The $network_wide variable we pass to activate_plugin is directly passed down to the activate_{$plugin} hook and causes a type error. Therefore I only fixed and added a test for wp plugin activate.

Since we were not defaulting to a boolean value, the $network_wide
variable was null, when the `--network` flag was omitted.

This causes type errors when activated plugins run their activation
hooks and they expect a boolean value for the $network_wide variable,
as illustrated in issue wp-cli#348.
@swissspidy
Copy link
Member

While reported in the original issue #348, I can't replicate the type problem with wp plugin deactivate. When deactivating, WP CLI doesn't (and probably can't) cause a type error since the value it passes down to the deactivate_plugins function is not used later in the code.

This is not the case when activating a plugin. The $network_wide variable we pass to activate_plugin is directly passed down to the activate_{$plugin} hook and causes a type error. Therefore I only fixed and added a test for wp plugin activate.

Thanks for digging into this!

IIRC I originally ran into this with the activate command, but included deactivate in the bug report too so that we check whether it's affected as well. So thanks for confirming that the deactivate command is not affected!

@raicem
Copy link
Contributor Author

raicem commented Jul 24, 2023

Thank you @swissspidy for the explanation!

My tests failed, probably because the example code in my test doesn't play nicely with PHP 5.6. I'll check that and fix it later today.

Also added STDERR check for completenes, as suggested by @swissspidy.

Co-authored-by: Pascal Birchler <[email protected]>
Copy link
Member

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work on this, @raicem !

@danielbachhuber danielbachhuber merged commit 6a68d24 into wp-cli:main Jul 24, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

activate/deactivate command does not pass $network_wide as bool
3 participants