Make WordPress Core

Opened 2 years ago

Last modified 2 years ago

#56025 new defect (bug)

wp_validate_boolean() not doing what it describes, causes issues with [video] shortcode

Reported by: arnodeleeuw's profile arnodeleeuw Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 6.0
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

The function in question:

/**
* Filter/validate a variable as a boolean.
*
* Alternative to `filter_var( $var, FILTER_VALIDATE_BOOLEAN )`.
*
* @since 4.0.0
*
* @param mixed $var Boolean value to validate.
* @return bool Whether the value is validated.
*/
function wp_validate_boolean( $var ) {
    if ( is_bool( $var ) ) {
        return $var;
    }
 
    if ( is_string( $var ) && 'false' === strtolower( $var ) ) {
        return false;
    }
 
    return (bool) $var;
}

Steps to recreate the issue:

Add the following shortcodes to a page:

[video src="YOUR-SOURCE-HERE"]
[video src="YOUR-SOURCE-HERE" loop="off"]
[video src="YOUR-SOURCE-HERE" loop="0"]
[video src="YOUR-SOURCE-HERE" loop="false"]
  • The first shortcode works as intended, rendering a video on the frontend without the loop attribute.
  • The second shortcode's <video> element will have the attribute loop="1" despite the loop attribute being set to off.
  • The third and fourth shortcode's <video> elements will not have the loop attribute set.

Description:

wp_validate_boolean() says it 's an alternative to filter_var( $var, FILTER_VALIDATE_BOOLEAN ) , however
filter_var( $var, FILTER_VALIDATE_BOOLEAN ) will return FALSE when you pass the string "off" whereas wp_validate_boolean() will return TRUE in this case.
(See this for the documentation for filter_validate_boolean: https://www.w3schools.com/php/filter_validate_boolean.asp).

Currently, wp_validate_boolean() only returns FALSE if the $var is a string and if it is === "false", or if the $var passed is the int 0.

This causes unexpected behaviour in the [video] shortcode. The documentation of the video shortcode (https://wordpress.org/support/article/video-shortcode/) mentions that the loop attribute needs to be either "on"/"off". Because they are strings wp_validate_boolean() will return TRUE, causing the loop attribute to always be added with a value of "1" unless you change the value of the attribute to be specifically the string "false", the int 0, or leave it completely empty.

The parsing of the attributes of the [video] shortcode happens here: (https://github.com/WordPress/wordpress-develop/blob/6.0/src/wp-includes/media.php#L3322-L3344).
As you can see on line 3340 it uses wp_validate_boolean() to get filter value of the loop attribute. Passing "off" here returns true, causing the loop attribute to always be added to the eventual HTML output later down the line with a value of "1".

Potential fix:

  • Change the working of wp_validate_boolean() to do as it says in its description. Make it smarter, and thus also return FALSE when the strings "off","OFF","no","NO", etc. are passed just like filter_var( $var, FILTER_VALIDATE_BOOLEAN ) does (See this documentation link: https://www.php.net/manual/en/function.filter-var.php#121263).
  • Change how the parsing of attributes happens in the [video] shortcode so that it also works as the documentation says: not adding certain attributes when they are set to "off". However there may be other instances where wp_validate_boolean() is used that I'm currently not aware of, so this would be more like a band-aid fix and similar issues may arise in the future.

Attachments (1)

56025.diff (493 bytes) - added by karpstrucking 2 years ago.
update wp_validate_boolean() to validate false-y strings

Download all attachments as: .zip

Change History (2)

@karpstrucking
2 years ago

update wp_validate_boolean() to validate false-y strings

#1 @karpstrucking
2 years ago

  • Component changed from Shortcodes to General
  • Keywords has-patch dev-feedback added

hey @arnodeleeuw !

thanks for opening this ticket.

I've uploaded a simple patch that addresses this for "off" and "no".

devs: any value if making this list of false-y strings filterable?

Note: See TracTickets for help on using tickets.