Opened 4 years ago
Last modified 4 months ago
#56025 new defect (bug)
wp_validate_boolean() not doing what it describes, causes issues with [video] shortcode
| Reported by: |
|
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 likefilter_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)
Change History (5)
#1
@
4 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?
This ticket was mentioned in PR #9670 on WordPress/wordpress-develop by @callumbw95.
5 months ago
#2
Update the wp_validate_boolean() function to work with truthy values.
Trac ticket: #56025
#3
@
5 months ago
Hi @karpstrucking,
I have just come across this ticket, and after reviewing the initial "falsey list" approach, I'd like to propose we consider flipping the logic to use a "truthy list" instead. My main concern with the original approach is that it defaults to true for any unrecognised string, which could lead to unexpected behaviour.
Here’s a quick summary of why I think a "truthy list" is a safer and more robust approach:
- It's Safer by Default: This approach works on an "allow list" principle. We explicitly define what is true. Anything else—including typos, empty values, or unexpected inputs—will safely default to false. This prevents a feature from being accidentally enabled.
- It's More Explicit: The code becomes self-documenting about what values are considered active or "on". There's no ambiguity.
- It Avoids Unintended Consequences: With a "falsey list," an input like 'truu' or 'ok' would evaluate to true. The proposed method correctly identifies these as false.
I have put in a PR in with this change now, so please let me know your thoughts. 😃
@mindctrl commented on PR #9670:
4 months ago
#4
@CallumBW95 this PR causes existing unit tests to fail.
1) Tests_Functions_wpValidateBoolean::test_wp_validate_boolean with data set #4 ('false', false)
Failed asserting that true is identical to false.
/var/www/tests/phpunit/tests/functions/wpValidateBoolean.php:24
2) Tests_Functions_wpValidateBoolean::test_wp_validate_boolean with data set #5 ('FalSE', false)
Failed asserting that true is identical to false.
/var/www/tests/phpunit/tests/functions/wpValidateBoolean.php:24
3) Tests_Functions_wpValidateBoolean::test_wp_validate_boolean with data set #6 ('FALSE', false)
Failed asserting that true is identical to false.
/var/www/tests/phpunit/tests/functions/wpValidateBoolean.php:24
update wp_validate_boolean() to validate false-y strings