WordPress.org

Make WordPress Core

Opened 17 months ago

Last modified 14 months ago

#26735 new defect (bug)

Plugin bulk deletion attempts to define WP_UNINSTALL_PLUGIN constant multiple times

Reported by: jdgrimes Owned by:
Milestone: Future Release Priority: low
Severity: minor Version: 2.7
Component: Plugins Keywords: has-patch
Focuses: administration Cc:

Description

The WP_UNINSTALL_PLUGIN constant is defined by uninstall_plugin() before the plugin's uninstall.php file is included (if the plugin has one). When deleting multiple plugins with uninstall.php files, the function attempts to define this constant each time, which will result in a notice.

PHP Notice: Constant WP_UNINSTALL_PLUGIN already defined

The notice will never be noticed by most users, because it will be silenced by default, but this could be an issue if plugins are checking for the value of WP_UNINSTALL_PLUGIN, as recommended here (which is linked to from the codex), and not just whether it is defined.

I don't have a good solution, sorry.

Attachments (2)

26735.diff (425 bytes) - added by spmlucas 16 months ago.
26735.2.diff (495 bytes) - added by jdgrimes 16 months ago.
Define WP_UNINSTALL_PLUGIN as true

Download all attachments as: .zip

Change History (10)

comment:1 @jdgrimes17 months ago

One possibility would be to start just defining WP_UNINSTALL_PLUGIN as true if it isn't already defined, but that won't be backward compatible.

comment:2 follow-up: @nacin16 months ago

  • Component changed from Plugins to Admin APIs
  • Focuses administration added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 3.9

WP_UNINSTALL_PLUGIN is simply there so a plugin's uninstall.php file can confirm it was indeed invoked from the uninstall routine. We simply need to do if ( ! defined( 'WP_UNINSTALL_PLUGIN' ) ) defined( 'WP_UNINSTALL_PLUGIN', true ); It'd only avoid the notice, not actually change what's happening.

comment:3 @nacin16 months ago

  • Component changed from Admin APIs to Plugins

Sorry for the noise.

comment:4 in reply to: ↑ 2 ; follow-up: @jdgrimes16 months ago

Replying to nacin:

WP_UNINSTALL_PLUGIN is simply there so a plugin's uninstall.php file can confirm it was indeed invoked from the uninstall routine. We simply need to do if ( ! defined( 'WP_UNINSTALL_PLUGIN' ) ) defined( 'WP_UNINSTALL_PLUGIN', true ); It'd only avoid the notice, not actually change what's happening.

Right, that seems like the logical solution. The problem is that it could cause some plugins not to uninstall themselves, because it's not 100% backward compatible. I mentioned that in the original post, but I now see that I really didn't make it clear. Some people are checking it like this:

if ( __FILE__ != WP_UNINSTALL_PLUGIN )
     return;

Because right now the value of WP_UNINSTALL_PLUGIN is the path of the plugin's main file. If we just change it to true, the plugin won't uninstall. Unfortunately, this might be fairly ubiquitous (see the original post).

@spmlucas16 months ago

comment:5 follow-up: @spmlucas16 months ago

  • Keywords has-patch added; needs-patch removed

26735.diff simply adds the if statement to prevent the error. I've left the declaration of WP_UNINSTALL_PLUGIN alone, but I don't think changing it to true is a big deal. The codex prescribes simply checking for the existence of the constant, not the value - checking the value during a bulk uninstall is going to fail unless the plugin happens to be the first in the batch.

comment:6 in reply to: ↑ 4 @jdgrimes16 months ago

Replying to jdgrimes:

The problem is that it could cause some plugins not to uninstall themselves, because it's not 100% backward compatible. I mentioned that in the original post, but I now see that I really didn't make it clear. Some people are checking it like this:

if ( __FILE__ != WP_UNINSTALL_PLUGIN )
     return;

Because right now the value of WP_UNINSTALL_PLUGIN is the path of the plugin's main file. If we just change it to true, the plugin won't uninstall. Unfortunately, this might be fairly ubiquitous (see the original post).

Actually, forget about this. I just realized that this will actually be backward compatible if we just define WP_UNINSTALL_PLUGIN as true. Because __FILE__ == true. So, as long as they aren't use strict typechecking (!==), they should be fine.

@jdgrimes16 months ago

Define WP_UNINSTALL_PLUGIN as true

comment:7 in reply to: ↑ 5 @jdgrimes16 months ago

Replying to spmlucas:

26735.diff simply adds the if statement to prevent the error. I've left the declaration of WP_UNINSTALL_PLUGIN alone, but I don't think changing it to true is a big deal. The codex prescribes simply checking for the existence of the constant, not the value - checking the value during a bulk uninstall is going to fail unless the plugin happens to be the first in the batch.

That is true, but it is being done, and possibly by a lot of developers. Although the value isn't checked in the codex examples, one of the reference links there does prescribe this (as I pointed out in the original post). The fact that it is defined to the file path is misleading. One naturally assumes that it is OK to check the value. I believe it was originally intended that the value always reflect the plugin being uninstalled. Changing it to true would make it so those plugin's that are checking the value (and thought they were doing it right) would uninstall properly. I don't know of any case where it is necessary to check which plugin is being uninstalled, that would be broken by this.

comment:8 @nacin14 months ago

  • Milestone changed from 3.9 to Future Release
  • Priority changed from normal to low
  • Severity changed from normal to minor

No good solution here, punting.

An uninstall routine really just needs to lead with defined( 'WP_UNINSTALL_PLUGIN' ) ) or exit;. I didn't even know until this ticket it was defined as __FILE__.

Note: See TracTickets for help on using tickets.