WordPress.org

Make WordPress Core

Opened 6 months ago

Closed 3 months ago

Last modified 3 months ago

#53002 closed defect (bug) (fixed)

Plugin_Upgrader's bulk_upgrade returns incorrect results when an error is encountered

Reported by: pwtyler Owned by: SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: normal Version: 2.9
Component: Upgrade/Install Keywords: has-patch commit
Focuses: Cc:

Description

I initially thought this was a WP-CLI issue and dug in from there, but we're seeing that the bad results are coming from bulk_upgrade() in wp-admin/includes/class-plugin-upgrader.php.

When multiple plugins are updated and one succeeds and another fails, both end up treated as a success (Updated).

The logs printed show the expected output (error updating the second plugin) but the final output shows success for both updates.

Steps to reproduce (with WP-CLI)

$ wp plugin install akismet --version=4.0 --force
$ wp plugin install wordpress-importer --version=0.5 --force
$ chmod -w wp-content/plugins/wordpress-importer
$ wp plugin update --all
# > See "Success: Updated 2 of 2 plugins."
$ wp plugin list 
# > See "wordpress-importer" is still at version 0.5

Digging deeper, I can see the array of results created at line 350 of wp-admin/includes/class-plugin-upgrader.php is not reporting the correct information for the second of the two updates (the failed wordpress-importer update)— instead of a WP_Error Object, we're seeing a result for Akismet, updated previously.

--
Thanks to @nullvariable for the assistance tracking this down.

Change History (17)

This ticket was mentioned in Slack in #cli by pwtyler. View the logs.


6 months ago

This ticket was mentioned in PR #1188 on WordPress/wordpress-develop by pwtyler.


6 months ago

  • Keywords has-patch added

When bulk updating plugins or themes, the result of a previous update remained through the loop, and if a subsequent update failed (which should have then been null result), the failed update would end up with the result of the previous operation.

This PR unsets the result at the top of the bulk upgrade loop if it is not already null to prevent the results of the first plugin or theme's success from cascading through a subsequent failure and resulting in a false positive on an update result.

Trac ticket: https://core.trac.wordpress.org/ticket/53002

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


6 months ago

#4 @SergeyBiryukov
6 months ago

  • Milestone changed from Awaiting Review to 5.8

#5 @SergeyBiryukov
6 months ago

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

This ticket was mentioned in Slack in #core-auto-updates by audrasjb. View the logs.


4 months ago

#7 @audrasjb
4 months ago

  • Milestone changed from 5.8 to 5.9

Hello,
WP 5.8 beta 1 is planned today. Let's move this ticket for 5.9 consideration.

This ticket was mentioned in PR #1364 on WordPress/wordpress-develop by afragen.


4 months ago

#9 @afragen
4 months ago

https://github.com/WordPress/wordpress-develop/pull/1364

When bulk updating plugins/themes, the global $this->result is used to identify the result of $this->run(). This seems to only update for the first plugin/theme updated. Using the local variable $result seems to fix this issue.

Using the example in the description.

$ wp plugin update --all                         
Downloading update from https://downloads.wordpress.org/plugin/akismet.4.1.9.zip...
Using cached file '/Users/afragen/.wp-cli/cache/plugin/akismet-4.1.9.zip'...
Unpacking the update...
Installing the latest version...
Removing the old version of the plugin...
Plugin updated successfully.
Downloading update from https://downloads.wordpress.org/plugin/wordpress-importer.0.7.zip...
Using cached file '/Users/afragen/.wp-cli/cache/plugin/wordpress-importer-0.7.zip'...
Unpacking the update...
Installing the latest version...
Removing the old version of the plugin...
Warning: Could not remove the old plugin.
Plugin update failed.
+--------------------+-------------+-------------+---------+
| name               | old_version | new_version | status  |
+--------------------+-------------+-------------+---------+
| akismet            | 4.0         | 4.1.9       | Updated |
| wordpress-importer | 0.5         | 0.7         | Updated |
+--------------------+-------------+-------------+---------+
Success: Updated 2 of 2 plugins.

The success message is somewhere inside of WP-CLI.

#10 @afragen
4 months ago

Issue also created for WP-CLI

#11 @prbot
4 months ago

afragen commented on PR #1364:

Perhaps a better method than #1188

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


4 months ago

#13 @SergeyBiryukov
4 months ago

  • Keywords commit added

#14 @SergeyBiryukov
3 months ago

  • Version changed from 5.7 to 2.9

Thanks for the PR! It looks good to me.

For reference, here's what currently happens in my testing (without the patch):

  • Plugin_Upgrader::bulk_upgrade() calls WP_Upgrader::run().
  • WP_Upgrader::run() calls ::download_package(), ::unpack_package(), and ::install_package().
  • $this->result is set in WP_Upgrader::install_package(), but only if an error has not occurred earlier.
  • If an error did occur, $this->result contains the result of the previous operation and not the current one.

It appears to have worked this way since the initial introduction in [12097] / #10973, setting the version accordingly.

With the patch, if an error occurred for one of the plugins being updated, a WP_Error object is stored, as expected.

#15 @SergeyBiryukov
3 months ago

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

In 51528:

Upgrade/Install: Store correct result when bulk updating plugins or themes.

This ensures that when multiple plugins or themes are updated and one succeeds and another fails, the error is reported accordingly.

Previously, both updates would end up treated as a success, due to $this->result containing the result of the previous operation and not the current one.

Follow-up to [12097].

Props pwtyler, afragen.
Fixes #53002.

#16 @prbot
3 months ago

SergeyBiryukov commented on PR #1188:

Thanks for the PR! An alternative approach was merged in https://core.trac.wordpress.org/changeset/51528.

Note: See TracTickets for help on using tickets.