[gabrielcld2]

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/5xx response, the import is reported as a success:

• Download_Sync::download_asset() returns a WP_Error on failure, but import_asset() only caught exceptions. The error was dropped and the REST endpoint responded with success: true.
• The editor was left with an attachment that has no file (grey placeholder, broken image on the frontend).
• handle_failed_download() never ran, and it didn't log anything anyway.
• On a non-200 response, the empty response body was streamed into the file primed in the uploads folder, which was then left behind.

One way to hit this is an account using authenticated delivery: inserting an asset with a crop or an effect applied makes the plugin download the unsigned derived URL, which Cloudinary answers with a 401. 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 calls handle_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, the stored public_id and the error, both to the plugin debug log (System Report) and to the PHP error log. The error can come from Cloudinary's x-cld-error header, so it's sanitized first.
• download_asset() deletes the primed file when the request fails, the response isn't a 200, or an exception is thrown. This also covers the other callers (Storage, Unsync).
• The endpoint used to only require upload_files and took the posted attachment_id as is. Since a failed download now deletes the attachment, that would have let any Author delete any attachment. The endpoint now requires an existing attachment the user can edit (edit_post), and a failed download only deletes an attachment that has no file yet and that the user can delete (delete_post). The normal flow is unaffected: the attachment is created by the user doing the import. This also stops an Author from replacing another user's attachment file through this endpoint, which was already possible before.
• The cleanup deletes the attachment in WordPress only. Deleting an attachment normally runs the delete sync, which sends a destroy request for the asset on Cloudinary, and a new import already counts as synced before its file is downloaded. That request currently fails because the pending attachment has no resource type yet, but the asset the user picked in the DAM must never be at risk.
• The Cloudinary DAM admin page (media-library.js) called the same endpoint after every insert, including assets already in the Media Library, which have no attachment to download into. It now only calls it for new assets, as the media modal does, shows the error when the download fails or the request is rejected, and always clears the import progress. Without this, the stricter permission check would leave the progress spinner stuck when re-inserting an imported asset. Includes the rebuilt js/cloudinary.js.
• Added tests/phpunit/tests/test-download-sync.php (16 tests), including the permission checks through the REST route and a check that no destroy request is made.

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 in media-modal.js, which was rarely reached before this fix. The attachment itself is deleted.

QA notes

Prerequisites

• A WP site with the Cloudinary plugin connected and the DAM available in the media modal.
• WP_DEBUG and WP_DEBUG_LOG enabled (both on by default in the repo's wp-env).

Simulate a failed download. Add this mu-plugin, e.g. .wp-env/mu-plugins/qa-block-cloudinary-download.php on wp-env. It makes the server's download of the asset from Cloudinary fail, while the DAM keeps working in the browser:

<?php
add_filter(
	'pre_http_request',
	static function ( $pre, $args, $url ) {
		// Only the full download of the asset; leave the partial file type check alone.
		if ( false !== strpos( $url, 'res.cloudinary.com' ) && ! empty( $args['stream'] ) && empty( $args['limit_response_size'] ) ) {
			return new WP_Error( 'http_request_failed', 'QA: blocked download from Cloudinary' );
		}
		return $pre;
	},
	10,
	3
);

To follow the debug log while testing on wp-env (the site's wp-content folder isn't the one in the repo):

npx wp-env run cli tail -f wp-content/debug.log

Steps

1. In the block editor, add an Image block, open the media modal and go to the Cloudinary tab.
2. Insert an image that isn't in the WordPress Media Library yet.
   • On develop: no error is shown. Depending on WP_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 a 4xx/5xx response), and nothing is logged.
   • On this branch: an alert shows QA: blocked download from Cloudinary. No new attachment is in Media > Library after a reload, no new file is in the uploads folder, and wp-content/debug.log has a line Cloudinary: failed to import asset into attachment <id> (public_id: <public_id>). Deleting the attachment. Error: QA: blocked download from Cloudinary.
3. With the mu-plugin still in place, go to the Cloudinary admin page (the standalone DAM page) and insert another image that isn't in the Media Library yet.
   • On develop: no error is shown, and the import progress clears as if it worked.
   • On this branch: the same alert shows, the import progress on the right clears, and the attachment is deleted.
4. Remove the mu-plugin. On the Cloudinary admin page, insert a new image, then insert the same image again.
   • Both inserts clear the import progress, without errors, and the second one doesn't add a second attachment.
5. Insert an image from the DAM in the block editor again, with and without an effect or crop applied. It imports as before: a local file is created, and the preview shows the transformation.

[utkarshcloudinary]

looks good, left some nits.

[utkarshcloudinary]

Nit (optional): error_log() runs on every failed import, even when WP_DEBUG is off. Utils::log() above already writes the message to the System Report. Consider calling error_log() only when WP_DEBUG_LOG is on, so production PHP logs stay clean.

[gabrielcld2]

Thanks! I'd rather keep it unconditional: one of the issues this fixes is that failures were logged nowhere.
Utils::log() only stores the message while the System Report is enabled, which is off by default, so a production site would otherwise have no trace of failed imports.

It's one line per failed import, which should be rare, and with WP_DEBUG_LOG off it goes to the server's PHP error log, where we'd want to look.

[utkarshcloudinary]

Nit (optional, follow-up): This REST callback still answers with wp_send_json_error() (status 200) and not with a REST error. This was already the case, and the comment explains why (media-modal.js expects the admin-ajax format), so it's fine for this PR. It would be good to move to WP_Error responses with a proper status code later, and update both JS callers.

[gabrielcld2]

Agreed, we'll do it in a follow-up: return a WP_Error with a proper status from the endpoint and update both JS callers. Core already flags wp_send_json() during REST requests as incorrect usage since WP 5.5 (it only adds an X-WP-DoingItWrong header for now), so it's worth doing. Leaving it out of this PR, but the follow-up ticket we already have planned can potentially handle this.

[utkarshcloudinary]

Nit (optional): When a user inserts several assets and they all fail, this shows one window.alert() for each asset. One combined message would be easier for the user.

[gabrielcld2]

Good point. The media modal also shows one alert per asset, so I'd rather change both together in the same follow-up as the REST error responses, to keep them consistent.