Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:83108
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: error (pb1.pair.com: domain basereality.com from 209.85.160.179 cause and error)
MIME-Version: 1.0
In-Reply-To: <CAF+90c8NAROuCDgRajF4AN2oTc4fzmxTjtVC_OAQH=ZFm5N9Uw@mail.gmail.com>
References: <CAF+90c-7w1fLqmffYvPRhmtt8rzESEpSmW8vfMD-YfLt1mG_8w@mail.gmail.com>
	<CAF+90c8NAROuCDgRajF4AN2oTc4fzmxTjtVC_OAQH=ZFm5N9Uw@mail.gmail.com>
Date: Wed, 18 Feb 2015 19:30:15 +0000
Message-ID: <CA+kxMuREL5RfEyB7cJhQ6PK+zHsb2wLVcaWxB=0KveYRrRYSsg@mail.gmail.com>
To: Nikita Popov <nikita.ppv@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Re: [RFC] Exceptions in the engine
From: danack@basereality.com (Dan Ackroyd)

On 13 February 2015 at 23:25, Nikita Popov <nikita.ppv@gmail.com> wrote:
> Subclassing: Should there be more specific subclasses of EngineException
> for particular errors?

It's not obvious that any subclasses would be useful. However using
the code to specify the exact type of error, rather than having to
inspect the message would be good.

> Should EngineException inherit from Exception (and as such be
> subject to catch(Exception)) or should we introduce some kind
> of special super-class that is not caught by default

Even ignoring the BC problem with having EngineExceptions extending
Exception, I think the EngineException needs to be in a different
hierarchy to Exception to be able to write reasonable code in the
future

Without having EngineException in a separate hierarchy of exceptions,
the code below will catch exceptions where the data is 'ok' but there
was a problem with the code, and continue to process items. This is
almost certainly not the correct behaviour when an EngineException has
been encountered.

interface Service {
    function foo($item);
}


function processData(array $itemsToProcess, service $service) {
    foreach ($itemsToProcess as $item) {
        try {
            $service->foo($item);
        }
// Because $service can throw an Exception that is specific to the
// implementation we have to catch \Exception, unless we are going
// to list all possible implementation specific exception types here.
// That would be a subtle case of strong coupling, and when a new
// implementation is made the new exception type would need to
// be added here.
        catch(\Exception $e) {
            // item was not processable but PHP engine is OK.
            $item->markAsErrored();
            //Go on to process the next item
        }
    }
}


To avoid having EngineExceptions in a separate hierarchy, this
function could be converted to:

function processData(array $itemsToProcess, service $service) {
    foreach ($itemsToProcess as $item) {
        try {
            $service->foo($item);
        }
        catch(\EngineException $ee) {
            //PHP engine is not stable - lets get out of here.
            throw $ee; //or throw new ProcessException($ee)
        }
        catch(\Exception $e) {
            $item->markAsErrored();
        }
    }
}

However that is bad as i)it's boiler plate to do the correct behaviour
ii) you have to remember to do that everywhere. Having to remember to
do the correct thing, is going to lead to people forgetting.

It will still be necessary to catch all types of Exception in a single
catch block i.e. at the top level of a script to prevent exceptions
being shown to the end user. This could be made easier by having a
common super class for Exception and EngineException. However having
one try block that is required to have multiple catch statements to
catch all different types of exception is not that much of a burden:

try {
    runApp();
}
catch(EngineException $e) {
    handleException($ee);
}
catch(Exception $e) {
    handleException($e);
}

As that would be the only place it would be required to catch both types.

TL:DR EngineException needs to not extend Exception, whether we need a
super class is not as clear.

cheers
Dan