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

Pass ErrorHandler to the RouteHandler. #238

Closed
wants to merge 2 commits into from

Conversation

map-g
Copy link

@map-g map-g commented Sep 27, 2023

In order to response with individual error messages and / or formats (e.g. JSON), we are able to put an ErrorHandler to the container (see #175). But the individual ErrorHandler is not used in the RouteHandler. This PR allows to pass an ErrorHandler to the RouteHandler, and the App passes the selected ErrorHandler to the RouteHandler. This will allow to response with individual error messages and / or formats in case a route is not found.

@SimonFrings
Copy link
Contributor

Hey @map-g, thanks for opening a pull request for this, always happy about contributions 👍

It makes sense to add more control over the Errorhandler to Framework X, I'm interested in why you need this feature. Can you share a short example here to help us understand your use case for this?

Furthermore, the pull request appears to be in good shape. However, I believe we should include some additional tests to ensure that this new feature works as expected.

I also talked with @clue as we have plans to change a few things about the Errorhandler in the future. For instance, we intend to remove the Errorhandler from the RouteHandler, rename it to ExceptionHandler, and give more control over this in total. At the moment, we do not have a precise timeline for when this will be added to the project. However, it is inevitable that there will be some BC-breaks involved in the process. The whole BC-break topic also depends on giving Framework X a first version on GitHub, which is our next priority.

Still trying to figure out if we should use your suggestion as a transitional solution until we tagged the first version and apply our planned changes to the Errorhandler. Interested in your thoughts on this.

@map-g
Copy link
Author

map-g commented Oct 4, 2023

Hi Simon,
Thanks for your reply! We need this feature because we are building a JSON API and we do not want to respond in HTML format in case of "404/Not Found". Generally it feels counterintuitive if you supply an ErrorHandler via a container that is used sometimes and sometimes not (BTW: shouldn't the @Final annotation be removed from the ErrorHandler class, because you have to inherit from it to create a custom ErrorHandler?).
Would have preferred to get the ErrorHandler from the container (already passed to the RouteHandler), but that will lead to problems if multiple containers are added as handlers (and causes errors in related tests).
I have added a test, but that fails during CI for older environments (likely due to the - now fixed - phpstan/phpstan#152 ?). Would need some help/advice how to deal with this.
I agree that this PR should only supply a transitional solution and that error handling should be more controllable in general.

@SimonFrings
Copy link
Contributor

@map-g As you may already know, we're currently working on a first version for Framework X which should go live tomorrow, see https://twitter.com/x_framework/status/1762499173830778907.

In preparation for this release, we've been brainstorming a ton of new features, including the possibility of supporting JSON responses. This is definitely on our roadmap and will probably be a feature addition in the near future! While it might not make it into tomorrow's version, we're confident it will be included in an upcoming release.

As there will be some refactorings and additions to the whole handler structure anyway, I think it's best we close this for now as we also expect to pick this up in the near future. We can't say for sure what the new handlers will look like as of today, so in my opinion it's best to focus on that first and then revisit this topic afterwards 👍

@SimonFrings SimonFrings closed this Mar 4, 2024
@SimonFrings SimonFrings added new feature New feature or request question Further information is requested labels Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants