Invalid Moment.js locale from get_user_locale()

[slightlyfaulty]

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

[slightlyfaulty]

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.

[SergeyBiryukov]

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().

[iandunn]

Related:

• ​https://github.com/WordPress/gutenberg/issues/15221 - Ordinal suffix token does not work in wp.date.format
• ​https://github.com/WordPress/gutenberg/pull/25782 - Use date-fns and date-fns-tz instead of moment

[noisysocks]

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

This makes sense to me!

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

[noisysocks]

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?

[peterwilsoncc]

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.

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;
}

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 🙂

[noisysocks]

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.

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 ;)

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

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.

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.

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?

[audrasjb]

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.

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.

@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.

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

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

WPCS issues are fixed :)

Thank you :)

[mukesh27]

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

@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?

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

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