WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 2 months ago

#40829 closed enhancement (fixed)

Improve wp_dropdown_languages() so it doesn't print out empty attributes

Reported by: afercia Owned by: SergeyBiryukov
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: I18N Keywords: has-patch needs-testing
Focuses: Cc:

Description

wp_dropdown_languages() takes an array of arguments with some defaults. Among them, the arguments related to the name and id HTML attributes default to empty string.

Later on, these attributes are printed out without any check if they're empty or not:
$output = sprintf( '<select name="%s" id="%s">', esc_attr( $args['name'] ), esc_attr( $args['id'] ) );

The final output might print out empty name and id attributes if the related arguments are not explicitly passed: <select name="" id="">, which doesn't look right to me.

Attachments (2)

40829.diff (4.9 KB) - added by johnjamesjacoby 7 months ago.
40829.2.diff (5.1 KB) - added by Mte90 3 months ago.
$r to $parsed_args

Download all attachments as: .zip

Change History (13)

#1 @johnjamesjacoby
7 months ago

  • Keywords has-patch 2nd-opinion added

40829.diff proposes the following changes:

  • Set id and name to locale by default instead of empty, to circumvent the issue proposed here. This also promotes the use of locale for future usages by core and plugins.
  • Bail early if ID or name are intentionally forced to be empty.
  • Move the beginning of the $output string creation to the end of the function with the other concatenation
  • Switch $args replacement to use the $r variable approach, as per most other wp_parse_args() usages in core. This prevents stomping the original $args array, in the event a filter is introduced into this function later.
  • Add some more inline documentation where blocks of code perform separate things.

Tests are passing. No further changes are needed to any usages of wp_dropdown_languages().

#2 follow-up: @ocean90
6 months ago

  • Keywords needs-refresh added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

Let's not change $args to $r, that's not really relevant for this change.

#3 in reply to: ↑ 2 @johnjamesjacoby
6 months ago

Replying to ocean90:

Let's not change $args to $r, that's not really relevant for this change.

It will be relevant when there's a variable collision, and it's already a commonly used approach in core.

This is still a bug, because form elements cannot have empty names.

Leaving wp_dropdown_languages() as is increases the opportunity for it to introduce more problems, as this ticket discovered already.

Why not just fix this now and move onto bigger things? This should be committed as patched, not moved into the Future Release abyss.

#4 @afercia
6 months ago

Scanning the codebase for occurrences of wp_parse_args( $args seems to me in most of the recent files, including for example class-language-pack-upgrader.php, the variable name is a bit more meaningful: $parsed_args. In old files, it's often $args or $r.

#5 @SergeyBiryukov
6 months ago

  • Milestone changed from Future Release to 4.9

Let's change $r to $parsed_args, seems good to me otherwise.

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


3 months ago

@Mte90
3 months ago

$r to $parsed_args

#7 @Mte90
3 months ago

  • Keywords needs-refresh removed

$r to $parsed_args done :-)

#8 @jbpaul17
2 months ago

  • Keywords needs-testing added

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


2 months ago

#10 @SergeyBiryukov
2 months ago

In 41733:

I18N: In wp_dropdown_languages(), change the parsed arguments variable to $parsed_args, to prevent stomping the original $args array.

Props Mte90.
See #40829.

#11 @SergeyBiryukov
2 months ago

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

In 41734:

I18N: Make sure wp_dropdown_languages() does not print out empty name and id attributes.

Props johnjamesjacoby, afercia.
Fixes #40829.

Note: See TracTickets for help on using tickets.