WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#41609 closed defect (bug) (fixed)

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

Reported by: melchoyce Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.8
Component: Customize Keywords: has-patch commit
Focuses: Cc:

Description

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.

Attachments (1)

41609.0.diff (1.3 KB) - added by westonruter 3 months ago.

Download all attachments as: .zip

Change History (9)

#1 @westonruter
3 months ago

  • Version set to 4.8

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
3 months ago

#2 @westonruter
3 months ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to 4.8.2

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.

#3 @westonruter
3 months ago

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.

#4 @westonruter
3 months ago

  • Keywords commit added; needs-testing removed
  • Milestone changed from 4.8.2 to 4.9
  • Owner set to westonruter
  • Status changed from new to accepted

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

#5 @westonruter
3 months ago

  • Summary changed from Images in Customizer not deleted until Customizer is saved? to Attachment deletion in media modal silently fails when opened for Media widgets

#6 @westonruter
3 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 41248:

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

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

#7 @melchoyce
3 months ago

  • Milestone changed from 4.9 to 4.8.2

Thanks @westonruter!

#8 @melchoyce
3 months ago

  • Milestone changed from 4.8.2 to 4.9

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

Note: See TracTickets for help on using tickets.