WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#15920 closed defect (bug) (fixed)

List table screens in network admin break if no item is selected for bulk actions

Reported by: PeteMall Owned by: PeteMall
Milestone: 3.1 Priority: normal
Severity: normal Version: 3.1
Component: General Keywords: has-patch commit dev-reviewed
Focuses: Cc:

Description

All list table screens in net admin other than plugins and site-themes break when you select a bulk action and don't select any items.

Attachments (8)

15920.patch (2.7 KB) - added by SergeyBiryukov 9 years ago.
15920.2.patch (1.5 KB) - added by SergeyBiryukov 9 years ago.
15920-take2.patch (2.8 KB) - added by SergeyBiryukov 9 years ago.
15920-take3.patch (4.6 KB) - added by SergeyBiryukov 9 years ago.
15920.diff (7.4 KB) - added by nacin 9 years ago.
15920.2.diff (7.5 KB) - added by nacin 9 years ago.
Patch includes better REQUEST_URI reset.
15920.3.diff (3.8 KB) - added by PeteMall 9 years ago.
Better messages for site-themes.
15920.4.diff (1.4 KB) - added by PeteMall 9 years ago.

Download all attachments as: .zip

Change History (34)

#1 @SergeyBiryukov
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

#2 @SergeyBiryukov
9 years ago

Fixed notices on network/sites.php and network/users.php.

#3 @ryan
9 years ago

Patch needs to be refreshed to handle the recently added delete-selected case.

#4 @SergeyBiryukov
9 years ago

Revised the patch.

#5 @scribu
9 years ago

The messages don't distinguish between individual actions and bulk actions, i.e., if I delete two themes, I still get "Theme deleted".

#6 @nacin
9 years ago

We should pass the number of themes affected back and use _n().

This can either be a new query arg (n=1, n=5), or just enabled=1 or enabled=5, versus the generic update.

#7 @PeteMall
9 years ago

  • Owner set to PeteMall
  • Status changed from new to accepted

#8 @nacin
9 years ago

(In [17128]) Prevent notices in the allblogs and allusers branches. props SergeyBiryukov, see #15920.

#9 @SergeyBiryukov
9 years ago

Added distinguishing between individual actions and bulk actions.

#10 @johnpbloch
9 years ago

Just tested against trunk. Everything checks out and works as expected (patch 15920-take3.patch).

@nacin
9 years ago

#11 follow-up: @nacin
9 years ago

New patch:

  • Removes apparently bogus esc_html() wrappers around $theme['Stylesheet'] in WP_MS_Themes_List_Table. This is the directory name so it should be pretty safe already, and every usage of it ends up in esc_url() or esc_attr(). If for some reason esc_html( $theme_key ) != $theme_key though, this would pose quite a number of problems, as the database would be improperly updated.
  • Hides the 'Delete' link for a theme active on the main site.
  • Prevents theme deletion for a theme active on the main site.
  • Properly resets the request URI arguments.
  • Switches to more direct enabled=N (etc) messages. Also doesn't show "1" in the string when only one theme is deleted.

I'm still not a fan of this direct-to-DB logic, but I'll let ryan weigh in on the necessity of reworking that into a saner check.

			$allowed_themes[ $_GET['theme'] ] = true;
			update_site_option( 'allowedthemes', $allowed_themes );

#12 in reply to: ↑ 11 ; follow-up: @SergeyBiryukov
9 years ago

Replying to nacin:

Also doesn't show "1" in the string when only one theme is deleted.

There's actually a slight i18n issue. It's good for English, however in Russian a single form is used for every number ending in 1 (except for 11): 1, 21, 31, etc. Standalone single form (as in “Theme enabled”) only makes sense for 1 item. In other cases (21, 31, ...) the number should be included in the string (as in “%s theme enabled”), but it's still single form. Currently there's no way to do this properly.

So my idea was to use “Theme enabled” for individual actions and “%s theme enabled” for bulk actions if only one item has been selected.

This is probably should go in another ticket, since there are some other messages concerned (e.g. “Item permanently deleted”). I'll create one if I get around to it.

#13 in reply to: ↑ 12 @nacin
9 years ago

Replying to SergeyBiryukov:

This is probably should go in another ticket, since there are some other messages concerned (e.g. “Item permanently deleted”). I'll create one if I get around to it.

Indeed, not a regression, so should be handled elsewhere.

#14 follow-up: @nacin
9 years ago

According to the gnu gettext manual, that should be handled by a proper Plural-Forms header in your PO file. Thus, 'Item deleted' or 'One item deleted' is fine, as it handles the singular case, and you are then given as many plural forms as you need based on the logic in the Plural-Forms header. Not my area of expertise, but that makes sense to me.

#15 @nacin
9 years ago

site-themes.php might need some of the logic in this patch, based on a quick glance.

See also #15974 -- it needs some nonce checks too.

#16 @nacin
9 years ago

Looks like the REQUEST_URI bit might be needed for non-redirect reasons.

#17 in reply to: ↑ 14 @SergeyBiryukov
9 years ago

Replying to nacin:

According to the gnu gettext manual, that should be handled by a proper Plural-Forms header in your PO file. Thus, 'Item deleted' or 'One item deleted' is fine, as it handles the singular case, and you are then given as many plural forms as you need based on the logic in the Plural-Forms header.

I've been using the header from that same manual:

Plural-Forms: nplurals=3; plural=(n%10==1 && n%100!=11) ? 0 : ((n%10>=2 && n%10<=4 && (n%100<10 || n%100>=20)) ? 1 : 2);

Perhaps it can be modified to handle this. Thanks for the hint!

#18 follow-up: @nacin
9 years ago

I don't know enough to be particularly helpful here, but http://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html specifically talks about the optional use of 'One' (no %d) in a singular form, and how that's perfectly fine for languages with advanced plural forms.

@nacin
9 years ago

Patch includes better REQUEST_URI reset.

#19 @ryan
9 years ago

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

(In [17130]) MS themes fixes. Fix bulk actions when no items selected. Props SergeyBiryukov, nacin. fixes #15920

@PeteMall
9 years ago

Better messages for site-themes.

#20 @PeteMall
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#21 @nacin
9 years ago

  • Keywords commit added; needs-testing removed

#22 in reply to: ↑ 18 @SergeyBiryukov
9 years ago

Replying to nacin:

I don't know enough to be particularly helpful here, but http://www.gnu.org/software/gettext/manual/html_node/Plural-forms.html specifically talks about the optional use of 'One' (no %d) in a singular form, and how that's perfectly fine for languages with advanced plural forms.

I was able to sort it out by adding an extra rule for a single item and updating translations. It's great knowing that there's a simple solution if you look at the right place. Thanks again!

#23 @westi
9 years ago

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

(In [17137]) Improved messaging for theme enabled/disabled on a per-site bases. Fixes #15920 props PeteMall

#24 @westi
9 years ago

  • Keywords dev-reviewed added

Looked great to me.

#25 @PeteMall
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@PeteMall
9 years ago

#26 @nacin
9 years ago

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

(In [17138]) Remove redundancy in these arguments. props PeteMall, fixes #15920.

Note: See TracTickets for help on using tickets.