Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Do we really need notFoundAction? #204

Open
wesperinteractive opened this issue Oct 6, 2016 · 4 comments
Open

Do we really need notFoundAction? #204

wesperinteractive opened this issue Oct 6, 2016 · 4 comments

Comments

@wesperinteractive
Copy link
Contributor

wesperinteractive commented Oct 6, 2016

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
Copy link
Contributor Author

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.

@ghost
Copy link

ghost commented Nov 11, 2019

+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?

@froschdesign
Copy link
Member

@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.

@weierophinney
Copy link
Member

This repository has been closed and moved to laminas/laminas-mvc; a new issue has been opened at laminas/laminas-mvc#21.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants