WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months 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
3 months 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 months 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 months 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 months 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
3 months 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 months 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 months ago

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

The esc_js is escaping the WP option, not the translation where actually the \d\e is.

Version 0, edited 3 months ago by pedromendonca (next)

#8 @ocean90
3 months ago

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

#9 @TimothyBlynJacobs
3 months 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 months ago

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


3 months ago

#12 @pedromendonca
3 months ago

It's working for me too :-)

Thanks!

#13 @pedromendonca
3 months 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 months 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 months 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 months ago

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

On Slack, @pedromendonca confirmed it works:

It works, and the solution is way more clean!

#20 @TimothyBlynJacobs
3 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#21 @pedromendonca
3 months ago

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

#22 @SergeyBiryukov
3 months 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
3 months ago

TimothyBJacobs commented on PR #784:

Committed in babba89dd81c7b9944d619b5aa628f33003bd896. Thanks @pedro-mendonca!

#24 @prbot
3 months ago

pedro-mendonca commented on PR #784:

Thanks! :-)

Note: See TracTickets for help on using tickets.