Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#51918 closed defect (bug) (fixed)

I18n - Application Passwords table dates

Reported by: pedromendonca's profile pedromendonca Owned by: timothyblynjacobs's profile 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
3 years 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
3 years 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
3 years 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.


3 years ago
#4

  • 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
3 years 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
3 years 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
3 years ago

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

Last edited 3 years ago by pedromendonca (previous) (diff)

#8 @ocean90
3 years ago

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

#9 @TimothyBlynJacobs
3 years 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.


3 years ago

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


3 years ago

#12 @pedromendonca
3 years ago

It's working for me too :-)

Thanks!

#13 @pedromendonca
3 years 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
3 years 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
3 years 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.


3 years ago

#17 @TimothyBlynJacobs
3 years 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
3 years 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
3 years ago

On Slack, @pedromendonca confirmed it works:

It works, and the solution is way more clean!

#20 @TimothyBlynJacobs
3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#21 @pedromendonca
3 years ago

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

#22 @SergeyBiryukov
3 years 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].

TimothyBJacobs commented on PR #784:


3 years ago
#23

Committed in babba89dd81c7b9944d619b5aa628f33003bd896. Thanks @pedro-mendonca!

pedro-mendonca commented on PR #784:


3 years ago
#24

Thanks! :-)

Note: See TracTickets for help on using tickets.