[wp-trac] [WordPress Trac] #27365: Upgrader bulk_upgrade() functions do not return the correct data
WordPress Trac
noreply at wordpress.org
Mon Mar 24 20:08:06 UTC 2014
#27365: Upgrader bulk_upgrade() functions do not return the correct data
-----------------------------+--------------------
Reporter: chrisbliss18 | Owner:
Type: defect (bug) | Status: new
Priority: normal | Milestone: 3.9
Component: Upgrade/Install | Version: trunk
Severity: normal | Resolution:
Keywords: has-patch | Focuses:
-----------------------------+--------------------
Comment (by chrisbliss18):
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 this is necessary to do this 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.
1. 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.
{{{
#!php
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;
}
}}}
--
Ticket URL: <https://core.trac.wordpress.org/ticket/27365#comment:9>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform
More information about the wp-trac
mailing list