[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