Skip to content

Remove warnings from inet_pton()/inet_ntop() #3200

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 1 commit into from
Apr 3, 2018

Conversation

DaveRandom
Copy link
Contributor

@DaveRandom DaveRandom commented Mar 29, 2018

This patch removes the warnings emitted by inet_ntop()/inet_pton() as they do not impart any useful information not already available to userland, and superfluous warnings can cause debugging problems. These functions are frequently used as a quick mechanism to validate data from external sources along with normalization and conversion, so it is reasonable to expect that bad input may be supplied and this should not be considered exceptional.

This patch targets 7.1, as the fact that these functions emit warnings is not documented and the warnings do not affect the required userland logic - the return value still needs to be checked for failures.

Updated to target master.

@bukka
Copy link
Member

bukka commented Mar 29, 2018

Couldn't this be an issue for apps that convert warnings to exceptions and might relay on exception to be thrown? The fact that it's not documented doesn't mean that it can't break anything... :) We have got bunch of stuff not documented...

+1 for master though...

@DaveRandom
Copy link
Contributor Author

@bukka reluctantly agreed, will re-target master and merge

@DaveRandom DaveRandom changed the base branch from PHP-7.1 to master March 30, 2018 18:53
@DaveRandom DaveRandom force-pushed the remove-inet-errors branch from ae90c7a to 822b47c Compare April 1, 2018 23:05
@DaveRandom DaveRandom force-pushed the remove-inet-errors branch from 822b47c to 658a23a Compare April 3, 2018 13:52
@php-pulls php-pulls merged commit 658a23a into php:master Apr 3, 2018
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.

3 participants