Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#49060 closed defect (bug) (fixed)

wp_die() to accept text_direction = 'ltr'

Reported by: apedog's profile apedog Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: I18N Keywords: has-patch needs-testing
Focuses: Cc:

Description

wp_die() only accepts text_direction='rtl'. It should accept 'ltr' as well.

If site language/locale is set to an rtl language, but a plugin produces a message that does not have a translation text to that rtl language, there is no way to make wp_die() style that message correctly.

Attachments (2)

49060.diff (752 bytes) - added by apedog 3 years ago.
49060.2.diff (2.1 KB) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (15)

#1 @SergeyBiryukov
3 years ago

Hi there, thanks for the ticket.

This appears to be a bit trickier than I thought.

_wp_die_process_input() actually supports both `rtl` and `ltr`, however _default_wp_die_handler() only uses that value if `language_attributes()` and `is_rtl()` are not available yet, otherwise it just uses get_language_attributes().

Introduced in [16392].

The passed text_direction argument would need to overwrite the one from get_language_attributes().

Some potential solutions:

  • Use str_replace() to overwrite dir="rtl" with dir="ltr" if $parsed_args['text_direction'] is ltr.
  • Add a new parameter to get_language_attributes() with an array of default arguments.

A workaround would be to use the language_attributes filter before calling wp_die():

add_filter(
	'language_attributes',
	function( $output ) {
		return str_replace( 'dir="rtl"', 'dir="ltr"', $output );
	}
);
wp_die( 'Message', 'Title' );

#2 follow-up: @apedog
3 years ago

I see.
The way I read this the problem is two-fold

  1. _default_wp_die_handler() prefers language_attributes over explicitly passed text_direction. ie. the argument is only a fallback. This is a design issue and is the main problem IMO.
  1. _default_wp_die_handler() cannot even know if text_direction was explicitly set in $args or was implicitly set by _wp_die_process_input() in $parsed_args. This is a minor technical issue.

Filtering language_attributes seems like overkill to me.

The way I see it, explicitly passed argument should be paramount. Followed by the current handling.
in _default_wp_die_handler() it should look something like this:

$text_direction = $parsed_args['text_direction'];
if ( ! empty( $args['text_direction'] ) && in_array( $args['text_direction'], array( 'ltr', 'rtl' ), true ) ){
    $dir_attr = "dir='$text_direction'";
} else if ( function_exists( 'language_attributes' ) && function_exists( 'is_rtl' ) ) {
    $dir_attr = get_language_attributes();
} else {
    $dir_attr = "dir='$text_direction'";
}

Note: the if condition is tested against $args - not $parsed_args

@apedog
3 years ago

#3 @apedog
3 years ago

  • Keywords has-patch added

#4 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.4
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

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


3 years ago

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


3 years ago

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


3 years ago

#8 @davidbaumwald
3 years ago

  • Milestone changed from 5.4 to 5.5

With 5.4 RC1 landing tomorrow, this is being moved to 5.5. If any maintainer or committer feels this can be resolved in time or wishes to assume ownership during a specific cycle, feel free to update the milestone.

#9 @SergeyBiryukov
3 years ago

  • Milestone changed from 5.5 to 5.6

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


3 years ago

#11 @williampatton
3 years ago

  • Keywords needs-testing added

#12 in reply to: ↑ 2 @SergeyBiryukov
3 years ago

  • Milestone changed from 5.6 to 5.5

wp_die() only accepts text_direction='rtl'. It should accept 'ltr' as well.

To summarize the previous comments a bit, while wp_die() does accept the text_direction argument, it is only used as a fallback if neither language_attributes() nor is_rtl() are available, e.g. if WordPress is loaded in SHORTINIT mode or is not fully loaded in some other way.

Some history:

  • The argument was introduced in [11408] / #6132 for a single instance in wp-load.php.
  • That instance was later removed in [19760] / #18180, since then the argument is not used anywhere in core.

So, technically both ltr and rtl are already accepted, it's just that get_language_attributes(), once available, overrides the passed text_direction value. The suggestion in this ticket is to flip the order and only use the value of get_language_attributes() if text_direction was not explicitly passed.

49060.2.diff simplifies the logic a bit and adds documentation.

#13 @SergeyBiryukov
3 years ago

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

In 48607:

I18N: Respect the passed text_direction argument in wp_die().

Previously, the passed value was only used as a fallback if get_language_attributes() is not yet available.

Props apedog.
Fixes #49060.

Note: See TracTickets for help on using tickets.