Make WordPress Core

Opened 5 years ago

Closed 3 months ago

Last modified 3 months ago

#48244 closed defect (bug) (fixed)

script-loader.php Need to use _n() when more than one results are found

Reported by: tobifjellner's profile tobifjellner Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.5 Priority: normal
Severity: normal Version: 4.5
Component: Script Loader Keywords: has-patch commit
Focuses: javascript Cc:

Description

Location:
https://build.trac.wordpress.org/browser/trunk/wp-includes/script-loader.php?marks=1178#L1178

Content:

'manyResults'  => __( '%d results found. Use up and down arrow keys to navigate.' ),

should (if possible) be converted to use _n() in order for some languages to be able to correctly construct the phrase.

Attachments (10)

48244.patch (905 bytes) - added by shaharia.azam 4 years ago.
_n() has been applied in script-loader.php when more than one results are found
48244.2.patch (742 bytes) - added by shaharia.azam 4 years ago.
String is returning correctly. Removed .replace() from plugin.js
48244.3.patch (651 bytes) - added by shaharia.azam 4 years ago.
String is returning correctly. Removed .replace() from tags-suggest.js
48244.4.patch (1.8 KB) - added by shaharia.azam 4 years ago.
It's for 4.5 . Both plugin.js and script-loader.php has been updated
48244.5.diff (2.1 KB) - added by shaampk1 4 years ago.
Merged plugin.js, tags-suggest.js, and script-loader.php patches into one .diff file
48244.5.patch (1.6 KB) - added by shaharia.azam 4 years ago.
As per @swissspidy on slack, _n implemented on plugin.js & tags-suggest.js
48244.2021091100.patch (4.0 KB) - added by rsiddharth 3 years ago.
Initial version (2021091100).
48244.2021091300.patch (5.9 KB) - added by rsiddharth 3 years ago.
Patch version 2021091300 - addresses issues pointed out by @swissspidy
48244.2021091500.patch (6.0 KB) - added by rsiddharth 3 years ago.
Patch version 2021091500 - addresses issues pointed out by @swissspidy
48244.2022022000.patch (6.0 KB) - added by rsiddharth 2 years ago.
Regenerate patch

Download all attachments as: .zip

Change History (59)

#1 @swissspidy
5 years ago

The correct number is only known client-side, so for correct results one would need to use _n() from the @wordpress/i18n JS package.

This would need to happen in at least two places:

https://github.com/WordPress/wordpress-develop/blob/3b14a06c8cb187d8abd3d4af8ea9c0ea3a475228/src/js/_enqueues/vendor/tinymce/plugins/wplink/plugin.js#L459-L469
https://github.com/WordPress/wordpress-develop/blob/715a65c56142262b938d6860f840e201b1fe074a/src/js/_enqueues/admin/tags-suggest.js#L135-L144

However, this is by far not the only place where plural forms in JS could or should be improved. In #20491 you can find a huge patch with possible changes.

#2 @SergeyBiryukov
5 years ago

  • Version changed from trunk to 4.5

Introduced in [36806].

#3 @shaampk1
5 years ago

Hey folks,

This ticket really got my attention. I found it an interesting issue to work with. I know there has been already said a lot about L10n which I have been reading and trying to catch up with for the past few days now but still, I am not sure how far I have come regarding this issue. I'd appreciate it if you could review the following code snippet and let me know what I am missing.

For script-loader.php

	// Strings for 'jquery-ui-autocomplete' live region messages
	did_action( 'init' ) && $scripts->localize(
		'jquery-ui-autocomplete',
		'uiAutocompleteL10n',
		array(
			'noResults'    => __( 'No results found.' ),
			/* translators: %s: Number of results found when using jQuery UI Autocomplete. */
			'manyResults' => _n( '%s result found. Use up and down arrow keys to navigate.', '%s results found. Use up and down arrow keys to navigate.', did_action( 'init' ) ),
			'itemSelected' => __( 'Item selected.' ),
		)
	);

For plugin.js & tags-suggest.js

messages: {
	noResults: ( typeof window.uiAutocompleteL10n !== 'undefined' ) ? window.uiAutocompleteL10n.noResults : '',
	results: function( number ) {
		if ( typeof window.uiAutocompleteL10n !== 'undefined' ) {
			if ( number > 1 ) {
				return window.uiAutocompleteL10n.manyResults.replace( '%s', number );
			}

			return window.uiAutocompleteL10n.oneResult;
		}
	}
}

Thank you!

Last edited 4 years ago by shaampk1 (previous) (diff)

@shaharia.azam
4 years ago

_n() has been applied in script-loader.php when more than one results are found

@shaharia.azam
4 years ago

String is returning correctly. Removed .replace() from plugin.js

@shaharia.azam
4 years ago

String is returning correctly. Removed .replace() from tags-suggest.js

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


4 years ago

@shaharia.azam
4 years ago

It's for 4.5 . Both plugin.js and script-loader.php has been updated

@shaampk1
4 years ago

Merged plugin.js, tags-suggest.js, and script-loader.php patches into one .diff file

@shaharia.azam
4 years ago

As per @swissspidy on slack, _n implemented on plugin.js & tags-suggest.js

#5 @rebasaurus
4 years ago

  • Keywords has-patch added; needs-patch removed

#6 @swissspidy
4 years ago

  • Keywords needs-patch added; has-patch removed

Please refer to the documentation on how to properly use these functions in JS: https://github.com/WordPress/gutenberg/tree/6942c1df30250d5f27e36c2938b7905acf79644c/packages/i18n#usage

We'd need to make sure that these scripts will have wp-i18n as a dependency as well.

Also: please test your patches.

#7 @johnbillion
4 years ago

  • Component changed from Script Loader to Editor
  • Focuses javascript added

@rsiddharth
3 years ago

Initial version (2021091100).

#8 @rsiddharth
3 years ago

  • Keywords has-patch added; needs-patch removed

@swissspidy @SergeyBiryukov uploaded initial version of the patch.

#9 @swissspidy
3 years ago

Thanks for the patch. Two things I noticed at first glance:

  • uiAutocompleteL10n definition needs to be removed in script-loader.php
  • window.uiAutocompleteL10n needs to be deprecated using deprecateL10nObject

See r48925 for an example of how that deprecation works.

@rsiddharth
3 years ago

Patch version 2021091300 - addresses issues pointed out by @swissspidy 

#10 @rsiddharth
3 years ago

@swissspidy, Thank you for reviewing the patch.

I've:

  • Removed uiAutocompleteL10n from script-loader.php
  • Deprecated window.uiAutocompleteL10n using deprecateL10nObject

Questions:

  • Is script-loader.php:1099 (in the patch) the right place to add wp-i18n?
    I see the wplink plugin used in wp_tinymce_inline_scripts. Wondering if
    wp-i18n needs to be loaded there somehow?

#11 @swissspidy
3 years ago

Nice! We're getting there!

With the deprecation being done in common.js, common should be added as a dependency to both scripts.

No need to add wp-i18n as a dependency.

Instead, use $scripts->set_translations( 'wplink' );, which will do this automatically. tags-suggest already has this call, so all good there.

Nice find with r36806! Since that changeset is referencing a ticket with milestone 4.5.0, that would be the correct version for the @since annotation.

#12 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.9

@rsiddharth
3 years ago

Patch version 2021091500 - addresses issues pointed out by @swissspidy

#13 @rsiddharth
3 years ago

@swissspidy, uploaded new version of the patch with the following changes:

  • Added common as a dependency for wplink and tags-suggest.
  • Used $scripts->set_translations( 'wplink' ); for loading wp-i18n for wplink.
  • Changed @since for window.uiAutocompleteL10n to 4.5.0.

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


3 years ago

#15 @hellofromTonya
2 years ago

  • Milestone changed from 5.9 to 6.0

With 5.9 Beta 1 is happening in less than 4 hours, moving this ticket to 6.0.

@rsiddharth
2 years ago

Regenerate patch

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


2 years ago

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


2 years ago

This ticket was mentioned in PR #2618 on WordPress/wordpress-develop by peterwilsoncc.


2 years ago
#18

https://core.trac.wordpress.org/ticket/48244

Existing patch, some cs fixes.

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


2 years ago

#20 @costdev
2 years ago

  • Milestone changed from 6.0 to 6.1

As we are past soft-string freeze and now approaching RC1, I'm moving this ticket to the 6.1 milestone for remaining work to be carried out in the next cycle.

#21 @audrasjb
22 months ago

I refreshed the existing PR and updated the @since mentions.
Tests are still passing.

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


21 months ago

#23 @chaion07
21 months ago

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

@tobifjellner thank you for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received we are updating the ticket with the following changes:

  1. Add keywords needs-testing since it needs to be tested. Even though JB mentioned in his most recent comment that the tests are still passing.
  2. Removing keywords good-first-bug since the ticket has multiple patches

Cheers!

Props to @audrasjb & @hilayt24

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


20 months ago

#25 @chaion07
20 months ago

We discussed this ticket during another recent bug-scrub session and would love for a response from JB on this ticket. Thanks!

Props to @mukesh27

#26 @audrasjb
20 months ago

  • Milestone changed from 6.1 to 6.2

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone as the proposed patch still needs testing.

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


15 months ago

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


15 months ago

#29 @SergeyBiryukov
15 months ago

  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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


15 months ago

This ticket was mentioned in Slack in #core-test by robinwpdeveloper. View the logs.


15 months ago

#32 @costdev
15 months ago

  • Milestone changed from 6.2 to 6.3

With 6.2 Beta 4 now released, this ticket hasn't had any commits this cycle and still needs testing. Moving this to 6.3.

Last edited 15 months ago by costdev (previous) (diff)

#33 @oglekler
11 months ago

  • Keywords changes-requested needs-testing-info added

@peterwilsoncc (or anyone who can squeeze it into own to-do list) I was unable to apply the patch, can you please look at it and update, and let's try to make it into 6.3 for not changing milestone continuously.

@tobifjellner it would be nice to provide testing instructions or/and screenshots.

Thank you all 🙏

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


10 months ago

#35 @audrasjb
10 months ago

  • Milestone changed from 6.3 to 6.4

As per today's bug scrub, let's move this to the next milestone as we are at the end of WP 6.3 beta cycle.

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


9 months ago

#37 @marybaum
9 months ago

@tobifjellner is going to talk to @SergeyBiryukov about doing some testing. Thanks, y'all!

#38 @nicolefurlan
8 months ago

@tobifjellner and @SergeyBiryukov is there any update on testing for this ticket?

#39 @tobifjellner
8 months ago

Testing is tricky, since it would need to be tested by running JavaScript, having a test language with test translation configured.

One simple way to solve this would be to change the string in line 937

'manyResults' => ( '%d results found. Use up and down arrow keys to navigate.' ),

To something like:
Number of results found: %d. Use up and down...

Last edited 8 months ago by tobifjellner (previous) (diff)

#40 @oglekler
7 months ago

  • Keywords needs-refresh added; changes-requested removed

@peterwilsoncc can you please rebase the patch?

#41 @peterwilsoncc
7 months ago

  • Keywords needs-refresh removed

@oglekler Done: I merged in trunk and updated the deprecation version.

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


7 months ago

#43 @oglekler
7 months ago

@tobifjellner we can run JS, please, just tell us where to look. It is difficult to understand where this phrase should appear. To have a screenshot will be good.

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


7 months ago

#45 @nicolefurlan
7 months ago

  • Milestone changed from 6.4 to 6.5

This ticket was discussed in today's bug scrub.

Since this ticket still needs testing info and testing, we're bumping it to 6.5.

@tobifjellner could you check out #comment:43 and provide some testing info?

#46 @swissspidy
3 months ago

  • Keywords commit added; needs-testing needs-testing-info removed

The patch at https://github.com/WordPress/wordpress-develop/pull/2618 looks good to me, marking as ready for commit.

#47 @peterwilsoncc
3 months ago

  • Component changed from Editor to Script Loader

Moving to the script loader component as it covers a few JS files

#48 @peterwilsoncc
3 months ago

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

In 57654:

Script Loader: Switch to JavaScript translations.

Update JavaScript files for tag suggestions and the TinyMCE link plugin to use client side translations. This allows for _n() to be used for strings requiring singular and plural versions in which the correct form is only known client side.

Props audrasjb, chaion07, costdev, hellofromtonya, johnbillion, marybaum, nicolefurlan, oglekler, rebasaurus, rsiddharth, sergeybiryukov, shaampk1, shahariaazam, swissspidy, tobifjellner.
Fixes #48244.

Note: See TracTickets for help on using tickets.