[VOTE] Catching Multiple Exception Types

If they are user defined exceptions, you should make them implement an
interface,

Even within one oranisation, getting different teams to agree to
common interfaces is not always possible. But when the exceptions are
originating in code that people don't have any control over (e.g.
anything open source) then changing the exceptions to match your
particular interface requirements is a completely impossible.

For example will you and Marco start taking requests to add particular
interfaces to any exceptions thrown by the code in the libraries you
maintain?

Derick wrote:

As you don't know which exception
class you have, how would you know how to handle each separately.

Why do you think you need to know how to handle each separately? If
you ever need to do something different based on the type of
exception, you probably wouldn't be using the multiple-catch. The
multiple-catch is almost only ever going to be used when you have a
list of exceptions where you're going to be doing the same thing for
each. e.g. for retrying an operation:

$success = false;
while ($attempts < 4) {
try {
foo();
$success = true;
}
catch (NetworkException | DBDeadLockException $e) {
// Save exceptions to add to surpressed exception
// when that RFC is hopefully passed.
$attempts++;
}
// All other
}
//Check for success or run out of attempts here.

For NetworkException and DBDeadLockException we're always going to do
the same thing; retry the operation a couple of times. For any
exception type where we would want to do something different......we'd
just use a separate catch block.

cheers
Dan

Hi!

Even this is a scary way to do it. As you don't know which exception
class you have, how would you know how to handle each separately. You'd
have to if ($caught instanceof InvalidArgumentException) etc to be sure
that what you're doing with the object is allowed.

I just recently had a case where I used something similar in Java code,
so I can tell when it's useful:

• You call some complex object that can fail for a lot of reasons (like
  HTTP client talking to remote service)
• It can fail with a number of exceptions because something went bad -
  i.e. network is bad, DNS is bad, server is down, service is too busy,
  your request syntax is wrong, request is correct but you're not allowed
  to execute it, etc.
• These causes can be roughly separated into two groups - requests that
  we can and should repeat - e.g. "server too busy" situation, and those
  that we should not repeat, e.g. "bad syntax" situation
• Furthermore, failures can be "soft" - fail this request but continue
  next ones in the batch, or "hard" - drop everything and call for help
  because something is seriously broken.

In this situation, grouping exceptions - which, due to being from
different layers of the complex object do not follow any single
structure - becomes very convenient. Of course it is possible to write a
long tree of copy-paste handlers for each of the three cases, but it's
annoying. Much cleaner to have just three branches. And of course I
don't want to do something like catch(Exception) because shudder...

say, it's very likely they should be handled differenty anyway. Or
rather, very generically, which you can do with "catch (Exception $e)".

catch(Exception) is practically never a good idea, unless you really are
able to handle every exception. Now in PHP that includes parse errors,
type errors, etc. I don't think any random code - except for a very
generic final error handler - would ever want to do that.

Stas Malyshev
smalyshev@gmail.com

-----Original Message-----
From: Derick Rethans [mailto:derick@php.net]
Sent: Monday, April 18, 2016 6:18 PM
To: Marco Pivetta ocramius@gmail.com
Cc: Dan Ackroyd danack@basereality.com; Bronisław Białek
after89@gmail.com; PHP internals internals@lists.php.net
Subject: Re: [PHP-DEV] [VOTE] Catching Multiple Exception Types

Heya,

I voted "NO" due to previous discussion. TL;DR: this is FAR off the
80/20 use-case for a language syntax change.

I'm with you on this one.

C&P from my answer elsewhere:

The following typical example is something REALLY rare, and requiring
a parser change for it seems excessive:

try {
    // ...
} catch (InvalidArgumentException $e) {
    // same handling
} catch (PDOException $e) {
    // same handling
} catch (BadMethodCallException $e) {
    // same handling
}

These 3 exceptions usually result in separate handling anyway. If same
handling is needed, you can as usual extract a private method (if you
are in a class) and deal with it there:

try {
    // ...
} catch (InvalidArgumentException $e) {
    $this->sameHandling($e);
} catch (PDOException $e) {
    $this->sameHandling($e);
} catch (BadMethodCallException $e) {
    $this->sameHandling($e);
}

private function sameHandling(Throwable $caught)
{
    // same handling
}

Even this is a scary way to do it. As you don't know which exception class you
have, how would you know how to handle each separately.

In reality, you often don't truly 'handle' exceptions at all. Different exception types are very often just a slightly fancier and easier way to return errors, and it's actually very common and perfectly reasonable to handle groups of exceptions in an identical way. It's also extremely common to see exception handlers that don't pay any attention to the exception object itself. In fact, glancing through the Zend Framework 2 codebase and the Drupal 8 codebase, it seems that the exception object is rarely ever used.

In Drupal 8, many catch blocks simply resort to return FALSE or TRUE. For example:

try {
  return $router->match($path);
}
catch (ResourceNotFoundException $e) {
  return FALSE;
}
catch (ParamNotConvertedException $e) {
  return FALSE;
}
catch (AccessDeniedHttpException $e) {
  return FALSE;
}
catch (MethodNotAllowedException $e) {
  return FALSE;
}

}

Note that this is in fact different from catch (Exception $e), and I'm guessing the author intentionally did not want to catch an exception the type of which she/he weren't expecting to get - and instead, have it propagate upwards.

With this RFC approved, it would become:

try {
  return $router->match($path);
}
catch (ResourceNotFoundException|ParamNotConvertedException|AccessDeniedHttpException|MethodNotAllowedException $e) {
  return FALSE;
}

Which is much, much nicer and readable. And I found this in the ZF2 codebase:

             } catch (DiRuntimeException $e) {
                 if ($methodRequirementType & self::RESOLVE_STRICT) {
                     //finally ( be aware to do at the end of flow)
                     array_pop($this->currentDependencies);
                     if (isset($alias)) {
                         array_pop($this->currentAliasDependenencies);
                     }
                     // if this item was marked strict,
                     // plus it cannot be resolve, and no value exist, bail out
                     throw new Exception\MissingPropertyException(
                         sprintf(
                             'Missing %s for parameter ' . $name . ' for ' . $class . '::' . $method,
                             (($value[0] === null) ? 'value' : 'instance/object')
                         ),
                         $e->getCode(),
                         $e
                     );
                 } else {
                     //finally ( be aware to do at the end of flow)
                     array_pop($this->currentDependencies);
                     if (isset($alias)) {
                         array_pop($this->currentAliasDependenencies);
                     }
                     return false;
                 }
             } catch (ServiceManagerException $e) {
                 // Zend\ServiceManager\Exception\ServiceNotCreatedException
                 if ($methodRequirementType & self::RESOLVE_STRICT) {
                     //finally ( be aware to do at the end of flow)
                     array_pop($this->currentDependencies);
                     if (isset($alias)) {
                         array_pop($this->currentAliasDependenencies);
                     }
                     // if this item was marked strict,
                     // plus it cannot be resolve, and no value exist, bail out
                     throw new Exception\MissingPropertyException(
                         sprintf(
                             'Missing %s for parameter ' . $name . ' for ' . $class . '::' . $method,
                             (($value[0] === null) ? 'value' : 'instance/object')
                         ),
                         $e->getCode(),
                         $e
                     );
                } else {
                    //finally ( be aware to do at the end of flow)
                    array_pop($this->currentDependencies);
                    if (isset($alias)) {
                        array_pop($this->currentAliasDependenencies);
                    }
                    return false;
                }
            }

To save everyone some time, the two code blocks for the two exception types are completely identical. I'm not sure why it wasn't function()alized, but regardless, it's another case where the new proposed feature would make perfect sense.

I think saying that identical handling for multiple exception types is bad or is grounds for an urgent code review is fairly disconnected from reality in many if not most cases.

Zeev

Hi!

catch constitutes a formal signature, exactly like a function signature. In
my mind, this proposal and the union types proposal are intrinsically tied.
If we have one, we must have the other. If not, we're inducing a sadness.

This has nothing to do with union types. It is just a shortcut for
repeated catch() clauses, it does not introduce anything into type
system, capture existing use case, and doesn't make anything more
complicated. While union types introduce a lot of new things, make
everything dealing with types more complicated and the need for them,
unless we import a real lot of other concepts like type pattern matching
is quite unclear. So I don't think this has much in common.

Stas Malyshev
smalyshev@gmail.com