Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Probably a wrong logical operator #697

Closed
zeleznypa opened this issue May 2, 2024 · 2 comments
Closed

Probably a wrong logical operator #697

zeleznypa opened this issue May 2, 2024 · 2 comments

Comments

@zeleznypa
Copy link

zeleznypa commented May 2, 2024

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
Copy link
Collaborator

PhrozenByte commented May 2, 2024

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" 😆) 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.

Copy link

github-actions bot commented May 9, 2024

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! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants