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

Being able to determine if only json or xml should be handled, instead of both. #2

Open
2 tasks done
weierophinney opened this issue Dec 31, 2019 · 3 comments
Open
2 tasks done

Comments

@weierophinney
Copy link
Contributor

I have run into this use case:

My application has a JSON API which is served under the /rest path, but also 3 or 4 more routes which are not part of this API context.

For the API I use "problem details" for the errors, but I also have some other error handling logic for the other routes.

Because of this, my middleware pipeline looks more or less like this:

// ...

$app->pipe(Zend\Stratigility\Middleware\ErrorHandler::class);
$app->pipe('/rest', Zend\ProblemDetails\ProblemDetailsMiddleware::class);

// More middlewares...

$app->pipe('/rest', Zend\ProblemDetails\ProblemDetailsNotFoundHandler::class);
$app->pipe(App\NotFoundHandler::class);

With this approach, most of the use cases work as expected, except when a request is performed to a not-found path which starts with /rest, proividing a non-JSON Accept header, but one that matches *+xml.

This usually happens when a request is performed from a browser, in which case the Accept has the value text/html,application/xhtml+xml,application/xml...

This is making the ProblemDetailsNotFoundHandler to generate an xml response, but I would expect/like it to be skipped and my custom NotFoundHandler (the second one) to be executed instead.

Proposal

In order to "solve" this, the first thing that comes to my mind is this approach:

Allowing to dynamically determine which "contexts" should be enabled for the module, as in "JSON only", "XML only" or "both".

I see that currently, the headers that determine if the ProblemDetailsMiddleware and the ProblemDetailsNotFoundHandler should act as error handlers, are hardcoded on ProblemDetailsResponseFactory::NEGOTIATION_PRIORITIES.

Since the three classes make use of that constant to call a Negotiator instance, maybe the whole logic could be wrapped into a helper which is injected in the three of them.

This helper could then get the "context" configuration or fallback to "both", keeping current behavior.

Let me know if you think this makes sense, or if you think there's any simpler way to achieve the same result.

In any case, I'm open to contribute a PR with the required changes.


Originally posted by @acelaya at zendframework/zend-problem-details#49

@weierophinney
Copy link
Contributor Author

@acelaya

The NEGOTIATION_PRIORITIES constant sets the prioritized list that is consulted by the Negotiator instance when it attempts to match the incoming request. The XML versions are fallbacks, in part because every browser uses a default Accept type that matches it, which ensures that there is something readable if it is hit. You'll notice in ProblemDetailsResponseFactory that if the MediaType returned by the negotiator is not "json", we fallback to XML representation (see lines 386-388).

We definitely could potentially allow users to hard-code the type returned, and I agree that this would require a helper that wraps when Negotiator is called.

Are you still interested in working on a patch for this?

@acelaya
Copy link
Contributor

acelaya commented Aug 6, 2020

Hey!

I had to read again my original text to remember what was I reporting, haha.

I'm absolutely open to contribute this if you find it useful.

However I have to say that I've been using this as it is for several months with no big issues at all, so it's probably a pretty marginal use case.

I wouldn't like to end up adding an unnecessary complexity to a perfectly working library, so I will leave the decision to you guys.

If the decision is to go with it, I'll take care of the implementation.

@ElDarco
Copy link

ElDarco commented Oct 30, 2020

Hi to all!
I used your library with ZendExpressive 2.0 and used my ExtendErrorHandler to coerce Exceptions to the way you implemented in the problem-details package.
But I only learned about the benefits of this package when I switched from ZE 3.0 to Mezzio.
To check the work of my api, I'm used to accessing the service through a browser. No rest client.
Unfortunately your package deprives me of this opportunity, since the Accept header that my browser sends text/html, application/xhtml + xml,application/xml

I join the author. I am also willing to contribute and modify the package and be able to skip your handler.

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

No branches or pull requests

3 participants