zend-mvc icon indicating copy to clipboard operation
zend-mvc copied to clipboard

Do we really need notFoundAction?

Open wesperinteractive opened this issue 9 years ago • 4 comments

The question is simple: do we really need notFoundAction?

https://github.com/zendframework/zend-mvc/blob/master/src/Controller/AbstractActionController.php#L43

I don't think so...

My problems:

  1. It's not triggering a dispatch error, while all other and similar (!) errors do. If we can't locate the controller, we trigger a dispatch error, if we can't locate the action inside that controller, we do not trigger any error. What is the reason for handling similar errors in different ways? If we don't have notFoundController, then why have we notFoundAction?
  2. Because it doesn't trigger any error, it's hard to bypass it. If a custom error and exception handling is needed, we can do it easily by attaching custom listeners to the dispatch.error event, but this doesn't work for notFoundAction becasue the missing event. We cannot use the dispatch event neither: with priority 1 or lower it's too late, notFoundAction already returned a ViewModel with 404 status code, with priority 2 or higher it's too early, we don't have the controller yet.

What should be done?

  1. We should remove the notFoundAction and modify onDispatch (https://github.com/zendframework/zend-mvc/blob/master/src/Controller/AbstractActionController.php#L60).
  2. We should check the existance of the method in the DispatchListener (https://github.com/zendframework/zend-mvc/blob/master/src/DispatchListener.php#L77). If the method doesn't exists, we should trigger a dispatch error and set the name of the error to the following:

const ERROR_CONTROLLER_METHOD_NOT_FOUND = 'error-controller-method-not-found';

(I think it would be better than the old ERROR_CONTROLLER_CANNOT_DISPATCH, which means nothing for me, it's too general.)

After that we could do small simplifications in RouteNotFoundReason, and remove weird lines (!!!) similar to this one: https://github.com/zendframework/zend-mvc/blob/master/src/View/Http/RouteNotFoundStrategy.php#L231

By the way, that line and this switch case (https://github.com/zendframework/zend-mvc/blob/master/src/View/Http/RouteNotFoundStrategy.php#L135) perfectly shows how illogical the error handling in it's current form is. There are errors (retrieved via getError from MvcEvent) and there is ERROR_CONTROLLER_CANNOT_DISPATCH.

(The impossibility of retrieving the error type ERROR_CONTROLLER_CANNOT_DISPATCH from the MvcEvent via the getError method can cause further problems too. Let's say we would like to log them...)

Opinions?

wesperinteractive avatar Oct 06 '16 18:10 wesperinteractive

One small remark: removing notFoundAction would allow us to remove Zend\Mvc\View\Http\RouteNotFoundStrategy listener (prepareNotFoundViewModel) from the dispatch event listeners.

Currently we are checking the status code of the response for each and every request (https://github.com/zendframework/zend-mvc/blob/master/src/View/Http/RouteNotFoundStrategy.php#L174), which, in my opinion, is a bad practice: 404 status code isn't a general thing in an application. Listeners of the dispatch event should do general things, unless we don't want bad performance.

wesperinteractive avatar Oct 08 '16 17:10 wesperinteractive

+1 I'm having the same issue with the notFoundAction in Zend 3. I want to catch dispatch errors but they are never thrown when an action that doesn't exist is called on a controller.

This issue has been opened since Oct. 2016 but it hasn't been assigned to anyone and it hasn't been labelled?

ghost avatar Nov 11 '19 11:11 ghost

@burrellramone

This issue has been opened since Oct. 2016 but it hasn't been assigned to anyone and it hasn't been labelled?

The zend-mvc component is subject to some changes which are under development. An assessment of the current problem did not occur previously. So the issue itself is still open and relevant.

froschdesign avatar Nov 11 '19 12:11 froschdesign

This repository has been closed and moved to laminas/laminas-mvc; a new issue has been opened at https://github.com/laminas/laminas-mvc/issues/21.

weierophinney avatar Dec 31 '19 21:12 weierophinney