WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 3 months ago

#27365 assigned defect (bug)

Upgrader bulk_upgrade() functions do not return the correct data

Reported by: chrisjean Owned by: aaroncampbell
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: Upgrade/Install Keywords: has-patch
Focuses: Cc:

Description

In the bulk_upgrade() function of Language_Pack_Upgrader, Plugin_Upgrader, and Theme_Upgrader the values returned are not set properly, resulting in very poor feedback from the function on success or failure.

For instance, take the following code sample from Plugin_Upgrader:

$result = $this->run( array(
    'package' => $r->package,
    'destination' => WP_PLUGIN_DIR,
    'clear_destination' => true,
    'clear_working' => true,
    'is_multi' => true,
    'hook_extra' => array(
        'plugin' => $plugin
    )   
) );

$results[$plugin] = $this->result;

Notice how the result is stored in the $result variable yet the $results variable, which is used as the return value, stores the $this->result variable. While the $this->result variable does contain some data, it's not very relevant as a return value for these functions. This issue causes a loss of most, if not all error feedback (I have yet to test every possible combination of result possibilities) and fails to provide any useful success condition responses. In most of my test cases, this issue results in each package having a response value of null.

This same issue can be found in the Language_Pack_Upgrader and Theme_Upgrader classes.

My solution is to simply change $this->result to $result as found in the supplied patch and as shown below:

$results[$plugin] = $result;

Attachments (4)

27365.diff (880 bytes) - added by chrisbliss18 5 years ago.
Proposed fix to allow relevant return values for the bulk_upgrade functions.
27365.diff.1 (1.2 KB) - added by chrisbliss18 5 years ago.
27365.2.diff (3.1 KB) - added by dd32 4 years ago.
27365.3.diff (9.9 KB) - added by dd32 4 years ago.

Download all attachments as: .zip

Change History (28)

@chrisbliss18
5 years ago

Proposed fix to allow relevant return values for the bulk_upgrade functions.

#1 @chrisbliss18
5 years ago

  • Keywords has-patch added

This ticket was mentioned in IRC in #wordpress-dev by chrisbliss18. View the logs.


5 years ago

#3 @SergeyBiryukov
5 years ago

  • Milestone changed from Awaiting Review to 3.9

#4 @johnbillion
5 years ago

  • Keywords needs-testing added
  • Milestone changed from 3.9 to Awaiting Review

#5 @johnbillion
5 years ago

  • Milestone changed from Awaiting Review to 3.9

#6 @chrisbliss18
5 years ago

  • Keywords needs-testing removed

johnbillion suggested that I provide details about what the $this->result variable is.

Digging through the code, it is set (as far as upgrading is concerned) by the WP_Upgrader::install_package() function. After the WP_Upgrader::install_package() function runs, the $this->result variable contains the value returned by the function. Since this value is ultimately returned by the WP_Upgrader::run() function, it would seem that this result would be seen in successful upgrade responses. In my testing, I would see null responses even on successful upgrades, so I'm not sure what accounts for that issue.

Even if the proper response was contained in $this->result, using it in this spot is still a poor decision as it would only contain the response from the WP_Upgrader::install_package() function and not the error responses that WP_Upgrader::run() returns before calling WP_Upgrader::install_package().

Last edited 5 years ago by chrisbliss18 (previous) (diff)

This ticket was mentioned in IRC in #wordpress-dev by chrisbliss18. View the logs.


5 years ago

#8 @dd32
5 years ago

I was asked to weigh in on this.. Bluntly, I'm not sure if it's the right or wrong change. $this->result and $result can be two different things I think, $result = true; $this->result = array( ..., .., ... ).

It could be that the logic needed is something like $results[$plugin] = ( true === $result && $this->result ) ? $this->result : $result;.. but I'm not entirely clear myself on what either of those variables should contain.

If someone can give it a good test through both the single upgrade (not using checkboxes to trigger the update), and bulk upgrade for core, plugins, and themes, hitting both failure and success cases of all, I'd trust this patch as-is

#9 @chrisbliss18
5 years ago

The combinations to test in order to trigger every possible combination of return values is quite high. This is because the entire test stack would have to involve every possible failure condition of request_filesystem_credentials(), download_url(), unzip_file(), and other complex functions.

I don't think that it is necessary to do a full stack test as looking through the code itself shows the problem is just how the returns are handled. I went through and simplified the entire call stack for a bulk_upgrade() function and represented it as pseudocode that focuses on just the return values. You can see that at the end of this comment.

Each USED comment represents a potential return value that is represented in the data returned from the bulk_upgrade() call. Each IGNORED comment represents a potential return value that is missing in the data returned from the bulk_upgrade() call.

From my count (assuming that I didn't miss anything), there are only two potential return values:

  1. false - fs_connect() call fails at the top of the bulk_upgrade() function call.
  2. array - Each entry in the array is for a specific package. There are a few potential values that each entry can have:
    • array - A successful upgrade will provide a non-empty array of details about the upgraded package.
    • WP_Error object - A successful upgrade runs the upgrader_post_install filter before returning. If this filter returns a WP_Error object, this WP_Error object is set in the $this->result variable and returned.
    • $this->result "random" value - If any error occurs when upgrading the package, the value of $this->result will be used. Since this value is not updated when an upgrade fails, whatever value it contains will be used, which means that its value will have no meaning to the actual result of the upgrade. Potential values are: an array from a successful upgrade from an earlier package or a WP_Error object from the upgrader_post_install filter.

Despite the thorough error checking, almost all of these values are missing from the returned data. In fact, most of the potential return values are ignored due to use of the $this->result value rather than the actual returned value. There is no reason that I can find for this as the value of $this->result is always accurately represented in the returned data when the value of $this->result represents a successful upgrade or a WP_Error object returend from the upgrader_post_install filter; however, by using $this->result almost all of the error checking is lost.

In terms of how this change would alter the return value, it would have only one change: adding false as a potential array entry for a package. Looking through the code that calls bulk_upgrade(), wp-admin/update.php has two calls that each ignore the return value, wp-admin/update-core.php has one call that ignores the return value, and wp-admin/includes/class-wp-upgrader.php has one call (in Language_Pack_Upgrader::upgrade()) which does make use of the returned value, but the use assumes an array, which is a problem considering how the value could be false rather than an array. The Language_Pack_Upgrader::upgrade() function is called in WP_Automatic_Updater::update() and has a specific handler for a value of false. So, the only place I can actually find these returned values used in core expects to see and handle a false value.

To account for the flaw in the array expectation in Language_Pack_Upgrader::upgrade(), I've added an additional patch for consideration that covers that issue. I should note that the original change would not create any issue with the Language_Pack_Upgrader::upgrade() function as the code can currently return a non-array result which that function would not handle properly.

function bulk_upgrade() {
        $res = $this->fs_connect();
        if ( !$res )
                return false; // USED
        
        $results = array();
        
        foreach ( $packages as $package ) {
                $result = $this->run() {
                        $res = $this->fs_connect();
                        if ( !$res )
                                return false; // IGNORED
                        if ( is_wp_error( $res ) )
                                return $res; // IGNORED
                        
                        $download = $this->download_package();
                        if ( is_wp_error( $download ) )
                                return $download; // IGNORED
                        
                        $working_dir = $this->unpack_package();
                        if ( is_wp_error( $working_dir ) )
                                return $working_dir; // IGNORED
                        
                        $result = $this->install_package() {
                                if ( 'invalid args' )
                                        return new WP_Error(); // IGNORED
                                
                                $res = apply_filters( 'upgrader_pre_install', true );
                                if ( is_wp_error( $res ) )
                                        return $res; // IGNORED
                                
                                if ( 'empty package archive' )
                                        return new WP_Error(); // IGNORED
                                
                                $source = apply_filters( 'upgrader_source_selection', $source );
                                if ( is_wp_error( $source ) )
                                        return $source; // IGNORED
                                
                                if ( $clear_destination ) {
                                        $removed = $wp_filesystem->delete();
                                        $removed = apply_filters( 'upgrader_clear_destination', $removed );
                                        if ( is_wp_error( $removed ) )
                                                return $removed; // IGNORED
                                        else if ( ! $removed )
                                                return new WP_Error(); // IGNORED
                                } elseif ( $abort_if_destination_exists && $wp_filesystem->exists($remote_destination) ) {
                                        $_files = $wp_filesystem->dirlist($remote_destination);
                                        if ( !empty( $_files ) )
                                                return new WP_Error(); // IGNORED
                                }
                                
                                if ( 'unable to create destination' )
                                        return new WP_Error(); // IGNORED
                                
                                $result = copy_dir($source, $remote_destination);
                                if ( is_wp_error( $result ) )
                                        return $result; // IGNORED
                                
                                // USED
                                $this->result = compact('local_source', 'source', 'source_name', 'source_files', 'destination', 'destination_name', 'local_destination', 'remote_destination', 'clear_destination', 'delete_source_dir');
                                
                                $res = apply_filters( 'upgrader_post_install', true );
                                if ( is_wp_error( $res ) ) {
                                        $this->result = $res; // USED
                                        return $res; // IGNORED
                                }
                                
                                return $this->result; // IGNORED
                        }
                        
                        return $result; // IGNORED
                }
                
                $result[$package] = $this->result;
        }
        
        return $results;
}
Last edited 5 years ago by chrisbliss18 (previous) (diff)

@chrisbliss18
5 years ago

#10 @samuelsidler
5 years ago

  • Keywords 4.0-early added
  • Milestone changed from 3.9 to Future Release

It's too late in the cycle to make this change now. We can get it early in 4.0.

#11 @chrisbliss18
5 years ago

While that's not what I wanted to hear, I do understand. What's the best way to ensure that this gets attention early in the 4.0 shuffle?

#12 @samuelsidler
5 years ago

It's got a 4.0-early keyword so it'll get moved to the 4.0 milestone after that exists and we'll get it on someone's radar (e.g., @dd32).

#14 follow-up: @dd32
5 years ago

FWIW, it looks like the changes here are probably correct, most of the $this->result should be replaced with simply $result, except in the Theme Upgrader where it is something else.

At present for example, Plugin_Upgrader->upgrade() returns null for failures instead of the WP_Error.

Sorry @chrisbliss18 but as you can probably tell, following some of the code isn't as easy as you expect at first glance.

#15 @johnbillion
5 years ago

  • Version changed from trunk to 2.9

#16 @jdgrimes
5 years ago

Somehow this never got on the 4.0 milestone.


This is also an issue in the install() and upgrade() methods of the plugin and theme upgraders. WP_Upgrader::run() is called, but the return value isn't used, just $this->result. Which again means that valuable error info is getting lost on the calling function. Of course, these upgraders were designed to output the error messages to the user directly through the skins, but since they are returning information, they might as well return useful information.

Really, the upgraders could use some unit tests (difficult, but not impossible), to at least define the expected behavior.

#17 in reply to: ↑ 14 @jdgrimes
5 years ago

Replying to dd32:

FWIW, it looks like the changes here are probably correct, most of the $this->result should be replaced with simply $result, except in the Theme Upgrader where it is something else.

At present for example, Plugin_Upgrader->upgrade() returns null for failures instead of the WP_Error.

Sorry @chrisbliss18 but as you can probably tell, following some of the code isn't as easy as you expect at first glance.

Indeed the code is difficult to follow, and I don't think that $this->result is any different than elsewhere in the theme upgrader's install() and upgrade() methods, though I haven't examined bulk_upgrade() closely.

#18 @aaroncampbell
4 years ago

  • Keywords 4.3-early added; 4.0-early removed

Running into this again. Unfortunately we're at the end of a cycle again, so I'm tagging it to look at early next cycle.

#19 @obenland
4 years ago

  • Owner set to aaroncampbell
  • Status changed from new to assigned

#20 @obenland
4 years ago

  • Keywords 4.3-early removed
  • Milestone changed from Future Release to 4.3

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


4 years ago

#22 @helen
4 years ago

  • Milestone changed from 4.3 to Future Release

No actual movement in 4.3, needs somebody to really own it. Since this area is so confusing, it is probably worth having more documentation and/or tests before making such a change.

@dd32
4 years ago

@dd32
4 years ago

#23 @dd32
4 years ago

Looking at this again based off #34403 and It's still as confusing as it was when reported.

It looks like $this->result is the "correct" thing here, the fault being that the WP_Error instance isn't being set to that variable.
It's much more clear if you ignore the bulk_upgrade() methods and instead focus on upgrade() and install().

  • $result = $this->run(..) is mostly a true|false conditional, with a WP_Error instance thrown in there, because why not?
  • $this->result is designed to hold the specific error, or, information about what was actually installed.

Based on that, 27365.2.diff does what's intended, the results from bulk_upgrade() now match what you'd expect.

Now, on the flip-side... Lets look at a life without $this->result having errors in it at all..

So if you take $this->result as mostly intended for a way for WP_Upgrader to pass details of what it had operated on to WP_Upgrader_Skin, so it could pull in contextual information (ie. plugin name/version) I think, then there was no need for it to ever store errors in $this->result as it was also calling $this->skin->error($error).

So removing $this->result for error cases ends up looking like 27365.3.diff instead, which looks sane-ish at first look.. and I can't see anything broken.. yet.. but I haven't tested in depth.
It should be noted, that with either of these patches, anything that uses $upgrader->bulk_upgrade() potentially breaks (Including our own Ajax update handlers - it fatals); so the fix here is really not going to be 100% backwards compatible, it's going to break it for some implementations for a better life for everyone else..

Last edited 4 years ago by dd32 (previous) (diff)

#24 @mordauk
4 years ago

27365.3.diff looks like it should work to me but I'm still not seeing any errors caught. When errors aren't caught, per 34403, smooth plugin updates never complete and just spin forever.

Note: See TracTickets for help on using tickets.