Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#55158 closed defect (bug) (fixed)

Attachment removed from grid view if the delete request is failed

Reported by: kapilpaul's profile kapilpaul Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.0 Priority: normal
Severity: normal Version: 6.0
Component: Media Keywords: has-patch needs-testing
Focuses: ui, javascript Cc:

Description

While working with media attachments got a bug. When try to delete a media attachment from modal by pressing Delete permanently button, if the ajax request response is failed the media attachment is removed from the list.

Steps to reproduce:

  1. Add pre_delete_attachment filter in your theme's functions.php to prevent the delete attachment operation.
add_filter( 'pre_delete_attachment', function ( $delete, $attachment ) {
    return false;
}, 10, 2 );
  1. Go to Media (grid mode) https://example.com/wp-admin/upload.php?mode=grid
  2. Click on any attachment. (The modal will open)
  3. Click on Delete permanently button
    • This will remove the attachment from grid list.
    • If you check the Ajax request the response is 0
  4. After done, reload the page. (The image is still there)

Expected Result:
If the media cannot be deleted or the response is failed, the Attachment should be there. It should not be removed.

Actual Result:
If the media cannot be deleted or the response is failed, the Attachment is getting removed from the list.

Change History (12)

This ticket was mentioned in PR #2315 on WordPress/wordpress-develop by kapilpaul.


3 years ago
#1

  • Keywords has-patch added

#2 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 6.0

#3 @mukesh27
3 years ago

  • Keywords needs-testing added

Hi there!

Thanks for the ticket and patch.

I tested the patch and it working fine without any error.

Let's add needs-testing so the testing team can help to test this issue in the next meeting.

#4 @azouamauriac
3 years ago

Hi thanks for the report and the patch two.

Patch works on my side two.

Introduced: [22869].

#5 @costdev
3 years ago

Env

  • Web Server: Apache
  • WordPress: 6.0-alpha-52448-src
  • Browser: Chrome
  • OS: Linux
  • Theme: Twenty Twenty-One
  • Plugins: None activated

Reproc Report

Steps to reproduce

  1. Add this filter to functions.php:
<?php
add_filter( 'pre_delete_attachment', '__return_false', 10, 2 );
  1. Navigate to Media > Library.
  2. Enable Grid mode.
  3. Click on a media item to open the modal.
  4. Click Delete permanently.
    • The media item will no longer show in the grid. ✅
    • If you check the Ajax request the response is 0. ✅
  5. Reload the page. The media item is still in the list. ✅

Results

  • Issue reproduced. ✅

Test Report

Steps to test

  1. Add this filter to functions.php:
<?php
add_filter( 'pre_delete_attachment', '__return_false', 10, 2 );
  1. Apply PR 2315.
  2. Run npm run build:dev.
  3. Navigate to Media > Library.
  4. Enable Grid mode.
  5. Click on a media item to open the modal.
  6. Click Delete permanently. Nothing will happen. ✅
  7. Close the modal. The media item is still in the list. ✅
  8. Reload the page. The media item is still in the list. ✅

Results


Comments

While the PR prevents the media item from being removed from the list, the end result is that Delete permanently, to a user, appears to do nothing. I think the PR would benefit from informing the user that the media item could not be deleted.

For example, this will create an alert when the media item cannot be deleted:

// src/wp-includes/media.php:4495

'couldNotDeleteMediaItem'     => __( 'The media item could not be deleted.' ),
// src/js/media/views/attachment/details.js:157

this.model.destroy( {
        wait: true,
        error: function() {
                alert( l10n.couldNotDeleteMediaItem );
        }
} );
Last edited 3 years ago by costdev (previous) (diff)

#6 @kapilpaul
3 years ago

@costdev thanks for testing this. I am also agree with your comment. We need to inform/display some message to end user. I will update the PR with the suggested.

#7 @kapilpaul
3 years ago

@costdev updated the PR.

#8 @mukesh27
3 years ago

The updated patch looks good to me.

Ping @SergeyBiryukov for final review.

#9 @SergeyBiryukov
3 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

SergeyBiryukov commented on PR #2315:


3 years ago
#10

Thanks for the PR! I think we can use an existing string here: Error in deleting the attachment, which we already use in a few other places. Looks good to me otherwise.

#11 @SergeyBiryukov
3 years ago

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

In 52725:

Media: Display an error message in grid view if the attachment could not be deleted.

Previously, the attachment was silently removed from the grid but reappeared after a page reload.

Follow-up to [22869].

Props kapilpaul, costdev, mukesh27, azouamauriac, SergeyBiryukov.
Fixes #55158.

Note: See TracTickets for help on using tickets.