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

Patch Valid::ip to work with PHP v7.0.10+ AND v5.6.25+ #699

Closed
wants to merge 2 commits into from

Conversation

enov
Copy link
Contributor

@enov enov commented Aug 29, 2016

In a breaking change, php/php-src#1954 added FILTER_FLAG_NO_PRIV_RANGE to FILTER_FLAG_NO_RES_RANGE.

The patch looks awful. Let me know if there is any better way to fix this.

Cheers!


$is_valid_public_ip = (bool) filter_var($ip, FILTER_VALIDATE_IP, $flags);

if ( ! $allow_private)
Copy link

@rjd22 rjd22 Sep 1, 2016

Choose a reason for hiding this comment

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

Please keep this if ($allow_private === FALSE) for backwards compatibility. What if someone does:

Valid::ip('ip', null);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... I understand.

@rjd22
Copy link

rjd22 commented Sep 1, 2016

@enov I'm missing tests if possible. :)

@enov
Copy link
Contributor Author

enov commented Sep 1, 2016

Thanks for reviewing @rjd22. Kindly note that the tests are failing already, as PHP v7.0.10+ AND v5.6.25+ introduced the breaking bug fixes in those minor versions.

see for example #698

Thanks again.

@enov
Copy link
Contributor Author

enov commented Sep 5, 2016

Please do not merge this, as it might be solved in the next minor versions of PHP php/php-src#2113

@enov
Copy link
Contributor Author

enov commented Sep 8, 2016

Fixed in PHP.

@enov enov closed this Sep 8, 2016
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