Make WordPress Core

Opened 13 years ago

Closed 12 years ago

#16741 closed defect (bug) (fixed)

uninstall_plugins is autoloaded

Reported by: duck_'s profile duck_ Owned by: nacin's profile nacin
Milestone: 3.4 Priority: normal
Severity: normal Version: 2.7
Component: General Keywords: has-patch
Focuses: Cc:

Description

The uninstall_plugins option is autoloaded despite documentation suggesting otherwise. This has been the case since it was introduced, see #5625 and [8585].

Example patch attached. Or it could be left as autoloaded and comments removed.

Attachments (3)

16741.diff (1.0 KB) - added by duck_ 13 years ago.
16741.upgrade.diff (1.8 KB) - added by duck_ 13 years ago.
16741.2.diff (1.6 KB) - added by nacin 12 years ago.

Download all attachments as: .zip

Change History (16)

@duck_
13 years ago

#1 follow-up: @nacin
13 years ago

No wonder why class method callbacks slowed sites down as much as they did.

We can just add this to $fat_options and handle the switch in an upgrade path.

@duck_
13 years ago

#2 in reply to: ↑ 1 @duck_
13 years ago

Replying to nacin:

No wonder why class method callbacks slowed sites down as much as they did.

We can just add this to $fat_options and handle the switch in an upgrade path.

16741.upgrade.diff: semi-done (no db_version bump or call to upgrade path).

The difference would be that uninstall_plugins would become a default option instead of added when necessary. If that's undesired then a mixture of the two can be taken. Have an upgrade path for existing installs and set autoload to 'no' on the first registration so it's not a default option.

#3 @scribu
13 years ago

Nice catch.

I don't see a problem with it becoming a default option, since it isn't autoloaded.

#4 @dd32
12 years ago

  • Milestone changed from Awaiting Review to 3.4

#5 @nacin
12 years ago

Upgrade path should probably be get, delete, add, rather than a direct query. No issues with it being a default option.

@nacin
12 years ago

#6 @nacin
12 years ago

  • Keywords 2nd-opinion removed

Refreshed for 3.4. I realized since populate_options() gets called before upgrade_*(), the option will always exist by the time we try to call get_option(), so a direct query to ascertain its autoload property seems like the best route.

#7 @nacin
12 years ago

  • Owner set to nacin
  • Resolution set to fixed
  • Status changed from new to closed

In [20080]:

Don't autoload the uninstall_plugins option. fixes #16741.

#8 @scribu
12 years ago

Note that now plugin authors will have to wrap register_uninstall_hook() in an is_admin() check, to avoid an extra query.

#9 @nacin
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Hmm. Perhaps we should just noop register_uninstall_hook() when we're not in the admin.

#10 @nacin
12 years ago

As it is, register_uninstall_hook() should only be occurring on an activation hook.

#11 @scribu
12 years ago

  • Keywords has-patch removed

Hmm. Perhaps we should just noop register_uninstall_hook() when we're not in the admin.

My thoughts exactly.

#12 @jkudish
12 years ago

  • Keywords has-patch added

#13 @nacin
12 years ago

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

register_uninstall_hook() should only be called on activation. If they discover that it is generating an extra query (on the frontend), then they are doing it wrong.

Note: See TracTickets for help on using tickets.