Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » Don't silence fatal errors »

Don't silence fatal errors

php.internals

Nikita Popov

7 years ago
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: 1. Code that legitimately wants to silence fatal errors for whatever reason should now use error_reporting() (or ini_set()) to do so, instead of @. 2. 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

Thomas Hruska

7 years ago
On 11/26/2018 2:42 PM, Nikita Popov 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: > > 1. Code that legitimately wants to silence fatal errors for whatever reason > should now use error_reporting() (or ini_set()) to do so, instead of @. > > 2. 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/

Nikita Popov

7 years ago
On Tue, Nov 27, 2018 at 2:20 PM Thomas Hruska <thruska@cubiclesoft.com> wrote:
> On 11/26/2018 2:42 PM, Nikita Popov 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: > > > > 1. Code that legitimately wants to silence fatal errors for whatever > reason > > should now use error_reporting() (or ini_set()) to do so, instead of @. > > > > 2. 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

Thomas Hruska

7 years ago
On 11/27/2018 8:26 AM, Nikita Popov wrote:
> On Tue, Nov 27, 2018 at 2:20 PM Thomas Hruska <thruska@cubiclesoft.com> > wrote: > >> On 11/26/2018 2:42 PM, Nikita Popov 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: >>> >>> 1. Code that legitimately wants to silence fatal errors for whatever >> reason >>> should now use error_reporting() (or ini_set()) to do so, instead of @. >>> >>> 2. 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/

Agustin Casiva

7 years ago
On Mon, Nov 26, 2018 at 6:43 PM Nikita Popov <nikita.ppv@gmail.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: > > 1. Code that legitimately wants to silence fatal errors for whatever reason > should now use error_reporting() (or ini_set()) to do so, instead of @. > > 2. 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

Claude Pache

7 years ago
> 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: > > 1. Code that legitimately wants to silence fatal errors for whatever reason > should now use error_reporting() (or ini_set()) to do so, instead of @. > > 2. 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. (BTW, it could be handy to have the following built-in constant: const E_ANY_ERROR = E_ERROR | E_CORE_ERROR | E_COMPILE_ERROR | E_USER_ERROR | E_RECOVERABLE_ERROR | E_PARSE; and ditto for warnings and notices.) —Claude

Fwentish Aelondes

7 years ago
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. On 11/26/18, Nikita Popov <nikita.ppv@gmail.com> wrote:

Michał Brzuchalski

7 years ago
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. > > On 11/26/18, Nikita Popov <nikita.ppv@gmail.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: > > > > 1. Code that legitimately wants to silence fatal errors for whatever > reason > > should now use error_reporting() (or ini_set()) to do so, instead of @. > > > > 2. 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 > > > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > >
-- regards / pozdrawiam, -- Michał Brzuchalski about.me/brzuchal brzuchalski.com

Dan Ackroyd

7 years ago
On Thu, 29 Nov 2018 at 09:59, Michał Brzuchalski <michal@brzuchalski.com> wrote:
> > 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

Christoph Becker

7 years ago
On 29.11.2018 at 12:24, Dan Ackroyd wrote:
> On Thu, 29 Nov 2018 at 09:59, Michał Brzuchalski <michal@brzuchalski.com> wrote: >> >> 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

Andrew Faulds

7 years ago
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/

Nikita Popov

7 years ago
On Mon, Nov 26, 2018 at 10:42 PM Nikita Popov <nikita.ppv@gmail.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: > > 1. Code that legitimately wants to silence fatal errors for whatever > reason should now use error_reporting() (or ini_set()) to do so, instead of > @. > > 2. 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

Zeev Suraski

7 years ago
On Tue, Feb 5, 2019 at 5:17 PM Nikita Popov <nikita.ppv@gmail.com> wrote:
> 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

Nikita Popov

7 years ago
On Tue, Feb 5, 2019 at 5:55 PM Zeev Suraski <zeev@php.net> wrote:
> On Tue, Feb 5, 2019 at 5:17 PM Nikita Popov <nikita.ppv@gmail.com> wrote: > >> 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

Zeev Suraski

7 years ago
On Tue, Feb 5, 2019 at 7:19 PM Nikita Popov <nikita.ppv@gmail.com> wrote:
> On Tue, Feb 5, 2019 at 5:55 PM Zeev Suraski <zeev@php.net> wrote: > >> On Tue, Feb 5, 2019 at 5:17 PM Nikita Popov <nikita.ppv@gmail.com> wrote: >> >>> 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

Christoph Becker

7 years ago
On 06.02.2019 at 05:42, Zeev Suraski 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.
-- Christoph M. Becker

Nikita Popov

7 years ago
On Wed, Feb 6, 2019 at 10:45 PM Christoph M. Becker <cmbecker69@gmx.de> wrote:
> On 06.02.2019 at 05:42, Zeev Suraski 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

Pierre Joye

7 years ago
Good morning Nikita, On Tue, Nov 27, 2018, 4:43 AM Nikita Popov <nikita.ppv@gmail.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: >
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

Girgias

7 years ago
On Thu, 7 Feb 2019 at 02:03, Pierre Joye <pierre.php@gmail.com> wrote:
> Good morning Nikita, > > On Tue, Nov 27, 2018, 4:43 AM Nikita Popov <nikita.ppv@gmail.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: > > > > > 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

Yasuo Ohgaki

7 years ago
On Thu, Feb 7, 2019 at 10:07 AM Girgias <george.banyard@gmail.com> wrote:
> On Thu, 7 Feb 2019 at 02:03, Pierre Joye <pierre.php@gmail.com> wrote: > > > Good morning Nikita, > > > > On Tue, Nov 27, 2018, 4:43 AM Nikita Popov <nikita.ppv@gmail.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: > > > > > > > > > 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

Pierre Joye

7 years ago
On Thu, Feb 7, 2019, 8:07 AM Girgias <george.banyard@gmail.com wrote:
> > 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

Christian Schneider

7 years ago
Am 07.02.2019 um 02:32 schrieb Pierre Joye <pierre.php@gmail.com>:
> On Thu, Feb 7, 2019, 8:07 AM Girgias <george.banyard@gmail.com wrote: >> 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

Pierre Joye

7 years ago
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>: > > On Thu, Feb 7, 2019, 8:07 AM Girgias <george.banyard@gmail.com wrote: > >> 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

Stas Malyshev

7 years ago
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

Pierre Joye

7 years ago
HI Stas, On Sun, Feb 10, 2019, 8:17 AM Stanislav Malyshev <smalyshev@gmail.com wrote:
> 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

Côme Chilliet

7 years ago
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

Benjamin Morel

7 years ago
> > 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. On Thu, 7 Feb 2019 at 02:32, Pierre Joye <pierre.php@gmail.com> wrote:

Christian Schneider

7 years ago
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

Benjamin Morel

7 years ago
> > 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:

Rowan Collins

7 years ago
On 7 February 2019 11:19:46 GMT+00:00, Benjamin Morel <benjamin.morel@gmail.com> wrote:
>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]

Christian Schneider

7 years ago
Am 07.02.2019 um 14:28 schrieb Rowan Collins <rowan.collins@gmail.com>:
> On 7 February 2019 11:19:46 GMT+00:00, Benjamin Morel <benjamin.morel@gmail.com> wrote: >> 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

Stas Malyshev

7 years ago
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