Make WordPress Core

Opened 4 years ago

Last modified 10 months ago

#51142 new defect (bug)

Invalid Moment.js locale from get_user_locale()

Reported by: slightlyfaulty's profile slightlyfaulty Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.0
Component: External Libraries Keywords: has-patch needs-unit-tests
Focuses: javascript Cc:

Description (last modified by mukesh27)

get_user_locale() returns a locale in the format en_US, which is passed directly to Moment.js, e.g. moment.updateLocale( 'en_US', {...} ).

Since Moment.js doesn't actually support two-part locales, this causes some issues. For example, moment().format( 'Do' ) returns 25 where it should return 25th. This is because the local *ordinal* is lost as a result of setting an undefined locale.

The locale should either be passed in a format that Moment.js understands, such as en, or moment.defineLocale( 'en_US', { parentLocale: 'en' } ) should be called before moment.updateLocale( 'en_US', {...} ). AFAIK these are equivalent.

You can see the issue reproduced here: https://codepen.io/slightlyfaulty/pen/WNwpMmG

Of course, swapping out old dusty Moment.js for its super modern and popular drop-in replacement Day.js is also an option

Change History (30)

#1 @slightlyfaulty
4 years ago

It might be worth mentioning that this doesn't just affect usage of Moment.js itself, but also the @wordpress/date package.

For example, format( 'jS' ) returns 25 instead of the expected 25th.

#2 @SergeyBiryukov
4 years ago

  • Version changed from 5.5 to 5.0

Hi there, welcome to WordPress Trac! Thanks for the report.

Some history of the code in question:

  • Introduced in [44262] / #45145, changing the version to 5.0 as the earliest applicable version.
  • Adjusted in [48075] / #50408 to use moment.updateLocale() instead of moment.locale().

#3 @iandunn
4 years ago

Related:

#4 @noisysocks
2 years ago

moment.defineLocale( 'en_US', { parentLocale: 'en' } ) should be called before moment.updateLocale( 'en_US', {...} )

This makes sense to me!

This ticket was mentioned in PR #2456 on WordPress/wordpress-develop by noisysocks.


2 years ago
#5

  • Keywords has-patch added

Fixes a few misconfigurations in the Moment.js locale set up by the inline 'moment' script.

  • parentLocale will not work when using updateLocale(). Use defineLocale() instead.
  • doy must be provided when setting dow.
  • meridiem can be set using the info we have in WP_Locale.
  • LTS, L and LLLL must be provided when setting longDateFormat.

Some of the fixes here are borrowed from https://github.com/WordPress/gutenberg/pull/39670 which fixes the call to moment.updateLocale made by @wordpress/date.

An alternative to this PR is to remove the inline script from moment altogether. It was added in https://core.trac.wordpress.org/changeset/44262 without any note. I believe it was a mistake. It's not necessary since @wordpress/date will also call moment.updateLocale / moment.defineLocale and @wordpress/date is the only part of WP Admin that relies on moment localisation. It's possible that third party plugins now rely on this, though. What do you think @hellofromtonya @SergeyBiryukov?

Trac ticket: https://core.trac.wordpress.org/ticket/51142

#6 @noisysocks
2 years ago

  • Milestone changed from Awaiting Review to 6.0

I opened a PR which should fix this. Noting this comment here for extra visibility:

An alternative to this PR is to remove the inline script from moment altogether. It was added in https://core.trac.wordpress.org/changeset/44262 without any note. I believe it was a mistake. It's not necessary since @wordpress/date will also call moment.updateLocale / moment.defineLocale and @wordpress/date is the only part of WP Admin that relies on moment localisation. It's possible that third party plugins now rely on this, though. What do you think @hellofromTonya @SergeyBiryukov?

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


2 years ago

#8 @peterwilsoncc
2 years ago

  • Milestone changed from 6.0 to Future Release

This was discussed in a bug scrub earlier.

Given the question as to whether the code is necessary it was decided to hold this off for a later release.

yoancutillas commented on PR #2456:


21 months ago
#9

Hello, thank you for providing this fix.
Not sure why it was blocked though...

However, I recently tried your fix and found out that one more thing needs to be fixed:
In https://github.com/noisysocks/wordpress-develop/blob/fix/moment-locale/src/wp-includes/script-loader.php#L175-L180

'longDateFormat' => array(
  'LT'   => get_option( 'time_format', __( 'g:i a' ) ),
  'LTS'  => null,
  'L'    => null,
  'LL'   => get_option( 'date_format', __( 'F j, Y' ) ),
  'LLL'  => __( 'F j, Y g:i a' ),
  'LLLL' => null,
),

The PHP format needs to be converted to the Moment JS format:

'longDateFormat' => array(
  'LT'   => wp_convert_php_datetime_format_to_moment_js( get_option( 'time_format', __( 'g:i a' ) ) ),
  'LTS'  => null,
  'L'    => null,
  'LL'   => wp_convert_php_datetime_format_to_moment_js( get_option( 'date_format', __( 'F j, Y' ) ) ),
  'LLL'  => wp_convert_php_datetime_format_to_moment_js( __( 'F j, Y g:i a' ) ),
  'LLLL' => null,
),

Where the wp_convert_php_datetime_format_to_moment_js function would be:

/**
 * Convert a PHP datetime format to moment JS format
 * @param {string} php_format
 * @returns {string}
 */
function wp_convert_php_datetime_format_to_moment_js( $php_format ) {
	$format_map = array(
		'd' => 'DD',
		'D' => 'ddd',
		'j' => 'D',
		'l' => 'dddd',
		'N' => 'E',
		'w' => 'd',
		'W' => 'W',
		'F' => 'MMMM',
		'm' => 'MM',
		'M' => 'MMM',
		'n' => 'M',
		'o' => 'GGGG',
		'Y' => 'YYYY',
		'y' => 'YY',
		'a' => 'a',
		'A' => 'A',
		'g' => 'h',
		'G' => 'H',
		'h' => 'hh',
		'H' => 'HH',
		'i' => 'mm',
		's' => 'ss',
		'u' => 'X',
		'e' => 'z',
		'O' => 'ZZ',
		'P' => 'Z',
		'T' => 'z',
		'c' => 'YYYY-MM-DD[T]HH:mm:ssZ',
		'r' => 'ddd, DD MMM YYYY HH:mm:ss ZZ',
		'U' => 'X'
	);
	
	$has_backslash = false;
	$moment_js_format = '';
	foreach( str_split( $php_format ) as $char ) {
		if( $char === '\\' ) { $has_backslash = true; continue; }
		$moment_js_format .= $has_backslash || ! isset( $format_map[ $char ] ) ? '[' . $char . ']' : $format_map[ $char ];
		$has_backslash = false;
	}
	
	return $moment_js_format;
}

noisysocks commented on PR #2456:


21 months ago
#10

Thanks @yoancutillas. I'll put the relevant Trac tickets for this patch into the 6.1 milestone so that it doesn't fall off our collective radar again 🙂

#11 @noisysocks
21 months ago

  • Milestone changed from Future Release to 6.1

I think it would be good to include https://github.com/WordPress/wordpress-develop/pull/2456 or some variation of it in 6.1. Just waiting on a code review.

Last edited 21 months ago by noisysocks (previous) (diff)

peterwilsoncc commented on PR #2456:


21 months ago
#12

An alternative to this PR is to remove the inline script from moment altogether. It was added in https://core.trac.wordpress.org/changeset/44262 without any note. I believe it was a mistake. It's not necessary since @wordpress/date will also call moment.updateLocale / moment.defineLocale and @wordpress/date is the only part of WP Admin that relies on moment localisation. It's possible that third party plugins now rely on this, though.

After four years, it's probably safer to assume that it's used somewhere.

I can see a few big plugins using moment as a dependency without wp-date but I am unable to figure out how to tell if they're using the i18n features. A search for moment\. did not help ;)

louwie17 commented on PR #2456:


21 months ago
#13

Hi @noisysocks, just ran across this as I was trying to decipher why the week.dow locale setting stopped working in Wordpress 6.0 (works in 5.9).
I was curious if this fix will also address this issue?
Currently when I run moment().startOf('week'); in 5.9, it adheres to the start_of_week setting from Wordpress settings, if I run it in 6.0, it always returns Sunday (unless I run the updateLocale script again).

Let me know if I should really be creating a new issue for this, more context can be found here: https://a8c.slack.com/archives/C7U3Y3VMY/p1658140947358049

noisysocks commented on PR #2456:


21 months ago
#14

Hey @louwie17. Yes. When I run moment().startOf('week').toString() in trunk I see that it does not respect start_of_week but when I run it on this branch it works okay.

louwie17 commented on PR #2456:


21 months ago
#15

Hey @louwie17. Yes. When I run moment().startOf('week').toString() in trunk I see that it does not respect start_of_week but when I run it on this branch it works okay.

@noisysocks that is good to hear, thanks for checking 👍 it will be great if this could be in the next Wordpress release and/or patch release if there is one.

yoancutillas commented on PR #2456:


19 months ago
#16

On an additional note, since Wordpress doesn't support ordinal suffixes translation, we are losing its support for momentJS too.
It means that every locale will have the english ordinal suffixes (-st, -nd, -rd, -th), inherited from the only available parentLocale 'en'.

We could add an "ordinal" parameter definition in the code (docs), but WP doesn't provides the localized values. And, even if moment-with-locales.js is loaded, our code overrides the locale definition, so with can't get it from momentJS either.

I can't see any solution for that, any idea?

#17 @audrasjb
18 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.

The patch still needs review from the committers assigned for review in the PR.

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


14 months ago

yoancutillas commented on PR #2456:


14 months ago
#19

Hi @peterwilsoncc, the provided wp_convert_php_datetime_format_to_moment_js function handles escaping: wp_convert_php_datetime_format_to_moment_js( '\d' ); returns '[d]' which is displayed by momentjs as the literal character d.

@peterwilsoncc commented on PR #2456:


14 months ago
#20

@yoancutillas Is there any chance you could create a pull request with the example code above and add some unit tests? If you don't have experience writing tests then it's fine if you'd rather not.

This ticket was mentioned in PR #4071 on WordPress/wordpress-develop by yoancutillas.


14 months ago
#21

Same fix as https://github.com/WordPress/wordpress-develop/pull/2456/commits/43e35e246e63e6564f4006545f41726a6f068692

Plus an additional fix:

  • Convert PHP datetime format to momentJS format to correctly display longDateFormat

See the original PR and discussion thread : https://github.com/WordPress/wordpress-develop/pull/2456

Trac ticket:
https://core.trac.wordpress.org/ticket/51142
https://core.trac.wordpress.org/ticket/52695

yoancutillas commented on PR #2456:


14 months ago
#22

I am not used to PR and I have no experience with unit tests, but I think I managed to make a PR: https://github.com/WordPress/wordpress-develop/pull/4071

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


14 months ago

@audrasjb commented on PR #4071:


14 months ago
#24

WPCS issues are fixed :)

yoancutillas commented on PR #4071:


14 months ago
#25

Thank you :)

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


14 months ago

#27 @mukesh27
14 months ago

  • Description modified (diff)
  • Keywords needs-unit-tests added
  • Milestone changed from 6.2 to Future Release

This ticket was discussed during the bug scrub.

As feedback on the PR that needs to address, I think this one is Future Release consideration.

If any committer believes it is ready for commit, please move it back to the 6.2 milestone. 

Additional props: @costdev

@peterwilsoncc commented on PR #4071:


10 months ago
#28

@yoancutillas You mentioned in the thread on the earlier pull request that you don't have experience writing unit tests. Do you mind if I push some to your branch?

yoancutillas commented on PR #4071:


10 months ago
#29

Sure, please do, so I'll see what it's like :)

yoancutillas commented on PR #4071:


10 months ago
#30

Sure, please do, so I'll see what it's like :)

Note: See TracTickets for help on using tickets.