[RFC][DISCUSSION] Error Exceptions mode

  110254
May 22, 2020 16:08 iggyvolz@gmail.com (Katie Volz)
Hello internals,

I want to start a discussion on an RFC to add a declare() statement to
convert all errors triggered within a file to exceptions.

Currently, the only way to handle notices/warnings in an exception-like
manner is via set_error_handler, (for example, example #1 on
https://www.php.net/errorexception) - however this has several
disadvantages (in particular, it cannot be safely used in libraries without
affecting other libraries).  The declare() would only affect code on the
particular PHP file for which the declare was set to 1, in the same way
that strict_types only affects code on a particular PHP file.  I have
listed error_exception, error_exceptions, and strict_errors as potential
names for the declare(), although I don't feel that any of them clearly and
succinctly describe what is happening here (if anyone has a better name I'm
certainly interested).

As I do not have wiki karma, I have posted a draft RFC here:
https://github.com/iggyvolz/php-rfcs/blob/master/error-exceptions.txt.  A
partially complete patch is linked at the bottom of the draft (very much
based off the strict_types patch).

Thanks,
Katie Volz
  110256
May 22, 2020 16:56 rowan.collins@gmail.com (Rowan Tommins)
On 22/05/2020 17:08, Katie Volz wrote:
> I want to start a discussion on an RFC to add a declare() statement to > convert all errors triggered within a file to exceptions.
Hi Katie, Personally, I'm not a fan of promoting messages to exceptions in this way, because APIs designed to throw exceptions generally look rather different from ones designed to warn and continue, so blindly converting seems like putting a square peg in a round hole. However, I know people do it already, so will give my thoughts on the principle. My main concern is that it needs a tighter definition of "error", and possibly a configurable one. The snippet from the manual which you include in your draft RFC references the current global error_reporting setting. This makes sense with a global error handler, but less so for a per-file declare - it means the caller can choose whether exceptions are thrown, somewhat defeating the point. Perhaps the declare directive could take an error level as its argument, e.g. declare(error_exceptions=E_ALL-E_NOTICE)? Relatedly, you would need to define how this interacts with the @ (error suppression or "shut up") operator - would that force the code to run past a point that would otherwise throw an exception? Again, I think that choice should be taken away from the caller, otherwise the author needs to account for both modes, e.g. declare(error_exceptions=E_WARNING); try {     $fh = fopen($blah, $mode);     // if caller can suppress ErrorExceptions, we still need to check if $fh is false     // ... } catch ( ErrorException $e ) {     // the caller shouldn't actually care what happens, because we can catch it here } Other than that, there's the usual awkwardness of declare - it has to be set in each file, and new directives are errors in old versions of PHP. That doesn't necessarily mean we shouldn't use it for now, though. Regards, -- Rowan Tommins (né Collins) [IMSoP]
  110271
May 24, 2020 21:07 iggyvolz@gmail.com (Katie Volz)
On Fri, May 22, 2020 at 12:56 PM Rowan Tommins collins@gmail.com>
wrote:

> Personally, I'm not a fan of promoting messages to exceptions in this > way, because APIs designed to throw exceptions generally look rather > different from ones designed to warn and continue, so blindly converting > seems like putting a square peg in a round hole. However, I know people > do it already, so will give my thoughts on the principle. >
To this (and also to George's point) - I definitely agree that this isn't a perfect solution. I see this as more of a "stopgap" for people who prefer exception-style handling rather than having to do both.
> My main concern is that it needs a tighter definition of "error", and > possibly a configurable one. > > The snippet from the manual which you include in your draft RFC > references the current global error_reporting setting. This makes sense > with a global error handler, but less so for a per-file declare - it > means the caller can choose whether exceptions are thrown, somewhat > defeating the point. Perhaps the declare directive could take an error > level as its argument, e.g. declare(error_exceptions=E_ALL-E_NOTICE)? >
I hadn't thought putting the error types in the declare(). My only issue with that is how that would interact with the error_reporting setting - for example if I set my error_reporting to E_WARNING and error_exceptions to E_ALL I'm not sure whether a notice would be thrown. As far as a tighter definition of error - I've kept it loose in the draft as I'm not sure how exactly to define errors which can and can't be processed at runtime. For example, a compile error cannot be caught at runtime, nor can (at least in my patch) warnings which are issued before current_execute_data is populated (that is the object which I read to check if the flag is set).
> Relatedly, you would need to define how this interacts with the @ (error > suppression or "shut up") operator - would that force the code to run > past a point that would otherwise throw an exception? Again, I think > that choice should be taken away from the caller, otherwise the author > needs to account for both modes, e.g. > > declare(error_exceptions=E_WARNING); > try { > $fh = fopen($blah, $mode); > // if caller can suppress ErrorExceptions, we still need to check > if $fh is false > // ... > } catch ( ErrorException $e ) { > // the caller shouldn't actually care what happens, because we can > catch it here > } >
The error suppression operator should behave as you've described it. In a file with declare(), no warnings will ever be issued, meaning that a suppression operator on a function defined with the flag (or, for that matter, within such a file itself) will never have an effect. I will clarify this in the RFC as well as add a test for that case. On Fri, May 22, 2020 at 1:22 PM G. P. B. banyard@gmail.com> wrote:
> I did a similar PoC last summer (see: > https://github.com/php/php-src/pull/4549) > where Nikita commented that a lot of those warnings could/should be > promoted > to error regardless, and this is what myself and a couple of other people > have > been during for the past year. >
From what I can see your approach is slightly different than mine - this does not manually change any function's error behaviour but removes the warning system entirely. As we saw in https://wiki.php.net/rfc/engine_warnings - there is still (and may always be) significant pushback on moving errors to exceptions. As mentioned above, I see this as a "stopgap" for authors who do not want to deal with the error system in any way.
  110272
May 24, 2020 23:28 rowan.collins@gmail.com (Rowan Tommins)
On 24/05/2020 22:07, Katie Volz wrote:
> I hadn't thought putting the error types in the declare(). My only issue > with that is how that would interact with the error_reporting setting - for > example if I set my error_reporting to E_WARNING and error_exceptions to > E_ALL I'm not sure whether a notice would be thrown.
In my mind error_reporting should make absolutely no difference. As you say, this option would be for people who don't want to interact with the current error system in any way, and that includes its global configuration state. For instance, imagine you wrote this: declare(error_exceptions=E_WARNING); $fh = fopen($filePath, 'r'); // code using $fh The point of using the declare here would be to avoid checking if $fh is false, since any errors opening the file would become exceptions. If error_reporting(0) can turn off that exception, you'll have to check for false anyway, defeating the purpose. Regards, -- Rowan Tommins (né Collins) [IMSoP]
  110282
May 26, 2020 17:12 iggyvolz@gmail.com (Katie Volz)
On Sun, May 24, 2020 at 7:29 PM Rowan Tommins collins@gmail.com>
wrote:

> In my mind error_reporting should make absolutely no difference. As you > say, this option would be for people who don't want to interact with the > current error system in any way, and that includes its global > configuration state. >
My main concern for this was that it would make implementation more difficult - however it turned out to be fairly straightforward. One note on this - currently declare() statements do not allow constants or expressions in them. I had to patch zend_compile.c to allow this, so this will affect other declare statements as well (for example, declare(ticks=E_ALL - 37) will be valid). Unknown constants or incorrect types (ex: declare(error_exception=[]), declare(error_exception=E_ALL - FOO_BAR)) will fail here.
  110257
May 22, 2020 17:22 george.banyard@gmail.com ("G. P. B.")
On Fri, 22 May 2020 at 18:09, Katie Volz <iggyvolz@gmail.com> wrote:

> Hello internals, > > I want to start a discussion on an RFC to add a declare() statement to > convert all errors triggered within a file to exceptions. > > Currently, the only way to handle notices/warnings in an exception-like > manner is via set_error_handler, (for example, example #1 on > https://www.php.net/errorexception) - however this has several > disadvantages (in particular, it cannot be safely used in libraries without > affecting other libraries). The declare() would only affect code on the > particular PHP file for which the declare was set to 1, in the same way > that strict_types only affects code on a particular PHP file. I have > listed error_exception, error_exceptions, and strict_errors as potential > names for the declare(), although I don't feel that any of them clearly and > succinctly describe what is happening here (if anyone has a better name I'm > certainly interested). > > As I do not have wiki karma, I have posted a draft RFC here: > https://github.com/iggyvolz/php-rfcs/blob/master/error-exceptions.txt. A > partially complete patch is linked at the bottom of the draft (very much > based off the strict_types patch). > > Thanks, > Katie Volz >
I did a similar PoC last summer (see: https://github.com/php/php-src/pull/4549) where Nikita commented that a lot of those warnings could/should be promoted to error regardless, and this is what myself and a couple of other people have been during for the past year. There are still extensions which didn't receive this treatment mostly due to lack of time and hands, so if you want to join on this feel free to send a PR to php-src Due to the work already done, I personally see less interest in such a declare. Mostly because doing a promotion of these warnings, which are usually tied in to a failure return value such as null or false, allows us in some instance to "clean" up the return value, but only if it's done on the extension level/in core. Now I do acknowledge the number of E_WARNINGs still present on master is large, but I'd personally prefer to focus on making the language better for everyone and let legit warnings be warnings mostly due to the legacy behaviour for I/O operations. Best regards George P. Banyard
  110316
May 30, 2020 17:56 Danack@basereality.com (Dan Ackroyd)
On Fri, 22 May 2020 at 17:09, Katie Volz <iggyvolz@gmail.com> wrote:
> > Hello internals, > > I want to start a discussion on an RFC to add a declare() statement to > convert all errors triggered within a file to exceptions. >
Hi Katie/Iggy, I'm not sure this is the right thing to do. Having internal code just throw generic ErrorExceptions does not get PHP to a place where it is possible for users to listen for individual error types, which I think is the biggest problem with the current situation. I won't duplicate my message from another thread, but I think there is an alternative path: https://news-web.php.net/php.internals/110315
> The error handler cannot be set in library code without affecting other libraries
We could also just make the default error handler convert all errors/warnings to exceptions.....I've been doing that for my error handlers for years, and I believe it's the right thing to do for 95%+ of applications. Though it does require some use of the silence operator, which apparently some people find disconcerting. cheers Dan Ack