WordPress.org

Make WordPress Core

Opened 4 weeks ago

Closed 3 weeks ago

#48319 closed defect (bug) (fixed)

About wp_date function when locale is 'ja'

Reported by: tmatsuur Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.3
Component: Date/Time Keywords: has-patch has-unit-tests commit dev-reviewed
Focuses: Cc:
PR Number:

Description

Version 5.3 adds the wp_date function.

If locale is 'ja' and 'F' is specified for the parameter of this function, an extra backslash is added to the return value.

Try dumping the return value of wp_date function and date_i18n function.

var_dump( wp_date( 'F' ), date_i18n( 'F' ) );

Dumped contents.

string(6) "\10月"
string(6) "\10月"

In the previous version, the return value of the date_i18n function has no backslash.

string(5) "10月"

I think the cause is in the part where the format is converted according to the locale.

wp-includes/functions.php : 259 - 261

case 'F':
	$new_format .= backslashit( $month );
	break;

This $new_format variable is used as is in the format method.

$date = $datetime->format( $new_format );

The date_i18n function of version 5.2.4 uses the preg_replace function during format conversion.

$dateformatstring     = preg_replace( '/([^\\\])F/', "\\1" . backslashit( $datemonth ), $dateformatstring );

Since wp_date function does not use preg_replace function, I don't think it is necessary to use backslashit function.

Attachments (2)

48319-fixed-wp_date-escaping.patch (3.1 KB) - added by Rarst 4 weeks ago.
48319.diff (3.0 KB) - added by peterwilsoncc 3 weeks ago.

Download all attachments as: .zip

Change History (19)

#1 @ocean90
4 weeks ago

  • Component changed from General to Date/Time
  • Milestone changed from Awaiting Review to 5.3

@Rarst Can you confirm this report?

#2 @Rarst
4 weeks ago

Can confirm the regression.

First we still do need to slash. Date localisation is double-pass, first WP handles the localized bits, then PHP handles the rest. But since we don't want PHP to interpret parts we already processed as date format we escape them.

The problem is backslashit() has a special case of double-slashing numbers at start of string (I don't know why on top of my head). So in this case 1 turns into \\\\\1 (two escaped slashes).

We don't need this to happen, however since older code was more convoluted \\\\1 collapsed into \1 while being passed around (PHP handles double slashes as single slash whenever it processes a string) and didn't affect the output.

Since new code is more streamlined \\1 reaches the final processing and produces \1 in final output.

So we need to swap backslashit() use in this case with code that only escapes the letters (e.g. addcslashes( $string, 'A..Za..z' ) part of backslashit() implementation). Maybe more narrowly code that only escapes valid date format letters.

We could do this inline or introduce new helper function for clarity. Will think a bit and work on a patch.

cc @remcotolsma who wrote the new implementation for opinion. :)

#3 @remcotolsma
4 weeks ago

I don't follow why backslashit adds backslashes before a number at the start of a string:
https://github.com/WordPress/WordPress/blob/5.2/wp-includes/formatting.php#L2615-L2628

Related commits:

It looks like it was added after the following Trac ticket en WordPress support topic:

Back then it seems to be related to a Date/Time issue with preg_replace and older PHP versions. But since preg_replace is no longer used and WordPress requires PHP 5.6.20+ the backslashit function might even be simplified to:

<?php

function backslashit( $string ) {
        return addcslashes( $string, 'A..Za..z' );
}

But I think using addcslashes with only the date format characters is also a nice fix for this problem.

I have added a unit test to our own datetime library for this issue:
https://github.com/pronamic/wp-datetime/commit/ff16527c5d7055c559dcf01760e6189f30044f42.

And a fix:
https://github.com/pronamic/wp-datetime/commit/09424a2c3585f495f8dd6ea880c400f557c475f9

#4 follow-up: @Rarst
4 weeks ago

I am hesitant to change backslashit() implementation. While de-facto core use seems to only be in date stuff, it's not documented as such and who knows how it might be used downstream.

So far I am thinking a conservative fix, changing to addcslashes( $string, 'A..Za..z' ) in wp_date() implementation. Not going to date format only yet. Probably more future proof too (e.g. what if new format character is added upstream).

#5 @remcotolsma
4 weeks ago

I also think there is currently an issue when in a translation a backslash is used.


global $wp_locale;

$month_translation = 'ABCD \ 1234';

$wp_locale->month['10'] = $month_translation;
$wp_locale->month_abbrev[ $month_translation ] = $month_translation;

echo date_i18n( 'F' ); // Result: 'ABCD  1234'

Maybe the charlist for addcslashes should be 'A..Za..z\\'...

https://github.com/pronamic/wp-datetime/commit/1b38e58f081db58c120ff808ce9f782d2ba3648f

We should add some unit tests for these cases in WordPress test library.

Last edited 4 weeks ago by remcotolsma (previous) (diff)

#6 @Rarst
4 weeks ago

Yeah, I was just thinking how are the slashes handled... If I understand it right there are also nuances with order of characters so that escaping slashes don't get themselves escaped.

#7 in reply to: ↑ 4 @SergeyBiryukov
4 weeks ago

Replying to Rarst:

So far I am thinking a conservative fix, changing to addcslashes( $string, 'A..Za..z' ) in wp_date() implementation. Not going to date format only yet. Probably more future proof too (e.g. what if new format character is added upstream).

Sounds good to me.

#8 @Rarst
4 weeks ago

  • Keywords has-patch needs-unit-tests added

This ticket was mentioned in Slack in #core-datetime by rarst. View the logs.


4 weeks ago

#10 @Rarst
4 weeks ago

  • Keywords has-unit-tests added; needs-unit-tests removed

#11 @SergeyBiryukov
4 weeks ago

  • Keywords commit added

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


4 weeks ago

#13 @SergeyBiryukov
4 weeks ago

  • Keywords dev-feedback added

Marking for a second committer's review.

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


3 weeks ago

@peterwilsoncc
3 weeks ago

#15 @peterwilsoncc
3 weeks ago

  • Keywords dev-reviewed added; dev-feedback removed

LGTM, tests well and unit tests passing.

Couple of non-code changes in 48319.diff:

  • a little white space tidy-up
  • added @ticket docblock to the second test so it's included when running npm run test:php -- --group 48319

#16 @SergeyBiryukov
3 weeks ago

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

#17 @SergeyBiryukov
3 weeks ago

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

In 46569:

Date/Time: Make sure wp_date() does not unnecessarily escape localized numbers, but keeps localized slashes.

Props Rarst, tmatsuur, remcotolsma, peterwilsoncc.
Reviewed by peterwilsoncc.
Fixes #48319.

Note: See TracTickets for help on using tickets.