Make WordPress Core

Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#38918 closed enhancement (fixed)

delete_plugins() needs a proper singular and plural form for its error message

Reported by: johnbillion's profile johnbillion Owned by: johnbillion's profile johnbillion
Milestone: 4.9 Priority: normal
Severity: normal Version: 2.6
Component: Plugins Keywords: has-patch dev-feedback
Focuses: Cc:

Description

The delete_plugins() function uses the following string as an error message:

Could not fully remove the plugin(s) %s.

This would be better if it used _n() and provided a proper singular and plural form based on the number of errors.

Attachments (4)

38918.diff (625 bytes) - added by eddhurst 7 years ago.
Removing plural for translation string and adding colon to make clearer that plugin name follows
38918-forced-test.diff (1.1 KB) - added by eddhurst 7 years ago.
Forced output of plugin delete failure message (even when successfully deleted) for easier testing. Not for production.
38918.2.diff (848 bytes) - added by johnbillion 6 years ago.
38918.3.diff (904 bytes) - added by SergeyBiryukov 6 years ago.

Download all attachments as: .zip

Change History (18)

#1 @johnbillion
7 years ago

  • Focuses administration removed

#2 @eddhurst
7 years ago

I tried swapping this to use _n() and then realised that the plugins are removed inline, so each error message is shown next to the plugin, rather than in a list at the top.

As a result, should the translation case instead be to remove the (s) plural altogether and just retain this as a pure __() singular translation string?

To illustrate: Each error will only ever appear once next to each plugin that fails to be deleted, so (should) output:

Could not fully remove the plugin hello-dolly/hello.php.

Rather than what would currently show:

Could not fully remove the plugin(s) hello-dolly/hello.php.

@eddhurst
7 years ago

Removing plural for translation string and adding colon to make clearer that plugin name follows

#3 @eddhurst
7 years ago

  • Keywords has-patch 2nd-opinion added; needs-patch removed

#4 @eddhurst
7 years ago

  • Keywords 2nd-opinion removed

Tested a couple of older versions to check against what they were doing and in the eventuality that multiple plugins failed to delete, only the last message would show anyway (deleted 3 plugins, but was shown the following error):

Plugin could not be deleted due to an error: Could not fully remove the plugin(s) rest-api/plugin.php.

Given that all these errors have been brought in-line anyway now, each error will show next to the relevant plugin that's failed (if multiple), so a singular phrase seems fine.

I've added an extra patch to force errors on successfully removing a plugin, just to make testing the output a little easier - the initial patch is the correct one to apply going forward.

Last edited 7 years ago by eddhurst (previous) (diff)

@eddhurst
7 years ago

Forced output of plugin delete failure message (even when successfully deleted) for easier testing. Not for production.

#5 @DrewAPicture
7 years ago

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

Assigning ownership to mark the good-first-bug as "claimed".

#6 @johnbillion
7 years ago

  • Keywords needs-testing added; good-first-bug removed

#7 @henry.wright
7 years ago

I tested 38918.diff at WordCamp Brighton Contributor Day. All looked good!

#8 @johnbillion
7 years ago

  • Owner changed from eddhurst to johnbillion
  • Status changed from assigned to reviewing

#9 @johnbillion
7 years ago

  • Milestone changed from Awaiting Review to 4.9

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 years ago

@johnbillion
6 years ago

#12 @johnbillion
6 years ago

  • Keywords dev-feedback added; needs-testing removed

@eddhurst Sorry for the big delay in responding. Your observations appear to be correct at first, but it's still technically possible for the plugin deletion error message to contain multiple plugin filenames. You can see this by disabling JavaScript in your browser and performing a bulk plugin deletion (which you can force to fail by no-oping the $wp_filesystem->delete calls).

38918.2.diff is the most technically correct patch, but it introduces an %l placeholder into a translation string due to it being passed into wp_sprintf() for the correct comma separated list terminology. I'm not sure if we have this currently in any translation string. Pinging @ocean90 for a sanity check.

#13 @SergeyBiryukov
6 years ago

I think it's exactly one of the cases where we should check for 1 === count( $errors ) specifically instead of using _n(), to avoid displaying the singular string for 21, 31, etc. in some languages.

Using %l and wp_sprintf() seems fine, but we only use it in two places in core: in get_the_taxonomies() and on Credits page. The goal there is to display a "smart" list with an Oxford comma and such: %s, %s, and %s for more than 2 items or %s and %s for only 2 items.

As noted above, it's rare for plugin deletion error message to contain multiple filenames, so I think there's no need for this list to be "smart", a simple implode( ', ', $errors ) should be fine as well.

See 38918.3.diff.

Last edited 6 years ago by SergeyBiryukov (previous) (diff)

#14 @johnbillion
6 years ago

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

In 41713:

Plugins: Introduce a singular and plural form for the plugin deletion error message.

Props eddhurst, SergeyBiryukov

Fixes #38918

Note: See TracTickets for help on using tickets.