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

[pwtyler]

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.

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

[audrasjb]

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

[afragen]

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

[afragen]

Issue also created for ​WP-CLI

Perhaps a better method than #1188

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

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

Thanks for the PR! Merged in https://core.trac.wordpress.org/changeset/51528.