\Safe\file_get_contents generating warning instead of exception

[josefsabl]

When calling \Safe\file_get_contents("http://nonexistanturl.foo") generates warning that cannot be caught.

The problem is that file_get_contents not only returns FALSE on failure but can also generate warning.

Supressing the warning by adding @ to \file_get_contents internal call fixes the issue.

[andreasschroth]

But that's not the correct solution of the problem, isn't it?

The purpose of this package is, imo, that we can safely use all the functions provided here. This means that an exception is thrown in case of an error / warning / etc. and that the script execution immediately stops.

If you just silence the call internally, it's not even logged in the logs - that's the opposite of what we want.

[josefsabl]

But that's not the correct solution of the problem, isn't it?

The purpose of this package is, imo, that we can safely use all the functions provided here. This means that an exception is thrown in case of an error / warning / etc. and that the script execution immediately stops.

If you just silence the call internally, it's not even logged in the logs - that's the opposite of what we want.

A bit of a misunderstanding I believe.

If the @ operator is not there, the script crashes and there is no way to handle it in the code. There is no way to catch the warning without fooling around with error handlers (I guess).

If the operator IS there, the Filesystem exception is generated by your library correctly, it can be caught and acted upon.

I would humbly reword your description of purpose of the package (to what I thought it was): "We can safely use all the functions provided here by turning all the PHP's inconsistent error handling (false return values, warnings, etc...) into manageable exceptions." and this issue is all about that.

PS: Maybe there is a cleaner solution to this (by those error handlers?) than the @ operator and that would be cool. I can just confirm that the @ works as expected and fixed the issue for me.

[andreasschroth]

But that's not the correct solution of the problem, isn't it? The purpose of this package is, imo, that we can safely use all the functions provided here. This means that an exception is thrown in case of an error / warning / etc. and that the script execution immediately stops. If you just silence the call internally, it's not even logged in the logs - that's the opposite of what we want.

A bit of a misunderstanding I believe.

If the @ operator is not there, the script crashes and there is no way to handle it in the code. There is no way to catch the warning without fooling around with error handlers (I guess).

If the operator IS there, the Filesystem exception is generated by your library correctly, it can be caught and acted upon.

I would humbly reword your description of purpose of the package (to what I thought it was): "We can safely use all the functions provided here by turning all the PHP's inconsistent error handling (false return values, warnings, etc...) into manageable exceptions." and this issue is all about that.

PS: Maybe there is a cleaner solution to this (by those error handlers?) than the @ operator and that would be cool. I can just confirm that the @ works as expected and fixed the issue for me.

Sorry then, got it wrong then at the beginning. :) Then I think your solution is a good way as the exception is already thrown anyway. Nobody needs the exception and the warning.

Thanks for explanation.

PS: I'm not the maintainer of this package.

[moufmouf]

@josefsabl sorry for the very late answer.

We are having a hard time to decide whether we should silence those warnings or not. Most of the time, it would work, as the exception thrown manages to get the message of the warning (via the error_get_last function).

The problem is that somtimes, some functions are throwing many errors (for instance one E_NOTICE, then one E_WARNING). If we silence the function, we will only get the last error.

As an alternative, we could overload the error handler, but that would mean overloading the error handler on every function call and restoring it back just after, which would certainly have a performance impact. So we are still looking for the best solution. Any idea is welcome.

[jakubvojacek]

I suggested the same in https://github.com/thecodingmachine/safe/issues/77. I think it would be great if all the warnings were silenced

[josefsabl]

@josefsabl sorry for the very late answer.

We are having a hard time to decide whether we should silence those warnings or not. Most of the time, it would work, as the exception thrown manages to get the message of the warning (via the error_get_last function).

The problem is that somtimes, some functions are throwing many errors (for instance one E_NOTICE, then one E_WARNING). If we silence the function, we will only get the last error.

As an alternative, we could overload the error handler, but that would mean overloading the error handler on every function call and restoring it back just after, which would certainly have a performance impact. So we are still looking for the best solution. Any idea is welcome.

I understand that this cannot be handled without loosing any information and am glad you are looking into the issue.

But I still believe that (and would vote/lobby for) making it consistent and in line with purpose of this library (turn unmanageable mess of false return values, uncatchable warnings etc. into manageable exceptions) is much better solution than keeping this 'broken'. And I believe it IS broken atm. Currently, the Safe\file_get_contents is not safe at all 🤔 and we cannot use it.

We keep hitting this problem randomly and costs us wasted time and effort. Our current use case: use file_get_contents to download .json from URL. file_put_contents to filesystem => crashes the app when the response status is 500. No way to handle it simply without rewriting it. As 500s are fairly random we will also need to write a mock for that so that we can test it without trial and error. I believe that suppressing the Safe\file_get_contents itself could do the trick, I am however not certain so will have to write the mock anyway.

[danepowell]

So would you accept a PR to suppress error output (including warnings) from file_get_contents?

I switched to this library specifically to suppress uncatchable warnings from file_get_contents in a safe way. I was disappointed to find that the warnings were not, in fact, suppressed.