file_get_contents warnings suppression

[blacktek]

Currently file_get_contents is called like this

$response = file_get_contents($this->siteVerifyUrl, false, $context);

Could it make sense to use the error suppression with @ to avoid random errors going to stdout?

We capture few errors per week like Failed to open stream: HTTP request failed! HTTP/1.1 503 Service Unavailable

[garak]

No, it doesn't make sense at all. You should handle your exceptions yourself.

[blacktek]

@garak I agree that you should handle the exceptions ourselves, which we do. But the default behaviour of file_get_contents is to also send a warning to stdout (and such output might break the response to the client)

[garak]

I get my errors into Sentry, so there's a way to do it.

[blacktek]

you get the errors in Sentry, but the output to the end customer will be broken, despite we catch the error. I make an example:

1. ajax request to an endpoint checking the recaptcha
2. there is a temporary error with "$response = file_get_contents($this->siteVerifyUrl, false, $context);" and we catch it and "try to" send a proper ajax/json response to the customer saying that there was a temporary error.

but due to the missing warning suppression the output sent back to the customer is corrupted (not a valid json anymore) due to PHP printing the warning in the output.

Is it more clear like this?

[garak]

The error output should never be active in production

[blacktek]

I agree @garak. It's only active in our production environment used by the internal staff

[garak]

And so in the end the one to blame for your wrong output is you, not this library.

[blacktek]

If you're happy with this answer and feel good to have defended a position, Yes, you're right.

[garak]

If you're happy with this answer and feel good to have defended a position, Yes, you're right.

I'm afraid that adding sarcasm and passive-aggressive messages won't help that much to defend your position (which is already hard to defend, in any case)

[blacktek]

Hi @garak I agree on the sarcasm. But you started with "And so in the end the one to blame for your wrong output is you, not this library."

The library output (not mine!) is caused by using the wrong approach to make requests, and is sending unmanaged output in case of warnings. e.g. this would not have happened if you were using curl. You're saying that it's our fault because in production we should keep error_display to no, but the principle is wrong in any case. The errors should be catched and thrown at the upper level. You can't justify that it's correct like this because it's not. A library should not write output to stdout/stderr

[garak]

Mine was not sarcasm, really. Libraries should never hide errors, because the way to manage errors is not the same for everyone, so the responsibility is (correctly) left to the developer. Moreover, hiding errors would prevent the developer from picking the correct way to handle them (see the aforementioned Sentry). Again, it's not the library that sends output, it's you.

[blacktek]

I've not said that you should hide errors. You should suppress the errors sent to the output (if you want to continue use file_get_contents), catch the error after the file_get_contents and throw the exception

[garak]

You can already suppress the error output by correctly configuring your environment. I agree that you should probably avoid using the Post request method entirely.