WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 5 months ago

#27188 new defect (bug)

deactivated_plugin behaves improperly

Reported by: wpsmith Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: Plugins Keywords: has-patch dev-feedback needs-refresh
Focuses: Cc:
PR Number:

Description (last modified by SergeyBiryukov)

Currently, if someone were to hook into deactivated_plugin, one should expect that the $plugin actually be deactivated.

So if, for example, I hook into it with the following code, deactivating Addthis, I don't get the expected behavior.

add_action( 'deactivated_plugin', 'dtat_deactivate_self', 10, 2 );
/**
 *  Deactivate ourself if Premise is deactivated.
 */
function dtat_deactivate_self( $plugin, $network_deactivating ) {
	if ( 'addthis/addthis_social_widget.php' == $plugin ) {
		die( 'Addthis: ' . print_r( is_plugin_active( $plugin ), 1 ) );
	}
}

The plugin still shows that it is active. So if I hook in here to check if plugin has been deactivated, then it fails. Instead, the deactivated_plugin hook should appear after the update_option call, which is where the plugin is actually deactivated.

OR, the docs are wrong and should be updated.

Attached is a sample addthis plugin extension that deactivates after Addthis is deactivated by being forced to use update_option_active_plugins and update_option_active_sitewide_plugins.

See Github Gist sample plugin for Addthis.

Attachments (4)

plugin.php.deactivated_plugin.docs-fix.patch (560 bytes) - added by wpsmith 6 years ago.
Option 2: Docs Update
plugin.php.move-deactivated_plugin.patch (1.6 KB) - added by wpsmith 6 years ago.
Option 1 (preferable): move action
27188.patch (3.0 KB) - added by ocean90 6 years ago.
27188.r1.patch (2.3 KB) - added by wpsmith 5 years ago.
Refreshed

Download all attachments as: .zip

Change History (16)

@wpsmith
6 years ago

Option 2: Docs Update

@wpsmith
6 years ago

Option 1 (preferable): move action

#1 @wpsmith
6 years ago

  • Focuses docs administration added

Related #27189.

#2 follow-up: @ocean90
6 years ago

  • Focuses docs administration removed
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.9

Option 1 seems ok. I would also move 'deactivate_' . $plugin and deactivate_plugin together.

#3 @ocean90
6 years ago

Related: #20241

#4 in reply to: ↑ 2 ; follow-up: @wpsmith
6 years ago

Replying to ocean90:

Option 1 seems ok. I would also move 'deactivate_' . $plugin and deactivate_plugin together.

I don't think they should be moved together. The tense of 'deactivate_' . $plugin suggests that it is in the middle of deactivating the current plugin or is not yet complete in the deactivation process. It is where the current plugin being deactivated hooks to run it's deactivating process, function, or method. The later, 'deactivated_plugin', suggests that the plugin has already been deactivated. To me, these are two different steps in the process, even thou a deactivated plugin is only an option in the WordPress database.

@ocean90
6 years ago

#5 in reply to: ↑ 4 ; follow-up: @ocean90
6 years ago

Replying to wpsmith:
I'm talking about deactivate_plugin without ed, see 27188.patch.

#6 follow-up: @SergeyBiryukov
6 years ago

  • Description modified (diff)
  • Summary changed from deactived_plugin behaves improperly to deactivated_plugin behaves improperly
  • Version changed from trunk to 2.9

#7 in reply to: ↑ 6 @wpsmith
6 years ago

Replying to SergeyBiryukov:
This is still slated for 3.9. Any movement on this since we are now in Beta?

#8 @nacin
6 years ago

  • Milestone changed from 3.9 to Future Release

This function works in a loop. The option is not actually updated until the end, while each plugin gets its hooks fired in turn. The patches attached won't work as they move deactivated_plugin to after the update_option() calls — from inside the loop to outside of it.

I agree, conceptually, that the option should be updated *before* deactivated_plugin. To do that, we'd need to call update_option() inside the loop, which means more database calls (at least when we're bulk deactivating). I don't see that as a terrible thing, but it's not something I'd like to just do (especially since we hadn't caught it before now that this change would be a problem).

activate_plugin() does *not* work in a loop. activate_plugins() calls activate_plugin() which handles both the hooks and the option calls. activated_plugin properly fires at the right time.

I don't really care if we introduce a deactivate_plugin() function to mimic activate_plugin(), but I'm otherwise fine with these changes if done properly.

In the process, I noticed some docs were wrong, so I'm fixing those as well.

#9 @nacin
6 years ago

In 27769:

Fix docs for hooks in deactivate_plugins() and activate_plugin(). see #27189, #27188.

#10 in reply to: ↑ 5 @wpsmith
5 years ago

  • Keywords dev-feedback added

Replying to ocean90:

Replying to wpsmith:
I'm talking about deactivate_plugin without ed, see 27188.patch.

I think this is the correct way and expected way. I am attaching a refreshed updated version.

This ticket was mentioned in Slack in #core by wpsmith. View the logs.


5 years ago

@wpsmith
5 years ago

Refreshed

#12 @chriscct7
4 years ago

  • Keywords needs-refresh added
Note: See TracTickets for help on using tickets.