WordPress.org

Make WordPress Core

Opened 4 months ago

Last modified 5 weeks ago

#53002 reviewing defect (bug)

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: 5.7
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 (13)

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


4 months ago

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


4 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.


4 months ago

#4 @SergeyBiryukov
4 months ago

  • Milestone changed from Awaiting Review to 5.8

#5 @SergeyBiryukov
4 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.


7 weeks ago

#7 @audrasjb
7 weeks 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.


7 weeks ago

#9 @afragen
7 weeks 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
7 weeks ago

Issue also created for WP-CLI

#11 @prbot
5 weeks 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.


5 weeks ago

#13 @SergeyBiryukov
5 weeks ago

  • Keywords commit added
Note: See TracTickets for help on using tickets.