Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:92446
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain zend.com designates 157.56.110.107 as permitted sender)
To: Derick Rethans <derick@php.net>, Marco Pivetta <ocramius@gmail.com>
CC: PHP internals <internals@lists.php.net>
Thread-Topic: [PHP-DEV] [VOTE] Catching Multiple Exception Types
Thread-Index: AQHRmMlr+2y+N0CM4kmdEfDlLDJpZJ+OyqWAgADzowCAABsIgIABF91w
Date: Tue, 19 Apr 2016 09:56:09 +0000
Message-ID: <BY2PR02MB298461D66EA1A39720B93CBAC6C0@BY2PR02MB298.namprd02.prod.outlook.com>
References: <CADFyporyFgnMgetcEj8FZUFs1Lbc_VidBsxg5jBE7J4_OFzKTQ@mail.gmail.com>
 <CA+kxMuTFfqHZa4sXNv=Cfcm8w6eHdkKSGYiPdUiTJ1DGd6_VQQ@mail.gmail.com>
 <CADyq6sJP_BYEmmTWzJ4OvhcM9uto2EY+vefZTKovAaKBA8hs7w@mail.gmail.com>
 <alpine.DEB.2.20.1604181614100.3391@whisky.home.derickrethans.nl>
In-Reply-To: <alpine.DEB.2.20.1604181614100.3391@whisky.home.derickrethans.nl>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:23
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="iso-8859-2"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Apr 2016 09:56:09.9293
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 32210298-c08b-4829-8097-6b12c025a892
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY2PR02MB300
Subject: RE: [PHP-DEV] [VOTE] Catching Multiple Exception Types
From: zeev@zend.com (Zeev Suraski)



> -----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=B3aw Bia=B3ek
> <after89@gmail.com>; PHP internals <internals@lists.php.net>
> Subject: Re: [PHP-DEV] [VOTE] Catching Multiple Exception Types
>=20
> On Mon, 18 Apr 2016, Marco Pivetta wrote:
>=20
> > Heya,
> >
> > I voted "NO" due to previous discussion. TL;DR: this is FAR off the
> > 80/20 use-case for a language syntax change.
>=20
> I'm with you on this one.
>=20
> > 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
> >     }
>=20
> Even this is a scary way to do it. As you don't know which exception clas=
s you
> have, how would you know how to handle each separately.

In reality, you often don't truly 'handle' exceptions at all.  Different ex=
ception types are very often just a slightly fancier and easier way to retu=
rn 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 s=
ee exception handlers that don't pay any attention to the exception object =
itself.  In fact, glancing through the Zend Framework 2 codebase and the Dr=
upal 8 codebase, it seems that the exception object is rarely ever used.=20

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 gues=
sing the author intentionally did not want to catch an exception the type o=
f which she/he weren't expecting to get - and instead, have it propagate up=
wards.

With this RFC approved, it would become:

    try {
      return $router->match($path);
    }
    catch (ResourceNotFoundException|ParamNotConvertedException|AccessDenie=
dHttpException|MethodNotAllowedException $e) {
      return FALSE;
    }

Which is much, much nicer and readable.  And I found this in the ZF2 codeba=
se:

                 } 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 . ' fo=
r ' . $class . '::' . $method,
                                 (($value[0] =3D=3D=3D null) ? 'value' : 'i=
nstance/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\ServiceNotCreatedExce=
ption
                     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 . ' fo=
r ' . $class . '::' . $method,
                                 (($value[0] =3D=3D=3D null) ? 'value' : 'i=
nstance/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, bu=
t regardless, it's another case where the new proposed feature would make p=
erfect 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