Make WordPress Core

Opened 4 years ago

Last modified 3 months ago

#48244 reviewing defect (bug)

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.3 Priority: normal
Severity: normal Version: 4.5
Component: Editor Keywords: has-patch needs-testing
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 21 months ago.
Initial version (2021091100).
48244.2021091300.patch (5.9 KB) - added by rsiddharth 21 months ago.
Patch version 2021091300 - addresses issues pointed out by @swissspidy
48244.2021091500.patch (6.0 KB) - added by rsiddharth 21 months ago.
Patch version 2021091500 - addresses issues pointed out by @swissspidy
48244.2022022000.patch (6.0 KB) - added by rsiddharth 16 months ago.
Regenerate patch

Download all attachments as: .zip

Change History (42)

#1 @swissspidy
4 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
4 years ago

  • Version changed from trunk to 4.5

Introduced in [36806].

#3 @shaampk1
4 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
3 years ago

  • Keywords has-patch added; needs-patch removed

#6 @swissspidy
3 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
3 years ago

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

@rsiddharth
21 months ago

Initial version (2021091100).

#8 @rsiddharth
21 months ago

  • Keywords has-patch added; needs-patch removed

@swissspidy @SergeyBiryukov uploaded initial version of the patch.

#9 @swissspidy
21 months 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
21 months ago

Patch version 2021091300 - addresses issues pointed out by @swissspidy 

#10 @rsiddharth
21 months 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
21 months 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
21 months ago

  • Milestone changed from Awaiting Review to 5.9

@rsiddharth
21 months ago

Patch version 2021091500 - addresses issues pointed out by @swissspidy

#13 @rsiddharth
21 months 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.


20 months ago

#15 @hellofromTonya
19 months 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
16 months ago

Regenerate patch

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


16 months ago

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


14 months ago

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


14 months 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.


14 months ago

#20 @costdev
14 months 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
11 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.


9 months ago

#23 @chaion07
9 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.


9 months ago

#25 @chaion07
9 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
8 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.


4 months ago

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


4 months ago

#29 @SergeyBiryukov
4 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.


3 months ago

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


3 months ago

#32 @costdev
3 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 3 months ago by costdev (previous) (diff)
Note: See TracTickets for help on using tickets.