[wp-trac] [WordPress Trac] #27365: Upgrader bulk_upgrade() functions do not return the correct data

[WordPress Trac]

#27365: Upgrader bulk_upgrade() functions do not return the correct data
-----------------------------+-----------------------------
 Reporter:  chrisjean        |       Owner:  aaroncampbell
     Type:  defect (bug)     |      Status:  assigned
 Priority:  normal           |   Milestone:  Future Release
Component:  Upgrade/Install  |     Version:  2.9
 Severity:  normal           |  Resolution:
 Keywords:  has-patch        |     Focuses:
-----------------------------+-----------------------------

Comment (by dd32):

 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, [attachment:27365.2.diff 27365.2.diff] does what's
 intended, the results from `bulk_upgrade()` now match what you'd expect.

 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
 [attachment: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..

--
Ticket URL: <https://core.trac.wordpress.org/ticket/27365#comment:23>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform