Don't silence fatal errors

5 years ago by Nikita Popov — view source

unread

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake. In particular, the following should not be silenced:

E_ERROR
E_CORE_ERROR
E_COMPILE_ERROR
E_USER_ERROR
E_RECOVERABLE_ERROR
E_PARSE

This change would have two main implications for backwards compatibility:

Code that legitimately wants to silence fatal errors for whatever reason
should now use error_reporting() (or ini_set()) to do so, instead of @.
Error handlers that want to only handle non-silenced errors may have to
be adjusted. A common pattern I found in our own tests if checks for
error_reporting() != 0 to detect silencing. This should be changed to
error_reporting() & $err_no to detect whether the specific error type is
silenced.

A preliminary patch for this change is available at
https://github.com/php/php-src/pull/3685.

What do you think about this?

Nikita

5 years ago by Thomas Hruska — view source

unread

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake. In particular, the following should not be silenced:

E_ERROR

E_CORE_ERROR

E_COMPILE_ERROR

E_USER_ERROR

E_RECOVERABLE_ERROR

E_PARSE

This change would have two main implications for backwards compatibility:

Code that legitimately wants to silence fatal errors for whatever reason
should now use error_reporting() (or ini_set()) to do so, instead of @.

Error handlers that want to only handle non-silenced errors may have to
be adjusted. A common pattern I found in our own tests if checks for
error_reporting() != 0 to detect silencing. This should be changed to
error_reporting() & $err_no to detect whether the specific error type is
silenced.

A preliminary patch for this change is available at
https://github.com/php/php-src/pull/3685.

What do you think about this?

Nikita

Instead of a blank screen (or early termination if some output has been
sent), maybe emit, "[GMT date/time] A fatal error occurred. Check the
error logs." The only bug I see here is that fatal errors are being
suppressed from reaching the log files. But they should still be
suppressed from the browser/client if the INI settings are configured to
send messages to the logs.

WSODs should have been fixed a long time ago to emit a simple, generic
message to check the logs (i.e. no more WSODs). The average WSOD is
usually accompanied with a HTTP 500 response but having to look at
network tools tab to see the 500 is an extra step. A surprising number
of developers I encounter don't know what a HTTP 500 means nor what to
do when they encounter one. Therefore, helpful but very generic
directions would be useful and save a few moments of head-scratching.

Also, forcing users to override the default error handler to restore
previous (and almost correct) behavior is a bit obnoxious.

--
Thomas Hruska
CubicleSoft President

I've got great, time saving software that you will find useful.

http://cubiclesoft.com/

And once you find my software useful:

http://cubiclesoft.com/donate/

5 years ago by Nikita Popov — view source

unread

On Tue, Nov 27, 2018 at 2:20 PM Thomas Hruska thruska@cubiclesoft.com
wrote:

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result
will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out
multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake. In particular, the following should not be silenced:

E_ERROR

E_CORE_ERROR

E_COMPILE_ERROR

E_USER_ERROR

E_RECOVERABLE_ERROR

E_PARSE

This change would have two main implications for backwards compatibility:

Code that legitimately wants to silence fatal errors for whatever
reason
should now use error_reporting() (or ini_set()) to do so, instead of @.

Error handlers that want to only handle non-silenced errors may have
to
be adjusted. A common pattern I found in our own tests if checks for
error_reporting() != 0 to detect silencing. This should be changed to
error_reporting() & $err_no to detect whether the specific error type is
silenced.

A preliminary patch for this change is available at
https://github.com/php/php-src/pull/3685.

What do you think about this?

Nikita

Instead of a blank screen (or early termination if some output has been
sent), maybe emit, "[GMT date/time] A fatal error occurred. Check the
error logs." The only bug I see here is that fatal errors are being
suppressed from reaching the log files. But they should still be
suppressed from the browser/client if the INI settings are configured to
send messages to the logs.

WSODs should have been fixed a long time ago to emit a simple, generic
message to check the logs (i.e. no more WSODs). The average WSOD is
usually accompanied with a HTTP 500 response but having to look at
network tools tab to see the 500 is an extra step. A surprising number
of developers I encounter don't know what a HTTP 500 means nor what to
do when they encounter one. Therefore, helpful but very generic
directions would be useful and save a few moments of head-scratching.

I think you are mixing two orthogonal degrees of error configurability
here, which are

a) The error_reporting level, which determines which errors are reported in
the first place, and which is what @ influences, and

b) The display_error, error_log etc. directives, which control what happens
when an error is reported.

The proposed change does not impact b) in any way. If you have
display_errors=Off and use error_log (as you should in production), you use
@ and a fatal error occurs, then (with the proposed change) no error will
be displayed, but it will be logged. If you have display_errors=On and
don't use error_log (as is common in development), you use @ and a fatal
error occurs, then (with the proposed change) the error will be directly
displayed. Without the proposed change, in both cases, you would not get an
error, either logged or displayed.

Nikita

5 years ago by Thomas Hruska — view source

unread

On Tue, Nov 27, 2018 at 2:20 PM Thomas Hruska thruska@cubiclesoft.com
wrote:

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result
will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out
multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake. In particular, the following should not be silenced:

E_ERROR

E_CORE_ERROR

E_COMPILE_ERROR

E_USER_ERROR

E_RECOVERABLE_ERROR

E_PARSE

This change would have two main implications for backwards compatibility:

Code that legitimately wants to silence fatal errors for whatever
reason
should now use error_reporting() (or ini_set()) to do so, instead of @.

Error handlers that want to only handle non-silenced errors may have
to
be adjusted. A common pattern I found in our own tests if checks for
error_reporting() != 0 to detect silencing. This should be changed to
error_reporting() & $err_no to detect whether the specific error type is
silenced.

A preliminary patch for this change is available at
https://github.com/php/php-src/pull/3685.

What do you think about this?

Nikita

Instead of a blank screen (or early termination if some output has been
sent), maybe emit, "[GMT date/time] A fatal error occurred. Check the
error logs." The only bug I see here is that fatal errors are being
suppressed from reaching the log files. But they should still be
suppressed from the browser/client if the INI settings are configured to
send messages to the logs.

WSODs should have been fixed a long time ago to emit a simple, generic
message to check the logs (i.e. no more WSODs). The average WSOD is
usually accompanied with a HTTP 500 response but having to look at
network tools tab to see the 500 is an extra step. A surprising number
of developers I encounter don't know what a HTTP 500 means nor what to
do when they encounter one. Therefore, helpful but very generic
directions would be useful and save a few moments of head-scratching.

I think you are mixing two orthogonal degrees of error configurability
here, which are

a) The error_reporting level, which determines which errors are reported in
the first place, and which is what @ influences, and

b) The display_error, error_log etc. directives, which control what happens
when an error is reported.

The proposed change does not impact b) in any way. If you have
display_errors=Off and use error_log (as you should in production), you use
@ and a fatal error occurs, then (with the proposed change) no error will
be displayed, but it will be logged. If you have display_errors=On and
don't use error_log (as is common in development), you use @ and a fatal
error occurs, then (with the proposed change) the error will be directly
displayed. Without the proposed change, in both cases, you would not get an
error, either logged or displayed.

Nikita

The way it was worded sounded like the changes might override the
directives in b).

Thanks for the clarification. Carry on.

--
Thomas Hruska
CubicleSoft President

I've got great, time saving software that you will find useful.

http://cubiclesoft.com/

And once you find my software useful:

http://cubiclesoft.com/donate/

5 years ago by Agustin Casiva — view source

unread

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out
multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake. In particular, the following should not be silenced:

E_ERROR

E_CORE_ERROR

E_COMPILE_ERROR

E_USER_ERROR

E_RECOVERABLE_ERROR

E_PARSE

This change would have two main implications for backwards compatibility:

Code that legitimately wants to silence fatal errors for whatever reason
should now use error_reporting() (or ini_set()) to do so, instead of @.

Error handlers that want to only handle non-silenced errors may have to
be adjusted. A common pattern I found in our own tests if checks for
error_reporting() != 0 to detect silencing. This should be changed to
error_reporting() & $err_no to detect whether the specific error type is
silenced.

A preliminary patch for this change is available at
https://github.com/php/php-src/pull/3685.

What do you think about this?

Nikita

I think the need of the @ is to silence everything, and I think is used
only in extreme cases where the developer can't handle properly the errors
(or the dev is kinda lazy to do it, I have used it a couple of times of
course :) ).

The developer that uses @ knows his risks and knows that can hides
important information for debugging, I don't think that showing fatal error
will help the developers in the long term, yes it might help it when is
debugging, but later when the code is in production and the @ is still in
place I think the developers will not expect to see any error.

I don't see the real value of this change knowing the price of lost of
backward compatibility.

My two cents.

Best

--
Casiva Agustin

Mail/Msn/GTalk/Jabber: casivaagustin@gmail.com
Skype: casivaagustin
CEL : 054-0362-155280015
Site: http://www.casivaagustin.com.ar

5 years ago by Claude Pache — view source

unread

Le 26 nov. 2018 à 22:42, Nikita Popov nikita.ppv@gmail.com a écrit :

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake. In particular, the following should not be silenced:

E_ERROR

E_CORE_ERROR

E_COMPILE_ERROR

E_USER_ERROR

E_RECOVERABLE_ERROR

E_PARSE

This change would have two main implications for backwards compatibility:

Code that legitimately wants to silence fatal errors for whatever reason
should now use error_reporting() (or ini_set()) to do so, instead of @.

Error handlers that want to only handle non-silenced errors may have to
be adjusted. A common pattern I found in our own tests if checks for
error_reporting() != 0 to detect silencing. This should be changed to
error_reporting() & $err_no to detect whether the specific error type is
silenced.

A preliminary patch for this change is available at
https://github.com/php/php-src/pull/3685.

What do you think about this?

Nikita

Although this can be viewed as an issue of the silencing operator, this can also be viewed as an issue of the default error handler, which should not blindly obey the error_reporting() directive (or the @ operator) in case of fatal error.

Hopefully, custom error handlers are able to scream even when they are asked to shut up.

My suggestion is rather to change the implementation of the default error handler, so that it refuses to ever sweep fatal error messages under the rug. Code that has legitimate reasons to silence fatal errors can always use a custom error handler that will obey it.

—Claude

5 years ago by Fwentish Aelondes — view source

unread

Breaking BC might be unnecessary if instead of changing the default
behavior of @, you add an additional flag to error_reporting that
enables the new behavior, something like E_UNSILENCE_FATAL. Then
developers would only need to switch a flag in php.ini to get the old
behavior back, instead of re-working existing code around the new
behavior.

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake. In particular, the following should not be silenced:

E_ERROR

E_CORE_ERROR

E_COMPILE_ERROR

E_USER_ERROR

E_RECOVERABLE_ERROR

E_PARSE

This change would have two main implications for backwards compatibility:

Code that legitimately wants to silence fatal errors for whatever reason
should now use error_reporting() (or ini_set()) to do so, instead of @.

Error handlers that want to only handle non-silenced errors may have to
be adjusted. A common pattern I found in our own tests if checks for
error_reporting() != 0 to detect silencing. This should be changed to
error_reporting() & $err_no to detect whether the specific error type is
silenced.

A preliminary patch for this change is available at
https://github.com/php/php-src/pull/3685.

What do you think about this?

Nikita

5 years ago by michal@brzuchalski.com — view source

unread

I don't really know if it fits here but some weeks ago I was thinking about
annotations with "@" prefix
and was considering to propose to handle @ (silence operator) similar way
as annotations, like:

$value = @ fopen('test.txt','rb+');

might be equivalent to:

$value = @SupressError(E_ALL) fopen('test.txt','rb+');

BTW This way I believe it would be easier to parse annotations with '@'
prefix in all desired places
with one branch inside parser.

As well as there will be a place to put supression error level per
function/method call
with more specific requirements, like:

$value = @SupressError(E_ALL ^ E_ERROR) fopen('test.txt','rb+');

Which might work as supress all errors except fatal errors.

Does that sound like a solution at all?
The developer then has full controll on what errors are supressed or not.

Sorry to bother you if it's insane and crazy idea.

czw., 29 lis 2018 o 05:44 Fwentish Aelondes fwentish@gmail.com napisał(a):

Breaking BC might be unnecessary if instead of changing the default
behavior of @, you add an additional flag to error_reporting that
enables the new behavior, something like E_UNSILENCE_FATAL. Then
developers would only need to switch a flag in php.ini to get the old
behavior back, instead of re-working existing code around the new
behavior.

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result
will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out
multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake. In particular, the following should not be silenced:

E_ERROR

E_CORE_ERROR

E_COMPILE_ERROR

E_USER_ERROR

E_RECOVERABLE_ERROR

E_PARSE

This change would have two main implications for backwards compatibility:

Code that legitimately wants to silence fatal errors for whatever
reason
should now use error_reporting() (or ini_set()) to do so, instead of @.

Error handlers that want to only handle non-silenced errors may have
to
be adjusted. A common pattern I found in our own tests if checks for
error_reporting() != 0 to detect silencing. This should be changed to
error_reporting() & $err_no to detect whether the specific error type is
silenced.

A preliminary patch for this change is available at
https://github.com/php/php-src/pull/3685.

What do you think about this?

Nikita

--

--
regards / pozdrawiam,

Michał Brzuchalski
about.me/brzuchal
brzuchalski.com

5 years ago by Dan Ackroyd — view source

unread

I don't really know if it fits here but some weeks ago I was thinking about
annotations with "@" prefix
....

Which might work as supress all errors except fatal errors.

Does that sound like a solution at all?
The developer then has full controll on what errors are suppressed or not.

I was thinking along similar lines a while ago:
https://gist.github.com/Danack/5ae0b1b1ce30a0d785dd

The reason I never formally suggested it as an RFC is that I think
it's doubling down on the wrong solution.

The vast majority of places where errors/warning are used currently,
could either be just removed or have the result of the function be
changed to be a tuple of the current result, and an error
flag/message.

[$result, $error] = foo($bar);
if ($error !== null) {
// something went wrong.
}
// $result is usable

cheers
Dan

5 years ago by Christoph M. Becker — view source

unread

I don't really know if it fits here but some weeks ago I was thinking about
annotations with "@" prefix
....

Which might work as supress all errors except fatal errors.

Does that sound like a solution at all?
The developer then has full controll on what errors are suppressed or not..

I was thinking along similar lines a while ago:
https://gist.github.com/Danack/5ae0b1b1ce30a0d785dd

The reason I never formally suggested it as an RFC is that I think
it's doubling down on the wrong solution.

The vast majority of places where errors/warning are used currently,
could either be just removed or have the result of the function be
changed to be a tuple of the current result, and an error
flag/message.

[$result, $error] = foo($bar);
if ($error !== null) {
// something went wrong.
}
// $result is usable

If a function issues a warning and returns some value indicating
failure, we should consider to let the function throw an exception
instead. Typical cases would be getimagesize(), fopen() and
password_hash(), for instance.

--
Christoph M. Becker

5 years ago by Andrea Faulds — view source

unread

Hi Nikita,

Nikita Popov wrote:

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake.
It's always been bizarre to me that @ can silence fatal errors, which
has no practical application and makes using @ to silence a lower-level
error potentially hszardous if its targeted function can also produce a
fatal error.

Obviously, I'd be in favour of fixing this. :)

--
Andrea Faulds
https://ajf.me/

5 years ago by Nikita Popov — view source

unread

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out
multiple times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake. In particular, the following should not be silenced:

E_ERROR

E_CORE_ERROR

E_COMPILE_ERROR

E_USER_ERROR

E_RECOVERABLE_ERROR

E_PARSE

This change would have two main implications for backwards compatibility:

Code that legitimately wants to silence fatal errors for whatever
reason should now use error_reporting() (or ini_set()) to do so, instead of
@.

Error handlers that want to only handle non-silenced errors may have to
be adjusted. A common pattern I found in our own tests if checks for
error_reporting() != 0 to detect silencing. This should be changed to
error_reporting() & $err_no to detect whether the specific error type is
silenced.

A preliminary patch for this change is available at
https://github.com/php/php-src/pull/3685.

What do you think about this?

Nikita

I'd like to move forward with this change. I think the overall reception
here has been positive, although in the discussion some other possibilities
that avoid/reduce the BC aspect have been discussed. I think now that we
have a PHP 8 branch, it would make sense to apply this as-is. The BC break
is quite minor (compared to the other changes in PHP 8) and I think this is
the cleanest way to solve the problem, as it only changes the list of
silences errors, without introducing any new error handling concepts or
mechanisms.

Nikita

5 years ago by Zeev Suraski — view source

unread

On Mon, Nov 26, 2018 at 10:42 PM Nikita Popov nikita.ppv@gmail.com
wrote:

I'd like to move forward with this change. I think the overall reception
here has been positive, although in the discussion some other possibilities
that avoid/reduce the BC aspect have been discussed. I think now that we
have a PHP 8 branch, it would make sense to apply this as-is. The BC break
is quite minor (compared to the other changes in PHP 8) and I think this is
the cleanest way to solve the problem, as it only changes the list of
silences errors, without introducing any new error handling concepts or
mechanisms.

I don't think that other changes that may or may not make it into PHP 8
should influence our decision - BC breaks accumulate and the more you have
of them, the more difficult it is to migrate. I'm also present unaware of
anything we already decided to 'break' in PHP 8 as of now (with the
exception of the removal of deprecated 7.x features).

That said - I think we all agree the BC breakage scope is small - but at
the same time, it may be quite fatal for those that are affected. Those
who have display_errors on (which is both horrible for production and at
the same time fairly popular) are risking exposing sensitive data that
beforehand, they were safely and explicitly hiding using @.

Is there any reason not to do it in such a way that provides a gentler
migration path? Introduce a new error level that would be a part of E_ALL
in 7.4, but outside of E_ALL in 8.0 - that would govern whether fatal
errors are silenced or not.

Zeev

5 years ago by Nikita Popov — view source

unread

On Mon, Nov 26, 2018 at 10:42 PM Nikita Popov nikita.ppv@gmail.com
wrote:

I'd like to move forward with this change. I think the overall reception
here has been positive, although in the discussion some other
possibilities
that avoid/reduce the BC aspect have been discussed. I think now that we
have a PHP 8 branch, it would make sense to apply this as-is. The BC break
is quite minor (compared to the other changes in PHP 8) and I think this
is
the cleanest way to solve the problem, as it only changes the list of
silences errors, without introducing any new error handling concepts or
mechanisms.

I don't think that other changes that may or may not make it into PHP 8
should influence our decision - BC breaks accumulate and the more you have
of them, the more difficult it is to migrate. I'm also present unaware of
anything we already decided to 'break' in PHP 8 as of now (with the
exception of the removal of deprecated 7.x features).

That said - I think we all agree the BC breakage scope is small - but at
the same time, it may be quite fatal for those that are affected. Those
who have display_errors on (which is both horrible for production and at
the same time fairly popular) are risking exposing sensitive data that
beforehand, they were safely and explicitly hiding using @.

I can see the general concern here, but I'm having a hard time imagining
that this will be an issue in practice. If you have display_errors=on you
are at risk of leaking information in error messages with or without this
change. The prime example of leaking information, which is passwords
contained in the exception stack trace of a failed PDO connection, isn't
even affected by this, because the silencing will be removed as part of
exception unwinding anyway.

Is there any reason not to do it in such a way that provides a gentler
migration path? Introduce a new error level that would be a part of E_ALL
in 7.4, but outside of E_ALL in 8.0 - that would govern whether fatal
errors are silenced or not.

The reason not to do it is pretty much the same as always: Adding that new
error level is basically the same as adding a new ini setting (and if we do
want that, then I think it should be it's own ini setting and not hacked in
as part of error_reporting), and you know our usual opinion on that topic.
Furthermore, in this particular case it would also defeat the purpose of
the change. If we set the option such that fatals are silenced by default,
then we may as well not make the change, because the people who benefit
most from it (non-expert users) are not going to change that default.
Conversely, if we do not silence fatals by default, then we don't solve the
issue with display_errors=on you mentioned above (as it requires explicitly
setting an option, in which case they could just set display_errors=off
instead.)

Nikita

5 years ago by Zeev Suraski — view source

unread

On Mon, Nov 26, 2018 at 10:42 PM Nikita Popov nikita.ppv@gmail.com
wrote:

I'd like to move forward with this change. I think the overall reception
here has been positive, although in the discussion some other
possibilities
that avoid/reduce the BC aspect have been discussed. I think now that we
have a PHP 8 branch, it would make sense to apply this as-is. The BC
break
is quite minor (compared to the other changes in PHP 8) and I think this
is
the cleanest way to solve the problem, as it only changes the list of
silences errors, without introducing any new error handling concepts or
mechanisms.

I don't think that other changes that may or may not make it into PHP 8
should influence our decision - BC breaks accumulate and the more you have
of them, the more difficult it is to migrate. I'm also present unaware of
anything we already decided to 'break' in PHP 8 as of now (with the
exception of the removal of deprecated 7.x features).

That said - I think we all agree the BC breakage scope is small - but at
the same time, it may be quite fatal for those that are affected. Those
who have display_errors on (which is both horrible for production and at
the same time fairly popular) are risking exposing sensitive data that
beforehand, they were safely and explicitly hiding using @.

I can see the general concern here, but I'm having a hard time imagining
that this will be an issue in practice. If you have display_errors=on you
are at risk of leaking information in error messages with or without this
change. The prime example of leaking information, which is passwords
contained in the exception stack trace of a failed PDO connection, isn't
even affected by this, because the silencing will be removed as part of
exception unwinding anyway.

Well, there are all sorts of information leaks that can happen as a result
of this, including filesystem layout and even the fact that the server is
running PHP in the first place.

Is there any reason not to do it in such a way that provides a gentler

migration path? Introduce a new error level that would be a part of E_ALL
in 7.4, but outside of E_ALL in 8.0 - that would govern whether fatal
errors are silenced or not.

The reason not to do it is pretty much the same as always: Adding that new
error level is basically the same as adding a new ini setting (and if we do
want that, then I think it should be it's own ini setting and not hacked in
as part of error_reporting), and you know our usual opinion on that topic.
Furthermore, in this particular case it would also defeat the purpose of
the change. If we set the option such that fatals are silenced by default,
then we may as well not make the change, because the people who benefit
most from it (non-expert users) are not going to change that default.
Conversely, if we do not silence fatals by default, then we don't solve the
issue with display_errors=on you mentioned above (as it requires explicitly
setting an option, in which case they could just set display_errors=off
instead.)

While I don't really agree that adding a new error level is equivalent to
adding a new INI entry, and I would go for having it as a part of the error
levels and not as a separate INI entry if we were to add this functionality

I think you're fundamentally right that this approach doesn't truly bring
value in making the migration smoother. I can't really think of an elegant
way to handle this given the unique situation where this whole change is in
the context of error suppression - which means deprecation notices aren't
helpful.

As long as we have a prominent warning about this in our migration guide
alerting people to the associated risk in setups where display errors is on

I can live with the change as-is. How do we ensure that it doesn't get
lost in the clutter given that it has no RFC?

Zeev

5 years ago by Stanislav Malyshev — view source

unread

Hi!

It's always been bizarre to me that @ can silence fatal errors, which
has no practical application and makes using @ to silence a lower-level
error potentially hszardous if its targeted function can also produce a
fatal error.

Yes, silencing something that kills the script is probably not a very
great idea. It can have some BC effects if this is a part of larger
environments where outgoing messages are tracked, but I think it's a
worthy price to pay for making it more sensible.

This won't fix WSOD problem by itself, but probably will make WSODs a
little less annoying to debug. Which is a win in my book.

Stas Malyshev
smalyshev@gmail.com

5 years ago by Zeev Suraski — view source

unread

Hi Nikita,

Nikita Popov wrote:

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result
will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out
multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake.
It's always been bizarre to me that @ can silence fatal errors, which
has no practical application and makes using @ to silence a lower-level
error potentially hszardous if its targeted function can also produce a
fatal error.

Well, it does have one practical application which I think we should pay
attention to if we are to change the behavior - in case of display_errors
being on (yes, nobody should have that in production, but that doesn't
change the fact that many do) - suddenly we would potentially be exposing
sensitive data to remote users - data that was previously explicitly
silenced and with this change we'd be breaking that contract. This is not
an unlikely scenario.

OTOH I do see the value of being able to silence only non-critical
messages. What if we do something along the lines of what somebody
proposed, and introduce another error level E_ERROR_WHILE_SILENCED or
something like that? Folks who would want to will be able to turn it on,
and we can leave it out of E_ALL so that it would require explicit enabling.

Zeev

5 years ago by Christoph M. Becker — view source

unread

As long as we have a prominent warning about this in our migration guide
alerting people to the associated risk in setups where display errors is on

I can live with the change as-is. How do we ensure that it doesn't get
lost in the clutter given that it has no RFC?

Generally, everything that needs to be mentionend in the migration
guide, should have a respective entry in UPGRADING. That makes creating
the migration guide straight forward for the doc team, and worked quite
well for PHP 7.3 at least.

--
Christoph M. Becker

5 years ago by Pierre Joye — view source

unread

Good morning Nikita,

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out
multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake. In particular, the following should not be silenced:

I am surely missing use cases because I wonder why we need @, at all?

Yes there are functions generating extra messages and should not, be fro.
PHP implementation or from external libraries (wrapping stderr to php
errors). All of them could be fixed.

Best,
Pierre

5 years ago by Girgias — view source

unread

Good morning Nikita,

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also silences
fatal errors. As fatal errors also abort request execution, the result
will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out
multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal errors
intake. In particular, the following should not be silenced:

I am surely missing use cases because I wonder why we need @, at all?

Yes there are functions generating extra messages and should not, be fro.
PHP implementation or from external libraries (wrapping stderr to php
errors). All of them could be fixed.

Best,
Pierre

The most common case which comes to mind is to suppress erros while file
reading.
Because even if you check a file exists it could be deleted inbetween the
check and the
read command. As you can see this is even written in the documentation [1]

Best regards

George Peter Banyard

[1] https://secure.php.net/manual/en/function.fopen.php

5 years ago by Yasuo Ohgaki — view source

unread

Good morning Nikita,

Hi internals,

When the silencing operator @ is used, the intention is generally to
silence expected warnings or notices. However, it currently also
silences
fatal errors. As fatal errors also abort request execution, the result
will
often be a hard to debug white screen of death.

The most recent occurrence which motivated me to write this mail is
https://bugs.php.net/bug.php?id=77205, but I've seen this play out
multiple
times already.

I would like to propose to change the behavior of @ to only silence
warnings, notices and other low-level diagnostics, but leave fatal
errors
intake. In particular, the following should not be silenced:

I am surely missing use cases because I wonder why we need @, at all?

Yes there are functions generating extra messages and should not, be fro.
PHP implementation or from external libraries (wrapping stderr to php
errors). All of them could be fixed.

Best,
Pierre

The most common case which comes to mind is to suppress erros while file
reading.
Because even if you check a file exists it could be deleted inbetween the
check and the
read command. As you can see this is even written in the documentation [1]

Best regards

George Peter Banyard

[1] https://secure.php.net/manual/en/function.fopen.php

If a developer decides to write sloppy code, then @ may be acceptable.
I wouldn't use @, though.

IMO, there should be INI switch that kills @ operator.
There is extension module for this purpose, but PHP itself should have it.
@ operator is convenient, but @ is evil and make debugging a lot harder.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net

5 years ago by Pierre Joye — view source

unread

The most common case which comes to mind is to suppress erros while file
reading.
Because even if you check a file exists it could be deleted inbetween the
check and the
read command. As you can see this is even written in the documentation [1]

Best regards

George Peter Banyard

[1] https://secure.php.net/manual/en/function.fopen.php

That is one the cases I meant. Also this is a bug in the user code. I can
imagine doing it on purpose for performance reasons (if the file is
created/deleted by other apps) tho'.
flock is what should be used here.

best,
Pierre

5 years ago by Pierre Joye — view source

unread

On Thu, Feb 7, 2019 at 3:14 PM Christian Schneider
cschneid@cschneid.com wrote:

Am 07.02.2019 um 02:32 schrieb Pierre Joye pierre.php@gmail.com:

The most common case which comes to mind is to suppress erros while file reading.
Because even if you check a file exists it could be deleted inbetween the check and the
read command. As you can see this is even written in the documentation [1]

[1] https://secure.php.net/manual/en/function.fopen.php

That is one the cases I meant. Also this is a bug in the user code. I can
imagine doing it on purpose for performance reasons (if the file is created/deleted by other apps) tho'.
flock is what should be used here.

Sorry if I'm missing something but would flock() help with file_get_contents() and an external program deleting a file?

We have the following fields where we sometimes use @ to suppress error_log entries so they do not hide real problems not handled by our code already:

filesystem operations like file_get_contents() or mkdir() which can have (benevolent) races

json_decode() of external data (we check the validity of the data afterwards anyway)

DB connections which we log more detailed separately where the generic error is not interesting

In general I agree with the notion of using @ as little as possible but I think it will has use cases where using error_reporting() instead would decrease code readability.

Please do not remove @ ;-)

My thought that @ mainly relates to another RFC where errors/warning
are very inconsistently reported or designed (like forcing one to use
@). 8 would be a good candidate to clean that up, like the TypeError
RFC.

best,
Pierre

5 years ago by Christian Schneider — view source

unread

Am 07.02.2019 um 02:32 schrieb Pierre Joye pierre.php@gmail.com:

The most common case which comes to mind is to suppress erros while file reading.
Because even if you check a file exists it could be deleted inbetween the check and the
read command. As you can see this is even written in the documentation [1]

[1] https://secure.php.net/manual/en/function.fopen.php

That is one the cases I meant. Also this is a bug in the user code. I can
imagine doing it on purpose for performance reasons (if the file is created/deleted by other apps) tho'.
flock is what should be used here.

Sorry if I'm missing something but would flock() help with file_get_contents() and an external program deleting a file?

We have the following fields where we sometimes use @ to suppress error_log entries so they do not hide real problems not handled by our code already:

filesystem operations like file_get_contents() or mkdir() which can have (benevolent) races
json_decode() of external data (we check the validity of the data afterwards anyway)
DB connections which we log more detailed separately where the generic error is not interesting

In general I agree with the notion of using @ as little as possible but I think it will has use cases where using error_reporting() instead would decrease code readability.

Please do not remove @ ;-)

Chris

5 years ago by Benjamin Morel — view source

unread

That is one the cases I meant. Also this is a bug in the user code. I can
imagine doing it on purpose for performance reasons (if the file is
created/deleted by other apps) tho'.
flock is what should be used here.

flock doesn't protect from other failures, such as a fread()ing a file
located on NFS. As it stands now, @ may still useful to mute warnings and
throw exceptions instead. The other solution today is to set up a temporary
error handler before calling the native function, and restore the previous
error handler right after, such as here
https://github.com/brick/std/blob/master/src/Internal/ErrorCatcher.php.
This allows to get the error message, but might incur a small performance
penalty.

IMO, @ can be safely removed the day PHP converts current warnings to
exceptions.

The most common case which comes to mind is to suppress erros while file
reading.
Because even if you check a file exists it could be deleted inbetween the
check and the
read command. As you can see this is even written in the documentation
[1]

Best regards

George Peter Banyard

[1] https://secure.php.net/manual/en/function.fopen.php

That is one the cases I meant. Also this is a bug in the user code. I can
imagine doing it on purpose for performance reasons (if the file is
created/deleted by other apps) tho'.
flock is what should be used here.

best,
Pierre

5 years ago by Christian Schneider — view source

unread

Am 07.02.2019 um 10:01 schrieb Benjamin Morel benjamin.morel@gmail.com:

IMO, @ can be safely removed the day PHP converts current warnings to exceptions.

Please don't do that either, I don't want to convert
@mkdir("foo");
to
try { mkdir("foo"); } catch (Exception $e) {}
just to stop my script from exiting if the directory already exists.

Chris

5 years ago by Benjamin Morel — view source

unread

Please don't do that either, I don't want to convert
@mkdir("foo");
to
try { mkdir("foo"); } catch (Exception $e) {}
just to stop my script from exiting if the directory already exists.

What you really want is either another function that does not throw an
exception in the specific case of "the directory already exists", or a
switch to not throw an exception in this specific case: there might be
other reasons that make mkdir() fail, such as a permission error. By
blindly silencing mkdir(), you have no guarantee that the directory will
exist after this call is executed. When mkdir() fails for another reason,
I can tell you that you do want an exception.

Also, some applications might assume that the directory does not already
exist, and want to fail if it does. So both use cases must be available in
the API.

On Thu, 7 Feb 2019 at 10:17, Christian Schneider cschneid@cschneid.com
wrote:

Am 07.02.2019 um 10:01 schrieb Benjamin Morel benjamin.morel@gmail.com:

IMO, @ can be safely removed the day PHP converts current warnings to
exceptions.

Please don't do that either, I don't want to convert
@mkdir("foo");
to
try { mkdir("foo"); } catch (Exception $e) {}
just to stop my script from exiting if the directory already exists.

Chris

5 years ago by Rowan Collins — view source

unread

Also, some applications might assume that the directory does not
already
exist, and want to fail if it does. So both use cases must be available
in
the API.

I absolutely agree, but I also agree that the task here is not "convert warnings to exceptions", it's "design a new I/O API".

It's a subtle difference in framing: the warnings are just a symptom, and we need to address the cause.

Regards,

--
Rowan Collins
[IMSoP]

5 years ago by Christian Schneider — view source

unread

Am 07.02.2019 um 14:28 schrieb Rowan Collins rowan.collins@gmail.com:

Also, some applications might assume that the directory does not already
exist, and want to fail if it does. So both use cases must be available in the API.

mkdir("foo") or fail();

I absolutely agree, but I also agree that the task here is not "convert warnings to exceptions", it's "design a new I/O API".

Agreed.

Please don't try to mutate an existing API into something it is not designed for while introducing BC headaches in the process.

Chris

5 years ago by Stanislav Malyshev — view source

unread

Hi!

I am surely missing use cases because I wonder why we need @, at all?

Many functions have to deal with "dirty" data - e.g. loading a file that
is supposed to be JSON but may be in fact be invalid in any of the
hundreds ways. In some cases, we want full diagnostics, in other cases,
just knowing it's a bad file is enough, and any messages are a waste of
time in the best case, and invitation to DoS in the worst. If it'd
invalid, we drop it and ignore it and don't want to hear anymore about it.

Of course, it's possible to make special validation function to be run
before actual parsing, but the obvious performance and stability issues
with this make it far inferior solution to just use actual parser and
suppress all diagnostics that could possibly come from it.

Yes there are functions generating extra messages and should not, be fro.
PHP implementation or from external libraries (wrapping stderr to php
errors). All of them could be fixed.

In theory, yes. In practice, no, it's not happening anytime soon - we
won't get rid of all warnings in all extensions that may not be needed.
Thus, right now and in foreseeable future, using @ in this cases would
be the easiest method.

--
Stas Malyshev
smalyshev@gmail.com

5 years ago by Stanislav Malyshev — view source

unread

Hi!

My thought that @ mainly relates to another RFC where errors/warning
are very inconsistently reported or designed (like forcing one to use
@). 8 would be a good candidate to clean that up, like the TypeError
RFC.

Cleaning up how PHP does errors would be awesome. After 20+ years of
organic growth without clear standards in this area, we've got a lot of
messy stuff happening there. But I don't think it can solve any
immediate issues - it's not likely that we'll replace every warning in
every extension overnight. It'd be nice if we found some model that
miraculously plugs into existing one and allows to improve things while
keeping everything working. If we get good new APIs - fine, but we'll
need @ to work with old APIs for a while.

Stas Malyshev
smalyshev@gmail.com

5 years ago by Pierre Joye — view source

unread

HI Stas,

Hi!

My thought that @ mainly relates to another RFC where errors/warning
are very inconsistently reported or designed (like forcing one to use
@). 8 would be a good candidate to clean that up, like the TypeError
RFC.

Cleaning up how PHP does errors would be awesome. After 20+ years of
organic growth without clear standards in this area, we've got a lot of
messy stuff happening there. But I don't think it can solve any
immediate issues

it's not likely that we'll replace every warning in

every extension overnight.

You are right, long due. Also 8 is not immediate or overnight.

However I think you are right as well in your other replies. While I wrote
@ free code since years and I rarely see some in modern code, removing it
may bring some BC issues that could delay 8 adoptions.

It'd be nice if we found some model that

miraculously plugs into existing one and allows to improve things while
keeping everything working. If we get good new APIs - fine, but we'll
need @ to work with old APIs for a while.

New APis would be amazing. We need a kind of miracle to agree on these new
APIs but it will be really amazing to have new clear APIs. As far as I
remember we have been there for 6/7 and failed to find a consensus.
Resources and procedural APIs behaviors have survived a few attempts to
kill them. I wish we could introduce some key ones with 8 and not barely
focused in JIT and some basics cleanup :)

By procedural behaviors, I mean all these functions based on the 90s
designed C-like behaviors which are not fit anymore for today needs.

best,
Pierre

5 years ago by come@opensides.be — view source

unread

Le dimanche 10 février 2019, 09:34:16 CET Pierre Joye a écrit :

However I think you are right as well in your other replies. While I wrote
@ free code since years and I rarely see some in modern code, removing it
may bring some BC issues that could delay 8 adoptions.

Not sure where in this thread is the right place to answer, but I felt like I had to report that the php-ldap module triggers warning for about all errors and is almost impossible to use without @ because of this.
I did not attempt at fixing this for now since it would be huge BC breaks and if we’re going for BC breaks a lot of the ldap module API could be redesigned.

The other case where error handling through warning is a big problem is fopen, for which I usually use the same kind of wrapper (setting error handler) as posted by someone else.

Côme

5 years ago by Nikita Popov — view source

unread

On Wed, Feb 6, 2019 at 10:45 PM Christoph M. Becker cmbecker69@gmx.de
wrote:

As long as we have a prominent warning about this in our migration guide
alerting people to the associated risk in setups where display errors is
on

I can live with the change as-is. How do we ensure that it doesn't get
lost in the clutter given that it has no RFC?

Generally, everything that needs to be mentionend in the migration
guide, should have a respective entry in UPGRADING. That makes creating
the migration guide straight forward for the doc team, and worked quite
well for PHP 7.3 at least.

As the discussion here has drifted off towards error handling in general
and there hasn't been any further input on the change itself, I've gone
ahead and committed this:
https://github.com/php/php-src/commit/a302d1161036988fe220ecd8ecd73e6af1a116fc
I've also included a few UPGRADING notes.

Nikita

Don't silence fatal errors

-- regards / pozdrawiam,

This won't fix WSOD problem by itself, but probably will make WSODs a little less annoying to debug. Which is a win in my book.

--
regards / pozdrawiam,

This won't fix WSOD problem by itself, but probably will make WSODs a
little less annoying to debug. Which is a win in my book.