Repository navigation
Report failed DAM imports instead of claiming success - #1358
Open
gabrielcld2 wants to merge 1 commit into
Open
gabrielcld2 wants to merge 1 commit into
gabrielcld2 wants to merge 1 commit into
Conversation
download_asset() returns a WP_Error when the download fails, but import_asset() only caught exceptions, so the error was dropped and the REST endpoint answered with success. The editor got a broken attachment with no file, and handle_failed_download() never ran. - import_asset() now returns the download error, so the attachment is deleted and the error is sent to the media modal. - handle_failed_download() logs the attachment ID, public_id and error to the plugin debug log and the PHP error log. - download_asset() deletes the file it primed for the download when the request fails or the response isn't a 200, instead of leaving an empty file in the uploads folder.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes failed imports from the Cloudinary DAM being reported as successful.
The issue
When the server fails to download an asset inserted from the DAM, for example a network error or a
401/404/5xxresponse, the import is reported as a success:Download_Sync::download_asset()returns aWP_Erroron failure, butimport_asset()only caught exceptions. The error was dropped and the REST endpoint responded withsuccess: true.handle_failed_download()never ran, and it didn't log anything anyway.One way to hit this is an account using
authenticateddelivery: inserting an asset with a crop or an effect applied makes the plugin download the unsigned derived URL, which Cloudinary answers with a401. That underlying download issue is out of scope here and will be handled separately. This PR makes sure any such failure is reported and cleaned up.The solution
import_asset()now returns the download error. The REST endpoint then callshandle_failed_download(), which deletes the attachment and sends the error to the media modal, which shows it in an alert.handle_failed_download()logs the attachment ID, public_id and error, both to the plugin debug log (System Report) and to the PHP error log.download_asset()deletes the primed file when the request fails or the response isn't a200. This also covers the other callers (Storage,Unsync).tests/phpunit/tests/test-download-sync.php(6 tests). 4 of them fail ondevelop; the other 2 guard existing behaviour.Known limitation: after a failed import, a tile without a thumbnail can stay in the media modal grid until the page is reloaded. It comes from the existing
.fail()handler inmedia-modal.js, which was rarely reached before this fix. The attachment itself is deleted.QA notes
Prerequisites
WP_DEBUGandWP_DEBUG_LOGenabled (both on by default in the repo'swp-env).Simulate a failed download. Add this mu-plugin, e.g.
.wp-env/mu-plugins/qa-block-cloudinary-download.phponwp-env. It makes the server's download of the asset from Cloudinary fail, while the DAM keeps working in the browser:Steps
wp-content/uploads/<year>/<month>/.develop: no error is shown. Depending onWP_DEBUG_DISPLAY, the item either stays in the modal with a grey placeholder or silently disappears (a PHP warning breaks the JSON response). After a reload, Media > Library has a new attachment with no image, there's a leftover file in the uploads folder (a few bytes, or empty on a4xx/5xxresponse), and nothing is logged.QA: blocked download from Cloudinary. No new attachment is in Media > Library after a reload, no new file is in the uploads folder, andwp-content/debug.loghas a lineCloudinary: failed to import asset into attachment <id> (public_id: <public_id>). Deleting the attachment. Error: QA: blocked download from Cloudinary.Optional: if you have an asset using
authenticateddelivery, insert it from the DAM with an effect applied (without the mu-plugin). On this branch you get an alert with Cloudinary's error (e.g.deny or ACL failure) instead of a broken attachment.