Re: [PHP-DEV] [RFC][DISCUSSION] Error Exceptions mode

This is only part of a thread. view whole thread
  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.