#38918 closed enhancement (fixed)
delete_plugins() needs a proper singular and plural form for its error message
Reported by: | johnbillion | Owned by: | 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)
Change History (18)
@
8 years ago
Removing plural for translation string and adding colon to make clearer that plugin name follows
#4
@
8 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.
@
8 years ago
Forced output of plugin delete failure message (even when successfully deleted) for easier testing. Not for production.
#5
@
8 years ago
- Owner set to eddhurst
- Status changed from new to assigned
Assigning ownership to mark the good-first-bug
as "claimed".
#8
@
7 years ago
- Owner changed from eddhurst to johnbillion
- Status changed from assigned to reviewing
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 years ago
This ticket was mentioned in Slack in #core by jeffpaul. View the logs.
7 years ago
#12
@
7 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
@
7 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.
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.