WordPress.org

Make WordPress Core

Opened 10 years ago

Closed 10 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 10 years ago.
15920.2.patch (1.5 KB) - added by SergeyBiryukov 10 years ago.
15920-take2.patch (2.8 KB) - added by SergeyBiryukov 10 years ago.
15920-take3.patch (4.6 KB) - added by SergeyBiryukov 10 years ago.
15920.diff (7.4 KB) - added by nacin 10 years ago.
15920.2.diff (7.5 KB) - added by nacin 10 years ago.
Patch includes better REQUEST_URI reset.
15920.3.diff (3.8 KB) - added by PeteMall 10 years ago.
Better messages for site-themes.
15920.4.diff (1.4 KB) - added by PeteMall 10 years ago.

Download all attachments as: .zip

Change History (34)

#1 @SergeyBiryukov
10 years ago

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

#2 @SergeyBiryukov
10 years ago

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

#3 @ryan
10 years ago

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

#4 @SergeyBiryukov
10 years ago

Revised the patch.

#5 @scribu
10 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
10 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
10 years ago

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

#8 @nacin
10 years ago

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

#9 @SergeyBiryukov
10 years ago

Added distinguishing between individual actions and bulk actions.

#10 @johnpbloch
10 years ago

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

@nacin
10 years ago

#11 follow-up: @nacin
10 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
10 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
10 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
10 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
10 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
10 years ago

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

#17 in reply to: ↑ 14 @SergeyBiryukov
10 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
10 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
10 years ago

Patch includes better REQUEST_URI reset.

#19 @ryan
10 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
10 years ago

Better messages for site-themes.

#20 @PeteMall
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#21 @nacin
10 years ago

  • Keywords commit added; needs-testing removed

#22 in reply to: ↑ 18 @SergeyBiryukov
10 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
10 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
10 years ago

  • Keywords dev-reviewed added

Looked great to me.

#25 @PeteMall
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

@PeteMall
10 years ago

#26 @nacin
10 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.