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

Add more ip validation #78

Merged
merged 4 commits into from Jun 25, 2015
Merged

Conversation

BallisticPain
Copy link

Adds validation for v4 and v6 through 'ip'
Adds validation for v6 through 'ipv6'
Updates validation for v4 by using filter_var instead of ip2long


// This validates without regard to reserved or private ranges in both v4 and v6
if (filter_var($value, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4 | FILTER_FLAG_IPV6) === false) {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

What about changing it to something like

if (filter_var($value, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4 | FILTER_FLAG_IPV6)) {
    return true;
}
return false;

Is there any reason you didn't do that ? Just my suggestion, you should hear what @pmjones thinks about this before doing the change.

Copy link
Author

Choose a reason for hiding this comment

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

@harikt,

Good question. I was just attempting to be explicit. I don't imagine there is an IP Address that could return and be considered a false, but the way I wrote it ensures that cannot happen.

Thoughts?
Jarvis

Copy link
Contributor

Choose a reason for hiding this comment

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

filter_var is an odd duck - like strcmp 0 might be a perfectly legal return and PHP does... odd things with strings that start with 0
it's generally better to do an exact === check, since that is actual failure for the filter, not just a value PHP would consider "empty " as the return

Copy link
Member

Choose a reason for hiding this comment

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

Thank you @auroraeosrose for clarifying.

@BallisticPain all is good .

Thank you.

@cxj
Copy link

cxj commented Jun 25, 2015

I read through the code. This looks like a good enhancement.

pmjones pushed a commit that referenced this pull request Jun 25, 2015
@pmjones pmjones merged commit 429e131 into auraphp:2.x Jun 25, 2015
@pmjones
Copy link
Member

pmjones commented Jun 25, 2015

Thanks @BallisticPain for submitting, and everyone else for reviewing. :-)

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.

None yet

5 participants