Attachment deletion in media modal silently fails when opened for Media widgets

[melchoyce]

I was testing out the image widget, and uploaded a new image to my library after clicking “Add image” in the widget. I decided I didn’t want that image anymore, so I deleted it and then closed the media library. Later, I noticed the because I hadn’t saved either the widget, or my Customizer session, the image hadn’t actually been deleted (despite confirming yes, I want to delete it permanently).

I was surprised, since I expect when I confirm that I want to delete it permanently, I'm expecting it to be deleted then.

[westonruter]

I can confirm this behavior. It's due to this bit of logic: ​https://github.com/WordPress/wordpress-develop/blob/4.8.0/src/wp-admin/js/widgets/media-widgets.js#L688-L692

// Disable syncing of attachment changes back to server. See <https://core.trac.wordpress.org/ticket/40403>.
defaultSync = wp.media.model.Attachment.prototype.sync;
wp.media.model.Attachment.prototype.sync = function rejectedSync() {
        return $.Deferred().rejectWith( this ).promise();
};

Nevertheless, this was implemented specifically to prevent syncing of edits to attachment fields, rather than deletions to those attachments entirely. Really if we were to disable deletions, then the “Delete Permanently” button should be removed entirely. Or rather in keeping with #40403 it could be preferable to trash such an attachment to then get permanently deleted when the changeset is published.

[westonruter]

41609.0.diff​ adds an exception for allow deletions of attachments to pass through.

Keeping changes to media inside of the changeset is a larger discussion for #40403.

[westonruter]

Without 41609.0.diff​ you'll see that upon clicking to delete an attachment there is no Ajax request to actually delete it once you click the confirmation dialog. The delete button is a no-op.

[westonruter]

Doesn't seem like something we need to rush for 4.8.x.

[westonruter]

In 41248:

Customize: Prevent attachment deletions from silently failing in media modals opened for Media widgets.

Amends [40640].
See #32417.
Fixes #41609.

[melchoyce]

Thanks @westonruter!

[melchoyce]

Ack, sorry, unsure why that milestone change happened — maybe I hadn't reloaded and it kept the previous milestone. :/ Fixing.