WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 3 years ago

#16741 closed defect (bug) (fixed)

uninstall_plugins is autoloaded

Reported by: duck_ Owned by: 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_ 5 years ago.
16741.upgrade.diff (1.8 KB) - added by duck_ 5 years ago.
16741.2.diff (1.6 KB) - added by nacin 4 years ago.

Download all attachments as: .zip

Change History (16)

@duck_5 years ago

comment:1 follow-up: @nacin5 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_5 years ago

comment:2 in reply to: ↑ 1 @duck_5 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.

comment:3 @scribu5 years ago

Nice catch.

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

comment:4 @dd324 years ago

  • Milestone changed from Awaiting Review to 3.4

comment:5 @nacin4 years ago

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

@nacin4 years ago

comment:6 @nacin4 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.

comment:7 @nacin4 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.

comment:8 @scribu4 years ago

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

comment:9 @nacin4 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.

comment:10 @nacin4 years ago

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

comment:11 @scribu4 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.

comment:12 @jkudish3 years ago

  • Keywords has-patch added

comment:13 @nacin3 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.