picocms / Pico

Pico is a stupidly simple, blazing fast, flat file CMS.
http://picocms.org/
MIT License
3.81k stars 616 forks source link

Probably a wrong logical operator #697

Closed zeleznypa closed 1 month ago

zeleznypa commented 2 months ago

Presumably what is meant here is a situation where the input value is validated to boolean and if it is not a boolean, it should return null instead of false.

Except that in this case, the behavior is that the text string 'NULL_ON_FAILURE' only works for 'FILTER_VALIDATE_BOOLEAN' and otherwise ends up with an error Undefined constant "FILTER_FLAG_NULL_ON_FAILURE".

https://github.com/picocms/Pico/blob/09aa82578710d82dd7dc482febe32991be0ea307/lib/Pico.php#L2572C51-L2572C53

However, when reversing the logic, 'NULL_ON_FAILURE' to 'FILTER_VALIDATE_BOOLEAN' is not always applied because $flags can be an empty array.

So personally, I find the && ($filters === FILTER_VALIDATE_BOOLEAN) part to be unnecessary

PhrozenByte commented 2 months ago

That was introduced to better distinguish the behaviour of validate and sanitize filters. Pico::filterVariable() aims to be used in Twig, thus it's a little stricter (nowadays we call that "opinionated" :laughing:) on how it's supposed to be used. Validate filters are supposed to return the original value if it's valid, or false otherwise. Sanitize filters are supposed to always return a sanitized value, or the default value.

However, for FILTER_VALIDATE_BOOLEAN the idea of a validate filter to return the original value is ambiguous in case of false, since false is a valid boolean, yet it's also used to yield an invalid value. Throwing an exception for an invalid value would have been better, but as I said, Pico::filterVariable() aims to be used in Twig, and Twig doesn't really support exception handling.

So, generally we don't want NULL_ON_FAILURE because it breaks the idea about validate and sanitize filters outlined earlier, yet we need it for a single use case:

if ($this->getPico()->filterVariable($someInput, 'boolean', false, [ 'null_on_failure' ]) !== null) {
    /* valid input */
}

If you want to use NULL_ON_FAILURE in your Pico plugin, you could always use PHP's filter_var() directly. For Pico themes I'd like to ask what you want to achieve first.

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in two days if no further activity occurs. Thank you for your contributions! :+1: