On Thu, Feb 19, 2015 at 6:14 PM, Trevor Suarez <ric...@gmail.com> wrote:
> I think that structure makes sense Dmitry. > > Though, just a bit on naming here (I know, its not a big deal, and naming > is hard, haha): > > I think that naming the new parent exception something like "Throwable" or > "Catchable" (as Nikita previously suggested) would be a bit more concise in > meaning than "BaseException". You may not have even meant that name as a > formal proposal, but I figured I'd weigh in regardless. :P > We thought about "Throwable" or "Catchable" interface, but this change would require more changes and will make more BC breaks. Thanks. Dmitry. > > - Trevor > > > On Thu Feb 19 2015 at 4:55:38 AM Dmitry Stogov <dmi...@zend.com> wrote: > >> On Wed, Feb 18, 2015 at 10:30 PM, Dan Ackroyd <dan...@basereality.com> >> wrote: >> >> > On 13 February 2015 at 23:25, Nikita Popov <nikita....@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 >> > >> > -- >> > PHP Internals - PHP Runtime Development Mailing List >> > To unsubscribe, visit: http://www.php.net/unsub.php >> > >> > >> I think we may introduce the following hierarchy >> >> abstarct class BaseException { >> } >> class Exception extends BaseException { >> } >> class EngineException extends BaseException { >> } >> >> the existing code that caught Exception is going to be unaffected. >> New code may decide to catch engine exception in separate catch block >> (using EngineException) or in single block (using BaseException) >> >> Thanks. Dmitry. >> >