laminas / laminas-stratigility

PSR-7 middleware foundation for building and dispatching middleware pipelines
https://docs.laminas.dev/laminas-stratigility/
BSD 3-Clause "New" or "Revised" License
55 stars 12 forks source link

Naming conflict for "ErrorHandler" which actually is a middleware and not a RequestHandler #3

Closed weierophinney closed 1 year ago

weierophinney commented 4 years ago

Hey there,

I am giving some trainings regarding zend-expressive, e.g. for some colleagues. One thing, which always came back to me as a question was:

Why is the ErrorHandler called ErrorHandler like all those request handlers and not ErrorMiddleware which actually suits better as its a middleware per definition.

Would like to see the next major version where the ErrorHandler is renamed to ErrorMiddleware.

Any thoughts on this?


Originally posted by @boesing at https://github.com/zendframework/zend-stratigility/issues/190

weierophinney commented 4 years ago

We can do this by doing the following:

Please feel free to submit a PR against develop to accomplish the first step.


Originally posted by @weierophinney at https://github.com/zendframework/zend-stratigility/issues/190#issuecomment-520848534

weierophinney commented 4 years ago

I like the idea of providing that new ErrorMiddleware. I am not sure if triggering a deprecation message is a good idea. As of zend-expressive, the ErrorHandler is being automatically set to the configuration. So after updating zend-stratigility, every zend-expressive project will receive deprecation messages.

I would rather implement just a @deprecated tag, so that the handler can be safely removed in v4 instead. A migration guide should probably mention that the registered delegators should attach the listeners to the new middleware, as just "aliasing" wont be enough. (Some more details can be found in this comment https://github.com/zendframework/zend-mvc/pull/294#issuecomment-421050228)

I will provide a PR shortly with the changes.


Originally posted by @boesing at https://github.com/zendframework/zend-stratigility/issues/190#issuecomment-522538492

boesing commented 1 year ago

@weierophinney this wasn't integrated or do I miss something? "as completed" sounds like something was implemented tho.

weierophinney commented 1 year ago

It wasn't integrated, and #7 was closed (by you) back in 2020, so I figured it was dead in the water.#43

We can re-open if you'd like, or we can message that we will consider a patch if submitted.

boesing commented 1 year ago

It was actually requested by me as well, I closed it as no1 reviewed it for quite some time but I also think it could be good to keep this as some kind of RFC. Back in the days, we did not had RFCs and I still would love to have some more reasonable naming strategies regarding this package as outlined in the PR:

the NotFoundHandler is definitely a handler which does not delegate the request to the next handler and still is in the Middleware namespace. So it has to be a middleware? 🤷‍♂ Fix for that is in https://github.com/laminas/laminas-stratigility/pull/8

However, I am not too keen about this change and thus I don't care too much. I got some feedback back in 2019 while I've given a training for mezzio in our company and people were actually wondering why the ErrorHandler is a middleware.

I just wonder if there are ways to close issues without marking them as complete.