Make WordPress Core

Opened 11 years ago

Closed 9 years ago

#26904 closed enhancement (fixed)

General 'delete_plugins' hook needed

Reported by: veraxus's profile Veraxus Owned by: drewapicture's profile DrewAPicture
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.8
Component: Plugins Keywords: has-patch needs-testing 2nd-opinion
Focuses: Cc:

Description

I'm working on an activity logging plugin and managed to use the 'upgrader_process_complete' hook to detect when plugins are installed, but while digging around core for a way to detect when plugins are deleted, I realized that there was no existing, suitable hook.

Now, there is an 'uninstall_'.$file hook - but that only allows targeting of a specific plugin... while I need to detect and identify any and all plugin deletions.

I'm proposing the addition of the following hook in /wp-admin/includes/plugin.php, immediately after the 'uninstall_'.$file hook in uninstall_plugin()...

do_action( 'uninstall_plugin', $plugin );

Attachments (7)

uninstall_plugin_hook.diff (479 bytes) - added by Veraxus 11 years ago.
Patch that adds the 'uninstall_plugin' hook
delete_plugin_hook.diff (469 bytes) - added by Veraxus 11 years ago.
'delete_plugins' hook - A much better, more comprehensive hook. Tested and working.
uninstall_plugin_hooks.diff (558 bytes) - added by Veraxus 11 years ago.
Adds the 'uninstall_plugin' hook to both uninstall_plugin() handlers
delete_plugin_hook_FINAL.diff (718 bytes) - added by Veraxus 11 years ago.
Option 3: Formal recommendation - 'delete_plugin' hook within delete_plugins() deletion loop
26904.diff (668 bytes) - added by DrewAPicture 10 years ago.
+ hook doc
26904.2.diff (1.2 KB) - added by johnjamesjacoby 9 years ago.
Introduce deleted_plugin hook, and move delete_plugin to before
26904.3.diff (1.1 KB) - added by johnjamesjacoby 9 years ago.
Same as 2.patch, but with corrected filter documentation

Download all attachments as: .zip

Change History (25)

@Veraxus
11 years ago

Patch that adds the 'uninstall_plugin' hook

#1 follow-up: @TobiasBg
11 years ago

This would be very unreliable as it would not catch the manual uninstallation of a plugin by directly deleting it on the server, e.g. via FTP or SSH...

#2 @Veraxus
11 years ago

  • Summary changed from General 'uninstall_plugin' hook needed in uninstall_plugin() to General 'delete_plugins' hook needed

The previous solution I posted was inadequate as it misses certain uninstall scenarios. A more reliable hook location would be at the end of the delete_plugins() function.

#3 in reply to: ↑ 1 ; follow-up: @Veraxus
11 years ago

  • Keywords needs-patch added

Replying to TobiasBg:

This would be very unreliable as it would not catch the manual uninstallation of a plugin by directly deleting it on the server, e.g. via FTP or SSH...

Right now I'm only trying to log actions taken through the admin. If a file changes on the server, I wouldn't be able to assign "blame", which is the primary reason I'm making this.

That said, my Wish List includes using WordPress's ability to detect remote deletions to log those as well... but right now it's just a "would be nice to have" feature, while this is a blocker.

I've been experimenting with a 'delete_plugins' hook instead ( in delete_plugins() ), and that is working very reliably for detecting when plugins are deleted from the admin. Once I've done some more testing (bulk deletes, custom and automatic uninstalls, etc) I'll post that patch.

Last edited 11 years ago by Veraxus (previous) (diff)

#4 in reply to: ↑ 3 ; follow-up: @nacin
11 years ago

Replying to Veraxus:

That said, my Wish List includes using WordPress's ability to detect remote deletions to log those as well... but right now it's just a "would be nice to have" feature

That sounds more like plugin territory.

Could you explain the "certain uninstall scenarios" that a hook in uninstall_plugin() misses?

@Veraxus
11 years ago

'delete_plugins' hook - A much better, more comprehensive hook. Tested and working.

#5 in reply to: ↑ 4 @Veraxus
11 years ago

Replying to nacin:

That sounds more like plugin territory.

Could you explain the "certain uninstall scenarios" that a hook in uninstall_plugin() misses?

My first patch was inadequate as there are two scenarios handled by that function...

  1. Plugin has it's own uninstall file, run it.
  2. Plugin has no uninstall file, run "uninstall_{$file}" hook instead.

By default, the first scenario has no hooks at all, and my first patch did not add one... so I was unable to handle it at all. And without a patch, the second scenario can't be handled either.

That means there are two viable options for adding the appropriate hooks for "delete any plugin"...

  1. A 'delete_plugins' hook that runs at the end of delete_plugins() (e.g. delete_plugin_hook.diff) OR...
  2. TWO separate hooks in uninstall_plugins(), one that handles custom uninstalls, and then another that handles everything else.

I've tried both options and they both work well (I'll upload a fresh patch momentarily that properly addresses point 2, as the original patch neglects scenarios where a custom uninstall is run).

'delete_plugins' seems to address the need in the simplest way, but would only be run after the files have been deleted (meaning you wouldn't be able to pull metadata like name or author out of the plugin before deletion occurs).

Honestly, I'd be fine with either solution, or both if there is a need.

@Veraxus
11 years ago

Adds the 'uninstall_plugin' hook to both uninstall_plugin() handlers

#6 @Veraxus
11 years ago

  • Keywords has-patch added; needs-patch removed

@Veraxus
11 years ago

Option 3: Formal recommendation - 'delete_plugin' hook within delete_plugins() deletion loop

#7 @Veraxus
11 years ago

So, I've spent the day testing a wide variety of plugins with a wide variety of potential hooks, and it's clear that everything I've proposed so far has serious drawbacks.

1. uninstall_plugin() does not always trigger, so placing any hooks in that function is unhelpful.

2. Likewise, placing a hook at the very end of delete_plugins() means that you can't fetch/log plugin metadata as the plugins have already been deleted.

However, after spending the day testing a massive number of plugins and hook options, I've found that placing a new 'delete_plugin' hook right in the middle of delete_plugins()'s deletion loop (just before $wp_filesystem->delete() is called) works perfectly.

I've uploaded my final recommendation as a patch: delete_plugin_hook_FINAL.diff​

One hook, called 'delete_plugin', that runs on each loop inside delete_plugins() just before each plugin is deleted. Bingo.

Last edited 11 years ago by Veraxus (previous) (diff)

#8 @Veraxus
11 years ago

Any chance we could get this slated for 4.0? My plugin is dead in the water without it.

#9 @juliobox
10 years ago

Hello

@nacin : This is absolutly not a plugin territory since plugin A could hook on plugin B. So when someone deletes plugin B, the hook from plugin A will trigger.

Real example:
The plugin WP Rocket add a hook on "uninstall_woocommerce/woocommerce.php" because when Wooc is deleted, WP Rocket needs to remove the "cart" page from the expluded pages from cache.

But this works only if Wooc has use the uninstall hook and not the uninstall file, not logical.

@TobiasBG : This argument is not valid, you can't refuse a hook because of a possible missuse from a user. Using this way of thinking, why do we have a hook on user_delete if someone can delete it directly from PHPMYADMIN? So, it's not relevant, sorry.

edit : i just thought this: why not just remove the first "return true"? So when uninstall files are included, this is not a blocking situation, then all hooks (added by register_uninstall) will also be triggered!

Thanks to add this simple hook quickly to restore the balance into the force ;)

Last edited 10 years ago by juliobox (previous) (diff)

@DrewAPicture
10 years ago

+ hook doc

#10 follow-up: @DrewAPicture
10 years ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 4.4
  • Type changed from feature request to enhancement

26904.diff moves the hook to just after the plugin deletion attempt and adds a hook doc.

@Veraxus: Can you please test?

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


10 years ago

#12 @DrewAPicture
10 years ago

26904.diff still applies. I'd like to get some others' eyes on it before we move forward here.

#13 @DrewAPicture
10 years ago

  • Owner set to DrewAPicture
  • Status changed from new to reviewing

#14 in reply to: ↑ 10 @Veraxus
10 years ago

Replying to DrewAPicture:

26904.diff moves the hook to just after the plugin deletion attempt and adds a hook doc.

@Veraxus: Can you please test?

@DrewAPicture : This version of the hook seems to be working swimmingly for my use-cases.

#15 @DrewAPicture
10 years ago

@Veraxus Thanks for getting back to us!

#16 @DrewAPicture
10 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 35094:

Plugins: Introduce the delete_plugin transactional hook, which fires immediately after a plugin deletion attempt occurs.

Props Veraxus for the initial patch.
Fixes #26904.

@johnjamesjacoby
9 years ago

Introduce deleted_plugin hook, and move delete_plugin to before

#17 @johnjamesjacoby
9 years ago

  • Keywords 2nd-opinion added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Hey everyone! Switching this up just a bit to match the other activate/deactivate/install/uninstall hooks. This new patch adds before & after hooks, which enables plugins to get information about the plugin (like file headers) being deleted before it happens.


For additional context, the way I stumbled into this omission is from a forum topic for my WP User Activity plugin, where a user noticed no activity was logged when a plugin was deleted from the file system. This was originally because no delete_plugin action existed, but in digging deeper I found the action was added here, but that it's impossible to get the name of the plugin being deleted after it's no longer in the file system (go figure, right?)

Last edited 9 years ago by johnjamesjacoby (previous) (diff)

@johnjamesjacoby
9 years ago

Same as 2.patch, but with corrected filter documentation

#18 @DrewAPicture
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 35669:

Plugins: Rename the delete_plugin action hook (introduced in [35094]) to deleted_plugin as it fires following a plugin deletion attempt.

Further, introduce a new delete_plugin action hook, to be fired before a plugin deletion attempt. Both changes bring parity with other such transactional hooks in core that fire before and after certain actions, including on plugin activation/deactivation and install/uninstall, among others.

Props johnjamesjacoby.
Fixes #26904.

Note: See TracTickets for help on using tickets.