WordPress.org

Make WordPress Core

Opened 7 weeks ago

Closed 7 weeks ago

Last modified 7 weeks ago

#51918 closed defect (bug) (fixed)

I18n - Application Passwords table dates

Reported by: pedromendonca Owned by: TimothyBlynJacobs
Milestone: 5.6 Priority: normal
Severity: normal Version: 5.6
Component: Application Passwords Keywords: has-patch
Focuses: Cc:

Description

Hi, in the Application Passwords table, there are some dates missing i18n in:

echo gmdate( get_option( 'date_format', 'r' ), $item['created'] );
Source: https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/includes/class-wp-application-passwords-list-table.php#L71

echo gmdate( get_option( 'date_format', 'r' ), $item['last_used'] );
Source: https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/includes/class-wp-application-passwords-list-table.php#L86

Would it be ok to change gmdate() to date_i18n()?

Change History (24)

#1 follow-up: @TimothyBlynJacobs
7 weeks ago

  • Component changed from I18N to Application Passwords
  • Milestone changed from Awaiting Review to 5.6

Thanks for the ticket! Do you want to work on a patch?

#2 in reply to: ↑ 1 @pedromendonca
7 weeks ago

Replying to TimothyBlynJacobs:

Thanks for the ticket! Do you want to work on a patch?

Sure. I need some help on the JS template.

#3 follow-up: @TimothyBlynJacobs
7 weeks ago

Are there issues with the JS side of things? It is using dateI18n.

This ticket was mentioned in PR #784 on WordPress/wordpress-develop by pedro-mendonca.


7 weeks ago

  • Keywords has-patch added

This PR fixes the dates i18n on the Application Passwords table for Created and Last Used columns

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

#5 in reply to: ↑ 3 @pedromendonca
7 weeks ago

Replying to TimothyBlynJacobs:

Are there issues with the JS side of things? It is using dateI18n.

Here is how to reproduce the JS issue:
The portuguese translation for F j, Y is j \d\e F, Y.
F j, Y - December 2, 2020
j \d\e F, Y - 2 de Dezembro, 2020

It's ok on page load, but is not on creating a new app password and JS row injection.

The result is:
2 02WET Dezembro, 2020 - It seems the \d\e aren't being treated as simple text letters.

#6 @TimothyBlynJacobs
7 weeks ago

Ah, I think that is because we're using esc_js which is dropping the backslashes and isn't actually correct for that placement I believe. We should be able to switch that out to esc_attr.

#7 @pedromendonca
7 weeks ago

Changing the esc_js to esc_attr doesn't solve it.
Not even removing the escaping on those lines.

Last edited 7 weeks ago by pedromendonca (previous) (diff)

#8 @ocean90
7 weeks ago

Just noting that the option shouldn't actually be used for the admin, see [35811].

#9 @TimothyBlynJacobs
7 weeks ago

Ah my mistake, thanks @ocean90!

@pedromendonca Interesting. This is what I get when I saved that date format.

wp eval "echo esc_js( get_option( 'date_format' ) );"
j de F, Y
wp eval "echo esc_attr( get_option( 'date_format' ) );"
j \d\e F, Y

Testing further, it seems like the backslashes need to be doubled when output. Replacing those lines with this seems to work for me:

esc_attr( str_replace( '\\', '\\\\', __( 'F j, Y' ) ) )

I'm testing with a filter in place to change the translation.

<?php
add_filter( 'gettext', function ( $translated, $text ) {
        if ( $text === 'F j, Y' ) {
                $translated = 'j \d\e F, Y';
        }

        return $translated;
}, 10, 2 );

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


7 weeks ago

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


7 weeks ago

#12 @pedromendonca
7 weeks ago

It's working for me too :-)

Thanks!

#13 @pedromendonca
7 weeks ago

On print_js_template_row(), for the column 'last_used', is it really necessary to check for existent data.last_used since it's a newly created application password?

Couldn't it just be set as empty?

Example:

case 'last_used':
	echo '—';
	break;

#14 @TimothyBlynJacobs
7 weeks ago

@pedromendonca Great! I'd prefer it to be more robust to handle that case. For instance this can be repurposed to allow for paginating over the list of App Passwords via the REST API instead of requiring separate page loads.

#15 @pedromendonca
7 weeks ago

That makes sense.

Do you have any idea on how to clean this up to avoid the str_replace() ?

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


7 weeks ago

#17 @TimothyBlynJacobs
7 weeks ago

Hey @pedromendonca,

It looks like we can avoid this by letting wp_json_encode handle the backslashing for us. Could you give the patch another go to make sure it all still works as expected?

#18 @TimothyBlynJacobs
7 weeks ago

  • Owner set to TimothyBlynJacobs
  • Resolution set to fixed
  • Status changed from new to closed

In 49746:

App Passwords: Ensure the Created At and Last Used dates are properly translated.

The date_i18n function is now used when formatting the dates in PHP instead of gmdate which doesn't handle localization properly.

Additionally, we now use a translation to get the date format to use instead of pulling from the date_format option which is only supposed to affect the front-end.

Lastly, when passing the date format to the Backbone JS template, we now use wp_json_encode() to format the value for JavaScript. This ensures that backslashes are properly preserved which are used by some locales to escape date formatting control characters.

Props pedromendonca, TimothyBlynJacobs, ocean90, hellofromtonya, SergeyBiryukov, antpb.
Fixes #51918.
See [35811].

#19 @hellofromTonya
7 weeks ago

On Slack, @pedromendonca confirmed it works:

It works, and the solution is way more clean!

#20 @TimothyBlynJacobs
7 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#21 @pedromendonca
7 weeks ago

It looks great to me, and clean!
Thanks @TimothyBlynJacobs for your help :-)

#22 @SergeyBiryukov
7 weeks ago

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

In 49747:

App Passwords: Ensure the Created At and Last Used dates are properly translated.

The date_i18n function is now used when formatting the dates in PHP instead of gmdate which doesn't handle localization properly.

Additionally, we now use a translation to get the date format to use instead of pulling from the date_format option which is only supposed to affect the front-end.

Lastly, when passing the date format to the Backbone JS template, we now use wp_json_encode() to format the value for JavaScript. This ensures that backslashes are properly preserved which are used by some locales to escape date formatting control characters.

Props pedromendonca, TimothyBlynJacobs, ocean90, hellofromtonya, SergeyBiryukov, antpb.
Reviewed by TimothyBlynJacobs, SergeyBiryukov.
Merges [49746] to the 5.6 branch.
Fixes #51918.
See [35811].

#23 @prbot
7 weeks ago

TimothyBJacobs commented on PR #784:

Committed in babba89dd81c7b9944d619b5aa628f33003bd896. Thanks @pedro-mendonca!

#24 @prbot
7 weeks ago

pedro-mendonca commented on PR #784:

Thanks! :-)

Note: See TracTickets for help on using tickets.