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

Suppress stream_socket_client warnings #184

Merged
merged 1 commit into from
Feb 17, 2024

Conversation

mbardelmeijer
Copy link
Contributor

Without this change, PHP warnings may pop-up:

PHP Warning:  stream_socket_client(): php_network_getaddresses: getaddrinfo for xxxx.xx failed: Name or service not known in vendor/spatie/ssl-certificate/src/Downloader.php on line 185

Seems to have been changed with #183. From what I can see, even when suppressing, error handling still happens.

@freekmurze freekmurze merged commit d9a3f23 into spatie:main Feb 17, 2024
4 checks passed
@freekmurze
Copy link
Member

Thanks!

@rudiedirkx
Copy link
Contributor

So now #183 doesn't work anymore. This needs better error handling. Maybe it's different in different PHP versions? Unfortunately 3v4l doesn't support this test: https://3v4l.org/Y7giY

@rudiedirkx
Copy link
Contributor

rudiedirkx commented Feb 19, 2024

I think this package needs a temporary error handler override, to make sure this works the same in any environment. In my tests a lot of errors didn't go to $errorNumber and $errorDescription (with @), but they did become an useful Exception (without @). I only tested inside Laravel though, which converts all notices/warnings/errors into exceptions. But the suppression missed a lot of useful error message info. Everything became a meaningless

$clientErrorMessage = ($this->usingIpAddress)
    ? "Could not connect to `{$connectTo}:{$this->port}` or it does not have a certificate matching `{$hostName}`."
    : "Could not connect to `{$connectTo}:{$this->port}`.";

because the $errorDescription was almost always empty.

@freekmurze
Copy link
Member

I'll revert this change for now.

@rudiedirkx
Copy link
Contributor

@mbardelmeijer In what framework/context did you get a PHP Warning instead of exception? Maybe we can all get what we want with a custom temp error handler or something, but to make sure we'll need a few (testable?) scenarios.

@mbardelmeijer
Copy link
Contributor Author

@mbardelmeijer In what framework/context did you get a PHP Warning instead of exception? Maybe we can all get what we want with a custom temp error handler or something, but to make sure we'll need a few (testable?) scenarios.

@rudiedirkx we're using Laravel 10. We were seeing better results with the error handling suppressed, but this indeed loses some specificity regarding the exact error.

For instance:

[ERROR] PHP Warning:  stream_socket_client(): php_network_getaddresses: getaddrinfo for XXXXX failed:
         Temporary failure in name resolution in

 [ERROR] PHP Warning:  stream_socket_client(): SSL operation failed with code 1. OpenSSL Error messages:
         error:14094438:SSL routines:ssl3_read_bytes:tlsv1 alert internal error in

After further debugging, it may be because we're running the SSL monitor via spatie/aysnc, which has its quirks with error handling. I've added a custom error handling to our codebase to resolve this.

The revert is correct for this package as the original solution without error suppression returns better errors. Sorry for the confusion here!

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