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

Fix: boolean casting #195

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

haddowg
Copy link

@haddowg haddowg commented Jun 3, 2024

This PR fixes two issues related to request validation.

  • If a parameter is not null but "falsey" such as 0 the parameter it is not validated
    this PR corrects this and updates the tests

  • It is currently not possible to validate "boolean" type parameters as the assertion will always fail since the string type value from the request parameter is not cast to boolean. The data (string) must match the type: boolean
    This PR casts true and false string values only to the appropriate boolean, and adds test coverage

@bastien-phi
Copy link
Collaborator

Hi @haddowg ! Thanks for your contribution.

I'm a bit concerned with your PR as 'true' and 'false' are not considered as valid boolean in Laravel validation

// Trait Illuminate\Validation\Concerns\ValidatesAttributes

public function validateBoolean($attribute, $value)
{
    $acceptable = [true, false, 0, 1, '0', '1'];

    return in_array($value, $acceptable, true);
}

So considering that they are valid booleans and not '0' and '1' is not consistent with that.

I'm marking this PR as draft in case you want to edit it.

@bastien-phi bastien-phi marked this pull request as draft January 17, 2025 08:01
fix: correctly validate "falsey" not null parameters values
@haddowg
Copy link
Author

haddowg commented Jan 18, 2025

@bastien-phi In my use case I am strictly validating the strings true and false only, as this is what URLSearchParams serialization of booleans will produce, but I agree with your point, it is best if the spec validation is more permissive.

i have updated my change to use the PHP native filter_var method with FILTER_VALIDATE_BOOLEAN this is slightly more permissive than laravel's boolean validation but feels like a better default as users are not necessarily using laravel's validation, but regardless should probably still be using filter_var to actually cast the value to bool.

@haddowg haddowg marked this pull request as ready for review January 18, 2025 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants