Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:92582
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 74.125.82.48 as permitted sender)
MIME-Version: 1.0
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> <BY2PR02MB298461D66EA1A39720B93CBAC6C0@BY2PR02MB298.namprd02.prod.outlook.com>
In-Reply-To: <BY2PR02MB298461D66EA1A39720B93CBAC6C0@BY2PR02MB298.namprd02.prod.outlook.com>
Date: Wed, 20 Apr 2016 22:16:11 +0000
Message-ID: <CACwt+Jfoat-3C1QBPEgc-mR5-jQnLBQxcs-6=LJmOMfuGyy3iA@mail.gmail.com>
To: Zeev Suraski <zeev@zend.com>, Derick Rethans <derick@php.net>, Marco Pivetta <ocramius@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a114cbc648f1fee0530f1f11a
Subject: Re: [PHP-DEV] [VOTE] Catching Multiple Exception Types
From: patrickallaert@php.net (Patrick ALLAERT)

--001a114cbc648f1fee0530f1f11a
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Zeev,

Le mar. 19 avr. 2016 =C3=A0 11:56, Zeev Suraski <zeev@zend.com> a =C3=A9cri=
t :

> In Drupal 8, many catch blocks simply resort to return FALSE or TRUE.  Fo=
r
> 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;
>     }
>   }
>

This looks much more like Exceptions used for not "Exceptional" cases:
where Exceptions are preferred over a clean API and avoiding at all price
if-conditions.

One of the very reason I voted "no".

It encourages people to replace any kind of condition with an exception,
leading to less readable code and incomplete API.


>     try {
>       return $router->match($path);
>     }
>     catch
> (ResourceNotFoundException|ParamNotConvertedException|AccessDeniedHttpExc=
eption|MethodNotAllowedException
> $e) {
>       return FALSE;
>     }
>

The "return FALSE" here on all those cases is just another way to ease
silencing Exceptions which is often a bad idea (like @).

It's also perfectly possible to make all those Exceptions implement a
"MatchingFailedException" interface (even empty). This would even make the
collection reusable AND extensible because new user Exception classes could
implement that "MatchingFailedException" interface without the need to
refactor code.

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] =3D=3D=3D 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] =3D=3D=3D 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 propose=
d
> feature would make perfect sense.
>
> I think saying that identical handling for multiple exception types is ba=
d
> or is grounds for an urgent code review is fairly disconnected from reali=
ty
> in many if not most cases.
>
> Zeev
>
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php


Patrick

--001a114cbc648f1fee0530f1f11a--