Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do we really need notFoundAction? #21

Closed
weierophinney opened this issue Dec 31, 2019 · 4 comments
Closed

Do we really need notFoundAction? #21

weierophinney opened this issue Dec 31, 2019 · 4 comments

Comments

@weierophinney
Copy link
Member

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?


Originally posted by @wesperinteractive at zendframework/zend-mvc#204

@weierophinney
Copy link
Member 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.


Originally posted by @wesperinteractive at zendframework/zend-mvc#204 (comment)

@weierophinney
Copy link
Member Author

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


Originally posted by @burrellramone at zendframework/zend-mvc#204 (comment)

@weierophinney
Copy link
Member Author

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


Originally posted by @froschdesign at zendframework/zend-mvc#204 (comment)

@alexmerlin
Copy link
Member

Closing issue due to being inactive for more than 1 year.

@alexmerlin alexmerlin closed this as not planned Won't fix, can't repro, duplicate, stale Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants