[wp-trac] [WordPress Trac] #33539: wp_upload_bits() should fire wp_handle_upload

[WordPress Trac]

#33539: wp_upload_bits() should fire wp_handle_upload
--------------------------+-----------------------------
 Reporter:  dllh          |      Owner:
     Type:  defect (bug)  |     Status:  new
 Priority:  normal        |  Milestone:  Awaiting Review
Component:  Media         |    Version:
 Severity:  normal        |   Keywords:
  Focuses:                |
--------------------------+-----------------------------
 I've found a tricky issue with `wp_upload_bits()`. My use case is that for
 some file replication functionality, I'm hooking onto `wp_handle_upload`,
 which works fine in most cases. But in the case of sideloading thumbnails
 for an uploaded mp3, my file replication breaks because `wp_upload_bits()`
 doesn't fire `wp_handle_upload` (nor does the sideloading code).

 The only two things I've found that use `wp_upload_bits()` in core are
 this attachment metadata code and the xml-rpc media endpoint. The xml-rpc
 code does fire `wp_handle_upload`, though it does so after inserting the
 attachment, when it seems like maybe it should happen before insertion (in
 case there's some error).

 I think the way to introduce consistency here is maybe to add
 `wp_handle_upload` to `wp_upload_bits()` and remove it from the xml-rpc
 endpoint. I'm not sure whether there might be backwards compatibility
 concerns here, especially with the xml-rpc endpoint.

 The repro in my case (which won't apply for most people but which might be
 helpfully illustrative) is simply to upload an mp3 file. When the file is
 parsed for ID3 tags and the thumbnail extracted, its bits are processed,
 but since `wp_handle_upload` doesn't fire in this context, the file isn't
 replicated properly and I wind up with a broken image in the media library
 (and thus a broken thumbnail for the mp3).

 You can simulate a repro without having to implement file replication by
 simply writing a plugin that hooks onto `wp_handle_upload` and logs some
 debug info; for normal uploads, you'll see the debug, but in the case of
 this thumbnail sideload, you'll see debug output for the mp3 but not for
 its thumbnail. So while the specific file replication repro here isn't
 terribly common, it could certainly affect other use cases.

 I'm attaching a patch that adds `wp_handle_upload` to `wp_upload_bits()`
 and removes it from the xml-rpc function (it won't be necessary since the
 xml-rpc function calls the here modified `wp_upload_bits()` function. The
 main things I'm not sure about in this patch are:

 * Are there any backwards compatibility concerns?
 * I'm not sure what the best second arg is in this case for
 `wp_handle_upload`; in the case of the media metadata, sideload seems
 appropriate, but is it sort of also appropriate for the xml-rpc or do we
 consider it an upload in that case? Is there a reasonable way to detect
 and send along a different context if needed?
 * I ran unit tests and nothing broke, though with default config, some
 tests are skipped. I think this is normal but am not positive.

 I've tested the patch on a trunk multisite with both image and mp3 uploads
 and see no regressions.

--
Ticket URL: <https://core.trac.wordpress.org/ticket/33539>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform