Zeev,
Le mar. 19 avr. 2016 à 11:56, Zeev Suraski <z...@zend.com> a écrit :
> 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;
> }
> }
>
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|AccessDeniedHttpException|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] === 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
>
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
Patrick
