WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 8 months ago

#24496 closed enhancement (fixed)

Plugin_Upgrader::bulk_upgrade() and Theme_Upgrader::bulk_upgrade() sanity checks

Reported by: jamescollins Owned by: dd32
Milestone: 3.7 Priority: normal
Severity: normal Version: 2.9
Component: Upgrade/Install Keywords: has-patch 3.7-early
Focuses: Cc:

Description

Currently, the Plugin_Upgrader::bulk_upgrade() and Theme_Upgrader::bulk_upgrade() functions don't perform any checks to make sure at least one plugin/theme is passed to it.

This has the side effect in WordPress Multisite that if the function is called with an empty array being passed to it, then maintenance mode is enabled and disabled even though no plugins or themes are updated.

Background: https://github.com/wp-cli/wp-cli/issues/491

Attachments (5)

24496.diff (543 bytes) - added by jamescollins 11 months ago.
24496.2.diff (595 bytes) - added by jamescollins 11 months ago.
24496.3.diff (6.5 KB) - added by jamescollins 8 months ago.
24496.4.diff (541 bytes) - added by jamescollins 8 months ago.
Return empty array (improvement to 24496.2.diff​)
24496.5.diff (1.9 KB) - added by jamescollins 8 months ago.
Implements ryan's suggestion and improves comments

Download all attachments as: .zip

Change History (13)

comment:1 rmccue11 months ago

  • Cc rmccue added

jamescollins11 months ago

comment:2 jamescollins11 months ago

  • Keywords has-patch added
  • Version set to 2.9

It looks like it has been this way since the bulk updater was introduced in v2.9 ([12097]).

jamescollins11 months ago

comment:3 jamescollins11 months ago

A workaround for this has now been implemented in wp-cli, but I think it would be a good idea to implement 24496.2.diff​ in core to help prevent problems with other third party code using the plugin/theme upgrader code.

comment:4 SergeyBiryukov10 months ago

  • Keywords 3.7-early added
  • Milestone changed from Awaiting Review to Future Release

comment:5 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

comment:6 ryan9 months ago

Can we return early like that or does footer() still need to be called? If so, a possible alternative is:

$maintenance = ( is_multisite() && ! empty( $plugins ) )

jamescollins8 months ago

jamescollins8 months ago

Return empty array (improvement to 24496.2.diff​)

comment:7 jamescollins8 months ago

Hi @ryan,

If no plugins/themes are selected when using the WordPress admin interface to upgrade, plugins/themes a message of
Please select one or more plugins to update.
or
Please select one or more themes to update.
is displayed (via http://core.trac.wordpress.org/browser/trunk/src/wp-admin/update-core.php#L436), which means the Plugin_Upgrader::bulk_upgrade() and Theme_Upgrader::bulk_upgrade() functions don't get executed at all.

So the only case where an empty array could be passed to the bulk_upgrade() function is via third party code (such as WP CLI), in which case I think it is ok to simply return an empty array and output nothing. 24496.4.diff​ does this.

Alternatively, if you think it is still best to output the header/footer information, then my approach in 24496.3.diff​ could make sense. I thought an explicit if statement was easier to read and understand compared to your suggested alternative.

My preferred approach is 24496.4.diff​.

jamescollins8 months ago

Implements ryan's suggestion and improves comments

comment:8 dd328 months ago

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

In 25048:

WP_Upgrader: Don't activate maintenance mode in bulk_upgrade() when no Themes or Plugins are specified. This doesn't affect Core, but rather, plugins who use the upgrade routines and do not do precautionary tests. Props jamescollins. Fixes #24496

Note: See TracTickets for help on using tickets.