Opened 11 years ago
Last modified 6 years ago
#27365 assigned defect (bug)
Upgrader bulk_upgrade() functions do not return the correct data
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (28)
This ticket was mentioned in IRC in #wordpress-dev by chrisbliss18. View the logs.
11 years ago
#6
@
11 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()
.
This ticket was mentioned in IRC in #wordpress-dev by chrisbliss18. View the logs.
11 years ago
#8
@
11 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
@
11 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:
- false -
fs_connect()
call fails at the top of thebulk_upgrade()
function call. - 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 theupgrader_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;
}
#10
@
11 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
@
11 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
@
11 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:
↓ 17
@
11 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.
#16
@
11 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
@
11 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
@
10 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.
This ticket was mentioned in Slack in #core by helen. View the logs.
10 years ago
#22
@
10 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.
#23
@
9 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 aWP_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..
Proposed fix to allow relevant return values for the bulk_upgrade functions.