wp-cli / extension-command

Manages plugins and themes, including installs, activations, and updates.
MIT License
87 stars 82 forks source link

Activation hooks are not run with --activate --force and already activated plugins #390

Open mrsdizzie opened 6 months ago

mrsdizzie commented 6 months ago

Feature Request

When running wp plugin install plugin --activate --force

It won't run activation hooks if the plugin is already activated: https://github.com/wp-cli/extension-command/blob/12618c730ce9aec3e3c1a4db7e13e57a42ec6e53/src/Plugin_Command.php#L352-L356

It would be nice if --force also triggered the activation hooks, because it is possible for a plugin to be activated but in a weird state where the activation hooks can fix it.

As an example:

WP Super Cache installs an advanced-cache.php dropin when activated. If something deletes that file, the plugin can be activated but now not work properly. Running wp plugin install wp-super-cache --activate --force will output:

Installing WP Super Cache (1.11.0)
Downloading installation package from https://downloads.wordpress.org/plugin/wp-super-cache.1.11.0.zip...
Unpacking the package...
Installing the plugin...
Removing the old version of the plugin...
Plugin updated successfully.

Which makes it seem like a full replacement, but it won't recreate advanced-cache.php because it doesn't actually run the activation hooks anywhere above. If you deactivate the plugin first and then run the command, it will run the activation hooks and recreate it. But then you have any side effects that might come from deactivating it. I think it would make sense if the existing --force option also bypassed the check above and cause it to run the activation hooks again as part of Installing the plugin...

danielbachhuber commented 6 months ago

I'm open to this, but a little uncertain about it. The change in behavior might be unexpected for existing usage.

Should we also add a --force flag for wp plugin activate ?

@schlessera @swissspidy Thoughts?

mrsdizzie commented 6 months ago

Looking down a bit, the --activate flag does already cause an initial deactivation if switching an active plugin to network mode, so there is somewhat mixed behavior in the current implementation.

https://github.com/wp-cli/extension-command/blob/12618c730ce9aec3e3c1a4db7e13e57a42ec6e53/src/Plugin_Command.php#L352-L361

But you are right, the actual --force behavior should belong to wp plugin activate and it should just be triggered when wp plugin install is used with both --activate --force

swissspidy commented 6 months ago

I'm open to this, but a little uncertain about it. The change in behavior might be unexpected for existing usage.

Same here. I wonder what could break here, as it would mean running the activation hook for a plugin that's already active. Plus, would we need to do the same sandboxing as activate_plugin() does?

Should rather be the job of plugins like WP Super Cache to detect whether they are correctly set up and provide guidance to users on how to fix issues?

mrsdizzie commented 6 months ago

It isn't really about needing a solution to a particular plugin issue, it is more about the expected behavior of a "force install and activate" command. With this output:

Installing the plugin...

I would expect it to behave the same as if installing a new plugin. Specifically, I'd expect --force to ignore all types of "does this already exist and should we skip it" checks, but it only ignores some (replacing the existing files).

Maybe it is just confusing having the two flags --force and --activate but having them not be related to each other at all. If wp plugin activate had a --force flag you would assume it caused activation hooks to be run always, but it is more ambiguous here what --force will do in this example.

In any case, deactivating the plugin first will cause the hooks to be run again and I'll use that as a work around -- so this isn't a huge problem without a solution. When switching network states, this command already deactivates and the reactivates the plugin, so I assume something similar would happen even if this was implemented.

swissspidy commented 6 months ago

I definitely see how that's ambiguous, especially with the wording that doesn't indicate that it's installing an already active plugin. Perhaps we could at least add some info message or something?

Just wary of actually running the hook as it could potentially cause issues

Related: https://core.trac.wordpress.org/ticket/52281