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

[6.x] Add boolean($key) method to Request #31160

Merged
merged 5 commits into from Jan 17, 2020
Merged

[6.x] Add boolean($key) method to Request #31160

merged 5 commits into from Jan 17, 2020

Conversation

@LasseRafn
Copy link
Contributor

LasseRafn commented Jan 17, 2020

This PR introduces a new helper method to the Illuminate\Http\Request: boolean($key) which takes input using the input() method and filters it through filter_var and FILTER_VALIDATE_BOOLEAN

Brief about FILTER_VALIDATE_BOOLEAN from php.net:

Returns TRUE for "1", "true", "on" and "yes". Returns FALSE otherwise.

// Before
$availableForHire = filter_var(Request::boolean('available_for_hire'), FILTER_VALIDATE_BOOLEAN);

// After
$availableForHire = Request::boolean('available_for_hire');

I set the default to be false so than an undefined variable (eg unchecked checkbox) would act as false.

Unsure how everyone feels about this and the naming of it though.

(edit: removed references to FILTER_NULL_ON_FAILURE as it was removed from the code)

LasseRafn added 4 commits Jan 17, 2020
boolean method added
Add tests for boolean method
@GrahamCampbell GrahamCampbell changed the title Add boolean($key) method to Request [6.x] Add boolean($key) method to Request Jan 17, 2020
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 17, 2020

What made you want to use FILTER_NULL_ON_FAILURE?

@LasseRafn

This comment has been minimized.

Copy link
Contributor Author

LasseRafn commented Jan 17, 2020

Honestly I did not want to at first, but figured if the value is something entirely different than a Boolean (“hello world”) then it probably shouldn’t be one? But I don’t know.

Personally I never use it myself but thought I’d hear screams if not used, haha.

— edit —

So I am 100% up for removing that!

@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 17, 2020

Personally I would remove it. It adds a third value that this function can return which gives the user more situations to consider, etc.

@LasseRafn

This comment has been minimized.

Copy link
Contributor Author

LasseRafn commented Jan 17, 2020

Very good point! I’ll modify

@taylorotwell taylorotwell merged commit bc8c1d2 into laravel:6.x Jan 17, 2020
2 checks passed
2 checks passed
continuous-integration/styleci/pr The analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@taylorotwell

This comment has been minimized.

Copy link
Member

taylorotwell commented Jan 17, 2020

Could you send us a PR to the laravel/docs repository (6.x branch). Thanks.

@LasseRafn

This comment has been minimized.

Copy link
Contributor Author

LasseRafn commented Jan 18, 2020

Thanks! Let me know if wording etc. should be improved

@shez1983

This comment has been minimized.

Copy link

shez1983 commented Jan 22, 2020

this is a general comment - ignore if not suitable - in another PR the tester used a php annotation

/*
* @dataProvider validUuidList 
*/

wheras here its being done 5/6 times in the test - is there concern for consistency or it doesnt matter?

@sharifzadesina

This comment has been minimized.

Copy link

sharifzadesina commented Feb 4, 2020

What is the point of this while we don't allow string values like "on"/"off" inside validation?
https://github.com/laravel/framework/blob/6.x/src/Illuminate/Validation/Concerns/ValidatesAttributes.php#L353-L358
Also, why adding too many methods in request? While people can use "sanitizer" libraries?
https://github.com/Waavi/Sanitizer

@LasseRafn

This comment has been minimized.

Copy link
Contributor Author

LasseRafn commented Feb 4, 2020

@sharifzadesina

This comment has been minimized.

Copy link

sharifzadesina commented Feb 4, 2020

@LasseRafn How this simplifies the code while the validator doesn't accept "false", "true", "on", "off", string values? also, in PHP, values like, 0, 1, "0", "1", can be considered as booleans, which are already accepted by the validator.
Anyways, this PR is merged, but I think, this just causes more confusion.

@LasseRafn

This comment has been minimized.

Copy link
Contributor Author

LasseRafn commented Feb 4, 2020

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.