WordPress.org

Make WordPress Core

Opened 9 years ago

Last modified 5 months ago

#16165 assigned enhancement

Media Library Bulk Delete: Error in deleting...

Reported by: hakre Owned by: nacin
Milestone: Priority: normal
Severity: normal Version: 3.1
Component: Media Keywords: has-patch dev-feedback
Focuses: administration Cc:
PR Number:

Description

While Bulk Deletion, when a user gets the "Error in deleting..." message, there is no information given of how many elements have been deleted so far.

Let's say there was a bulk of N deletions, getting this error can mean up to N-1 items have been deleted already.

Same is the case if for some item, no permissions are granted to delete it. The number of successfully deleted items is missing as well.

Attachments (3)

16165.diff (3.0 KB) - added by solarissmoke 9 years ago.
16165_r21080.patch (3.0 KB) - added by bpetty 7 years ago.
16165-r21374.patch (4.1 KB) - added by bpetty 7 years ago.

Download all attachments as: .zip

Change History (17)

#1 @ocean90
9 years ago

  • Component changed from General to Media
  • Type changed from defect (bug) to enhancement

#2 @nacin
9 years ago

  • Summary changed from Media Library Bulk Delete: Error in deleting... / You are not allowed to delete this post. is not given any information about the amount of successfully deleted items. to Media Library Bulk Delete: Error in deleting...

#3 @westi
9 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Priority changed from normal to low

I guess this probably needs applying to all bulk actions.

Moving to Future for now - no Patch yet.

#4 @solarissmoke
9 years ago

  • Keywords has-patch dev-feedback added

This patch reports exactly which items failed to trash/untrash/delete for bulk edits.

@solarissmoke
9 years ago

@bpetty
7 years ago

#5 @bpetty
7 years ago

  • Cc bpetty added

I have attached an updated patch against latest SVN trunk. Besides updating it though, I also made 3 fixes to the old patch.

  1. Added esc_html filter for titles in error messages (required for security).
  2. Fixed usage of WP_Error so it doesn't add a blank error first incorrectly (when there is at least one error).
  3. The get_post() check actually needed to be negated in order to properly skip over items that were in fact deleted already, and also so that bulk item deletion actually worked properly (the original patch made it impossible to delete items using bulk deletion).

This patch, like the original, is a fix for both #16165 as well as #16164.

#6 @nacin
7 years ago

Related w/r/t better error messages, #19826.

#7 @nacin
7 years ago

  • Milestone changed from Future Release to 3.5

Looks good. If you try to delete a few hundred posts and they all fail for some reason, is it necessary to error that every one of them couldn't be deleted? At some point should we just show a number, if count( $errors ) is too high, or if there were no successes at all?

@bpetty
7 years ago

#8 @bpetty
7 years ago

  • Owner set to nacin
  • Status changed from new to assigned

New patch added that only shows the count of the number of items that failed if it's greater than 10 items. Also fixed some spacing to adhere to coding guidelines.

#9 follow-up: @scribu
7 years ago

  • Milestone changed from 3.5 to Future Release

From the patch:

// Escape item titles displayed in error messages. 
add_filter( 'the_title', 'esc_html' );

Why is that needed?

And I agree with Westi that we should handle this for all list screens. Punting.

#10 @scribu
7 years ago

  • Component changed from Media to Administration

#11 in reply to: ↑ 9 @bpetty
7 years ago

Replying to scribu:

From the patch:

// Escape item titles displayed in error messages. 
add_filter( 'the_title', 'esc_html' );

Why is that needed?

These are error messages, not post previews, and the wp_die() page they end up on don't even have the same styles set for either the current theme or wp-admin which would result in displaying them in yet a third, completely different way then the user is expecting to see them. Here, we're only concerned about the user identifying the corresponding post that failed, not display it.

That, and it's one less place to worry about CSRF if it were ever a problem in post titles (not saying the post edit pages don't already trust titles with markup, but limiting the locations this is exposed is still ideal).

#12 @nacin
6 years ago

#16164 was marked as a duplicate.

#13 @nacin
6 years ago

  • Component changed from Administration to Media
  • Focuses administration added

#14 @chriscct7
4 years ago

  • Priority changed from low to normal
Note: See TracTickets for help on using tickets.