WordPress.org

Make WordPress Core

Opened 5 weeks ago

Last modified 3 weeks ago

#47720 reviewing defect (bug)

Walker_Nav_Menu filter nav_menu_link_attributes checks $atts argument via empty() and not isset(), causing confusion with legitimate values that evaluate to boolean false

Reported by: nevma Owned by: SergeyBiryukov
Milestone: 5.3 Priority: normal
Severity: normal Version: 5.2.2
Component: Menus Keywords: has-patch needs-testing
Focuses: template Cc:

Description

In file wp-includes/class-walker-nav-menu.php, line 206, class function start_el(), where the nav_menu_link_attributes filter is applied, the current code checks the supplied attributes via PHP empty() function.

This is the current code:

foreach ( $atts as $attr => $value ) {
    if ( ! empty( $value ) ) {
        $value       = ( 'href' === $attr ) ? esc_url( $value ) : esc_attr( $value );
        $attributes .= ' ' . $attr . '="' . $value . '"';
    }
}

If an attribute with a legitimate value, which evaluates to something that equals to boolean false is passed, then the if clause will not be executed. However, there are many cases where such values are useful.

For instance an HTML data attribute with a value of zero (0) could be passed as an attribute in the $atts array, but its value would then be equal to boolean false (as far as PHP empty() is concerned and thus the value would be erroneously ignored.

I believe that this check would be better done with the PHP isset() function like this:

foreach ( $atts as $attr => $value ) {
    if ( ! isset( $value ) ) {
        $value       = ( 'href' === $attr ) ? esc_url( $value ) : esc_attr( $value );
        $attributes .= ' ' . $attr . '="' . $value . '"';
    }
}

Attachments (2)

47720.diff (564 bytes) - added by AkSDvP 5 weeks ago.
47720.2.diff (564 bytes) - added by AkSDvP 5 weeks ago.

Download all attachments as: .zip

Change History (5)

#1 @AkSDvP
5 weeks ago

  • Keywords has-patch needs-testing added; needs-patch removed

Key differences between both methods is-

isset - Determine if a variable is set and is not NULL.
!empty - Determine whether a variable is NOT empty.

Considering the above point I have replaced
if ( ! empty( $value ) )
with
if ( isset( $value ) )

@AkSDvP
5 weeks ago

@AkSDvP
5 weeks ago

#2 @greenshady
3 weeks ago

Just ran into this problem myself.

In a similar attribute system I built, here's how I handle it, which also allows attributes without a value (where the value would be the same as the attribute name):

foreach ( $this->all() as $name => $value ) {

	$esc_value = '';

	// If the value is a link `href`, use `esc_url()`.
	if ( $value !== false && 'href' === $name ) {
		$esc_value = esc_url( $value );

	} elseif ( $value !== false ) {
		$esc_value = esc_attr( $value );
	}

	$html .= false !== $value ? sprintf( ' %s="%s"', $name, $esc_value ) : " {$name}";
}

#3 @SergeyBiryukov
3 weeks ago

  • Milestone changed from Awaiting Review to 5.3
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing
Note: See TracTickets for help on using tickets.