Skip to content

[2.5] Added the Valiator method isBoolean #197

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

Merged
merged 11 commits into from Jan 28, 2018
Merged

[2.5] Added the Valiator method isBoolean #197

merged 11 commits into from Jan 28, 2018

Conversation

ghost
Copy link

@ghost ghost commented Oct 1, 2016

No description provided.

@GrahamCampbell
Copy link
Collaborator

Thanks for the PR. Our variables are always strings though sent they?

@ghost
Copy link
Author

ghost commented Oct 1, 2016

Yes all values are in the format of strings this method ensures they are in the format accepted as booleans - Yes, No, 1, 0, On and Off

@danhunsaker
Copy link

Has some weird indentation, but otherwise it looks sound.

Copy link
Contributor

@jpuck jpuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. It uses simple built-ins, but in my opinion there is one non-intuitive condition with filter_var and FILTER_NULL_ON_FAILURE which returns FALSE on the empty string "" which I don't think makes sense with dotenv (but maybe it does).

There needs to be some unit tests to accompany changes, and there's some minor whitespace consistency issues.

I submitted a pull request which should resolve all these issues.

Furthermore, while probably out of scope, it might be worth considering other edge cases such as (true) like it's handled in the laravel framework.

add tests and fix empty string evaluated as false
@jpuck
Copy link
Contributor

jpuck commented Feb 3, 2017

whoops, looks like the tests are not compatible with ye olde php 5.3
I'll update that to use the formal array() instead of []

fixed in https://github.com/sephedo/phpdotenv/pull/2

build: https://travis-ci.org/scratchers/phpdotenv/builds/198163739

@jpuck jpuck mentioned this pull request Apr 27, 2017
@jamesj2
Copy link

jamesj2 commented Jan 25, 2018

It's been a year. Any possibility of this getting merged?

@vlucas
Copy link
Owner

vlucas commented Jan 28, 2018

This looks pretty solid to me. Has tests and will not affect anyone if they are not using the boolean validator.

@vlucas vlucas merged commit 90b2f41 into vlucas:master Jan 28, 2018
@GrahamCampbell GrahamCampbell changed the title Added the Valiator method isBoolean [2.5] Added the Valiator method isBoolean Jan 28, 2019
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.

5 participants