Throwable

[sebastianbergmann]

Message-ID: <54F07FC7.8050901@php.net>
Date: Fri, 27 Feb 2015 15:31:35 +0100
From: Sebastian Bergmann <sebastian@php.net>
To: internals@lists.php.net
References: <CAF+90c8nqCBGWpsqdfy+EKekOm3Cw5_zzE_nKeyLK5pzROTueg@mail.gmail.com>
In-Reply-To: <CAF+90c8nqCBGWpsqdfy+EKekOm3Cw5_zzE_nKeyLK5pzROTueg@mail.gmail.com>
Subject: Re: [PHP-DEV] [VOTE] Exceptions in the engine

Am 23.02.2015 um 19:15 schrieb Nikita Popov:
> Voting on the engine exceptions RFC, which proposes to convert existing
> fatal and recoverable fatal errors into exceptions, has opened:
> 
>     https://wiki.php.net/rfc/engine_exceptions_for_php7#vote
> 
> The primary vote requires a 2/3 majority, as this is a language change.
> 
> A second vote will decide whether to use a BaseException based inheritance
> hierarchy. This vote uses a simple majority.

 I have voted yes on "Allow exceptions in the engine and conversion of
 existing fatals?" and no on "Introduce and use BaseException?" and
 would like to elaborate on the latter.

 I am sorry that I was unable to raise this concern earlier (did not
 really become aware of the RFC before it was put to the vote), but I
 would prefer the following:

   * Introduce a Throwable interface
   * Let Exception implement the Throwable interface
   * Introduce an Error class that implements the Throwable interface
   * Use Error class as base class for exceptions raised by the engine

 This would be along the lines of what Java does.

-- 
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php

[staabm]

indent

[beberlei]

its spaces, wheras the code needs to be with tabs.

[sebastianbergmann]

Would be great if we could focus on conceptual problems and implementation issues before critiquing whitespace.

[ircmaxell]

I'm not sure about the naming change from Exception to Error.

Could you elaborate a bit more on the semantics of Error? Is user-land allowed to raise errors? Can they extend them? Etc?

[Wes0617]

Also, note that Java's exception hiearchy is sh*t. Checked errors are a super class of unchecked ones:

catch(Exception amICheckedOrUnchecked){
    if(amICheckedOrUnchecked instanceof RuntimeException){
        throw amICheckedOrUnchecked; // sorry this can't be handled
    }
    // actual handling here -__-
}

[Kubo2]

@sebastianbergmann Anyway, +1 for the Throwable interface.

[nikic]

What's the reason for having a Throwable interface if in the end you're required to use BaseException anyway?

[Kubo2]

@nikic So what's the reason for having a BaseException? If it is only supposed to be the base class of all engine exceptions, it could be at least named differently. (But I am personally not very familiar with the concept of BaseException.)

[trowski]

Linking to the relevant RFC: https://wiki.php.net/rfc/throwable

Based on the above RFC, the goal is to make Throwable like Traversable and remove BaseException.

Throwable could be required for an object to be thrown like Traversable is required to for an object to be used in foreach. Error and Exception would both implement Throwable. If code wanted to catch any exception, it could catch Throwable. Seems a little cleaner and nicer than BaseException.

[trowski]

I looked into how separate exception classes could be implemented with only a common interface and it seems like there would be a lot of unnecessary duplication. Having BaseException makes a lot of sense from an implementation standpoint. It seems the main problem is that people just don't like the name.

Perhaps a hierarchy like the one below would be more acceptable? This is similar to what has already been implemented in PHP 7, except BaseException would become Throwable. Throwable classes not extending Exception would instead extend Error and use Error as a suffix. Problems that raised errors in PHP5 would throw classes extending Error, making the separation a little clearer.

• Throwable (abstract class, not an interface)
  • Exception
    • ErrorException
    • RuntimeException
    • LogicException
    • ...
  • Error
    • EngineError
      • TypeError
    • ParseError

---

Edit: This was so straightforward to implement, I quickly did it here: https://github.com/trowski/php-src/tree/throwable

Many tests would need to be updated yet, but for the most part find and replace would do the job.

[dzuelke]

IMO the "new" error exceptions, be that ParseError or a fatal, should not be named ...Exception if they don't extend the base Exception, or it'll be super confusing for users why a catch(Exception $e) doesn't catch those.

And at the same time and for reasons of BC, EngineException obviously can't extend Exception or a lot of old stuff would break.

The current hierarchy, where you have Exception, EngineException and ParseException extend from BaseException is a bit messed up, because the last two should have a common superclass.

That part isn't apparent from https://wiki.php.net/rfc/throwable; it says "Error class" in the title, but only mentions that both EngineException and ParseException should implement Throwable, so the common Error superclass is probably still up for debate.

I like ...Error in the class name(s), because it denotes the semantic difference (an error thrown by the engine, not an error thrown by code it executed, be that PHP or C code) quite well.

And with an Error "base" exception, you probably don't need an EngineError anymore either, unless you want to differentiate between "runtime" and "parse time" errors.

Thusly:

• interface Throwable
• class Exception implements Throwable
  • class 🐙Exception extends Exception
  • ...
• class Error implements Throwable
  • class TypeError extends Error
  • class ParseError extends Error

Or:

• interface Throwable
• class Exception implements Throwable
  • class 🐙Exception extends Exception
  • ...
• class Error implements Throwable
  • class RuntimeError extends Error
    • class TypeError extends RuntimeError
  • class ParseError extends Error

[Wes0617]

@dzuelke i agree that Error suffix must be adopted, but i don't agree on the hiearchy. say you want to catch a fatal error to log it / send it to debugger, ok?
what would you write, catch(Throwable $e){} or catch(Error $e){} ?

[dzuelke]

Yeah, catch (Error $e), @WesNetmo - that's why I have that in there.

[dzuelke]

If Throwable were to be an internal interface (like Traversable), userland code would not be allowed to implement it.

That's a good thing, IMO.

[Wes0617]

it doesn't make sense to make a distinction between exceptions thrown by the engine and those that are user-defined

[trowski]

it doesn't make sense to make a distinction between exceptions thrown by the engine and those that are user-defined

Unfortunately we probably should for BC reasons. Exceptions thrown by the engine shouldn't directly extend Exception because they would cause BC issues, being unintentionally caught by catch (Exception $e). Since this distinction has been made, I think it's better to make the distinction clearer rather than try and obfuscate it and cause confusion for users.

[morrisonlevi]

Just to document my opinion somewhere: I think anyone doing catch(Exception $e) would want to catch any exception. Since it currently catches all possible exceptions it would be a BC break for it to no longer catch all possible exceptions.

It's a BC break either way. Just make them all extend Exception and preserve existing catch-all behaviors.

[morrisonlevi]

No pre-PHP 7 code could have been written with the expectation that catch (Exception $e) would catch parse errors.

No, but they were written with the expectation they would catch all exceptions, both current and future.

[morrisonlevi]

PHP 7 is a new major version. Some things are expected to break. Having a clean exception hierarchy is more important than not interfering with some edge cases.

Please provide an logical argument of how it is more "clean" than if EngineException extended Exception, and be sure to include BC impact of both stance since breaking BC is messy (not clean).

[dzuelke]

@morrisonlevi Because the try block in millions of lines of code out there also contains calls that may cause fatal errors. These would have lead to the script dying until now. Under PHP7, suddenly, the catch block would be invoked instead if the "engine error" exceptions were subclasses of Exception. That's a massive BC break. The code was never written with the expectation that calls that are not thrown as exceptions suddenly turn into exceptions.

[Kubo2]

@trowski: Why Throwable wouldn't be an interface but a class? I don't like
the idea having other parent for exceptions than actual Exception except it
would be an interface

2015-05-16 7:08 GMT+02:00 Aaron Piotrowski notifications@github.com:

I looked into how separate exception classes could be implemented with
only a common interface and it seems like there would be a lot of
unnecessary duplication. Having BaseException makes a lot of sense from
an implementation standpoint. It seems the main problem is that people just
don't like the name.

Perhaps a hierarchy like the one below would be more acceptable? This is
similar to what has already been implemented in PHP 7, except
BaseException would become Throwable. Throwable classes not extending
Exception would instead extend Error and use Error as a suffix. Problems
that raised errors in PHP5 would throw classes extending Error, making
the separation a little clearer.

• Throwable (abstract class, not an interface)
  • Exception
    • ErrorException
    • RuntimeException
    • LogicException
    • ...
      • Error
    • EngineError
      • TypeError
      • ParseError

—
Reply to this email directly or view it on GitHub
#1274 (comment).

[trowski]

@Kubo2 It was easier to implement as an abstract class, but I changed my mind and made another version where Throwable is an interface instead: #1284

[nikic]

Closing in favor of #1284, which has been accepted.