Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#40829 closed enhancement (fixed)

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

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


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 years ago.
40829.2.diff (5.1 KB) - added by Mte90 7 years ago.
$r to $parsed_args

Download all attachments as: .zip

Change History (13)

#1 @johnjamesjacoby
7 years 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
7 years 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
7 years 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
7 years 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
7 years 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.

7 years ago

7 years ago

$r to $parsed_args

#7 @Mte90
7 years ago

  • Keywords needs-refresh removed

$r to $parsed_args done :-)

#8 @jbpaul17
7 years ago

  • Keywords needs-testing added

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

7 years ago

#10 @SergeyBiryukov
7 years 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
7 years 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.