zendframework / zend-mvc

Mvc component from Zend Framework
BSD 3-Clause "New" or "Revised" License
105 stars 89 forks source link

Problem on DispatchListener with a controller abstract factory #238

Closed Ppito closed 7 years ago

Ppito commented 7 years ago

Hello,

I develop a module of error handler to integrate whoops in Zend Framework 3 (Ppito/zf3-whoops), and because of a bug issue was open to me, we found a limitation on distach with a controller abstract factory.

The problem is, when a syntax error is generated with abstract factory configuration, the application crashes when it's tried to create the controller. Can you move the test of creation on the try/catch below ? Otherwise the error cannot be caught and a blank page appears.

Like that :

try {
    // Query abstract controllers, too!
    if (! $controllerManager->has($controllerName)) {
        $return = $this->marshalControllerNotFoundEvent($application::ERROR_CONTROLLER_NOT_FOUND, $controllerName, $e, $application);
        return $this->complete($return, $e);
    }

    $controller = $controllerManager->get($controllerName);
} catch (Exception\InvalidControllerException $exception) {
    $return = $this->marshalControllerNotFoundEvent($application::ERROR_CONTROLLER_INVALID, $controllerName, $e, $application, $exception);
    return $this->complete($return, $e);
} catch (InvalidServiceException $exception) {
    $return = $this->marshalControllerNotFoundEvent($application::ERROR_CONTROLLER_INVALID, $controllerName, $e, $application, $exception);
    return $this->complete($return, $e);
} catch (\Throwable $exception) {
    $return = $this->marshalBadControllerEvent($controllerName, $e, $application, $exception);
    return $this->complete($return, $e);
} catch (\Exception $exception) {  // @TODO clean up once PHP 7 requirement is enforced
    $return = $this->marshalBadControllerEvent($controllerName, $e, $application, $exception);
    return $this->complete($return, $e);
}
Ocramius commented 7 years ago

This is up to your abstract factory to handle: has operations should be silent. If your abstract factory performs unsafe operations such as reflection, code discovery, etc, then AbstractFactory#canCreate needs to be designed in a safe way.

Closing as invalid, since ContainerInterface#has() does indeed not expect exceptions to be thrown, and the code using has shouldn't either.

bitwombat commented 7 years ago

If there's a syntax error in a controller, the class_exists call in \Zend\Mvc\Controller\LazyControllerAbstractFactory::canCreate throws an exception.

Ocramius commented 7 years ago

Yes, and that is up to the maintainer of that abstract factory to fix that, if relevant.

On 29 Apr 2017 14:50, "Bit Wombat" notifications@github.com wrote:

If there's a syntax error in a controller, the class_exists call in \Zend\Mvc\Controller\LazyControllerAbstractFactory::canCreate throws an exception.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/zendframework/zend-mvc/issues/238#issuecomment-298167169, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakCr-t2IRiw5KPH6SLRvLY_FHxbqpks5r0zH3gaJpZM4NMOxO .

Ppito commented 7 years ago

yes, but it's not an exception we need to catch, it's a php error. And it's this php error that will be nice to be catch with Throwable (only in php 7, I know).

Ocramius commented 7 years ago

No, that mentality will just lead you to having every layer of everything being wrapped in a try-catch. Fix your factory to be compliant with the silent signature.

On 29 Apr 2017 3:46 p.m., "Tonnelier Mickael" notifications@github.com wrote:

yes, but it's not an exception we need to catch, it's a php error. And it's this php error that will be nice to be catch with Throwable (only in php 7, I know).

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/zendframework/zend-mvc/issues/238#issuecomment-298169995, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakNZOE0VRx-PaMo45DGHFHMGBq0lgks5r0z9HgaJpZM4NMOxO .

Ppito commented 7 years ago

yes, yes, I know that. Ok thanks anyway.

bitwombat commented 7 years ago

@Ocramius you say "your factory", but this factory is part of \Zend\Mvc. Do I just need to make a separate, more specific issue?

Ocramius commented 7 years ago

Yes, or a PR

On 30 Apr 2017 01:17, "Bit Wombat" notifications@github.com wrote:

@Ocramius https://github.com/Ocramius you say "your factory", but this factory is part of \Zend\Mvc. Do I just need to make a separate, more specific issue?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/zendframework/zend-mvc/issues/238#issuecomment-298200433, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJakDJhlvTdcRhhm1p3BlRfqHEqidmpks5r08TvgaJpZM4NMOxO .