[wp-trac] [WordPress Trac] #51928: Provide plugin/theme update failure data to dot org

[WordPress Trac]

#51928: Provide plugin/theme update failure data to dot org
--------------------------------------+---------------------
 Reporter:  afragen                   |       Owner:  (none)
     Type:  task (blessed)            |      Status:  new
 Priority:  normal                    |   Milestone:  5.6.1
Component:  Upgrade/Install           |     Version:
 Severity:  normal                    |  Resolution:
 Keywords:  has-patch has-unit-tests  |     Focuses:
--------------------------------------+---------------------

Comment (by dd32):

 I've been testing PR 850 and it's data storage on dotorg, and I can
 happily say the data is being saved correctly now, however..

 Replying to [comment:34 afragen]:
 > > I suspect I know the answer here, being that it's hard to retrieve
 those details, but I'm thinking maybe the Automatic updater skin could act
 as a temporary store of what is currently being processed. This could be
 done in a follow up change I guess.
 > >
 > Unfortunately that data isn't available in the Upgrader object.
 Depending upon the data passed in the WP_Error, it might show the slug by
 reference to the destination folder. I know, not necessarily the same
 thing.

 In my testing of this PR, it looks like it's actually very common for it
 to not have even the name available. The current way that it's retrieving
 the details isn't reliable enough and only covers a singular use-case
 (Plugin/Theme upgrade via the ajax upgrader).

 It also incorrectly identifies most plugin upgrade/install attempts as
 `automatic_plugin_update`.


 > I have made a small PR to @hellofromTonya's PR
 [https://github.com/WordPress/wordpress-develop/pull/850] to include error
 reporting for `copy_dir()` errors.

 Currently the PR as is, actually sends two API pings, with your proposed
 changes, it sends 3 :)

 I'm not sure I understand what the 'process' field is for, as the WP_Error
 objects should have a unique error_code value that specifies the issues
 being encountered... The inclusion of that is what was causing double api
 pings to be sent.

 I've made some significant-ish changes via
 [https://github.com/hellofromtonya/wordpress-develop/pull/2 PR2 on PR850]
 ([https://github.com/dd32/wordpress-develop/tree/add/51928-update-failure-
 telemetry-2 my branch]) which simplifies the process a little, and ensures
 that slug/version context is always sent with the failure pings, which
 makes the data much more useful.

 I've ditched the `process` key, relying upon the WP_Error error codes
 being unique enough.
 I've also removed most of the pings from WP_Upgrader, relegating the
 failure catching to `::install(), ::upgrade(), and ::bulk_upgrade()`
 instead.
 I haven't tested this with Background auto-updates, but I think it should
 catch those upgrades correctly, and identify them as such.

 I've ignored the unit tests, because I'm almost 100% certain it'll now be
 failing after these changes, and as far as I can tell was probably too
 decoupled from it anyway to catch anything useful.
 The tests also include some platform-specific error codes,
 `PCLZIP_ERR_MISSING_FILE` which to me suggests the existing tests probably
 would've failed on a PHP with the `zip` extension enabled.
 I'd highly support not including any unit testing what so ever of this
 change.

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