Accept header ignored when exceptions sent from "forced" ParamFetcher #432

Closed
mdhooge opened this Issue Apr 10, 2013 · 15 comments

Comments

Projects
None yet
3 participants
Contributor

mdhooge commented Apr 10, 2013

With exactly the same code:

  • when the request has the header Accept: application/json, and,
  • when an exception (e.g. HttpException) is thrown from ParamFetcher,
    the response is different depending on the param_fetcher_listener value.

When param_fetcher_listener: true, a JSON status is sent back -- which is the expected behaviour.
When param_fetcher_listener: 'force', an HTML text is sent -- which isn't correct given the header.

I guess this is because the priority of the ParamFetcherListener is 5 while other FOS listeners are at 0. But I don't know how to correct it, sorry.

Thanks

Owner

lsmith77 commented Apr 12, 2013

can you spend some time debugging this? for example it should be fairly easy to change the priorities of the events.

Contributor

mdhooge commented Apr 15, 2013

Here are my understanding of the problem… if you could advise me on how to investigate more, I'll happily do it.

In both cases, an exception is thrown because a parameter is missing.

With param_fetcher_listener: true, the ParamFetcher instance is simply created and handed over to the Controller action method. Then within the controller, when $paramFetcher->get() is called, the fetcher detects that the param is missing and throws the exception. But I don't know how the exception is caught (not by me, for sure) and who translates it into JSON.

In the other case, with param_fetcher_listener: 'force', the fetcher tries to retrieve all the declared parameters (so Symfony's default code can put them into the action method parameters). The exception is thrown but not caught at the same level. So finally an HTML page is constructed.

I mentioned priorities in my previous comment but unfortunately they cannot be changed. Otherwise the Fetcher won't be called before the action is executed.

Thanks for your help

Owner

lsmith77 commented Apr 15, 2013

Yes your understanding is correct. To fill in the blanks. Uncaught exceptions are handled by the kernel.exception event which in the end calls an ExceptionController. there is one shipped with TwigBundle which can optionally be replaced with one by FOSRestBundle (needs to be configured). both however do support returning json if that is set as the request format in the request instance.

Now in both cases you have an uncaught exception, which will eventually end up in the ExceptionController. the problem is likely that in the force case the exception happens before the format is set in the request via the format listener.

Contributor

mdhooge commented Apr 15, 2013

Thanks, it was the missing part of the puzzle ;-)

I'll experiment that way…

Contributor

choomz commented Jul 2, 2013

@lsmith77 thanks for your answer. I got exactly the same problem, but I don't see how to set the request format before it goes throught the ParamFetcher. My exception is triggered in html like mdhooge. Can you provide a good way to handle it ? I would like to keep using the force parameter of th param fetcher listener.

Thanks a lot !

Owner

lsmith77 commented Jul 2, 2013

at this point i do not have a solution. what we might need to do is trigger the format listener explicitly in the ExceptionController. could you have a look at that?

Contributor

choomz commented Jul 3, 2013

Well, I checked deeper into the ExceptionController & FormatNegotiator and it seems that the problem come from the _format parameter wich is set in the getFormat() in the ExceptionController.

https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/Controller/ExceptionController.php#L179

Commenting this line make the trick, unit tests are still OK, but I am not sure about potential side effects...
Can you confirm that it should be OK ?

I will then make a small PR on that. Thanks.

Owner

lsmith77 commented Jul 3, 2013

Ah I see .. I had already integrated the format negotiation into the ExceptionController. the impact of removing that line is very hard to predict. what might make more sense is to change the default 'html' to null for $format and only set the attribute if its non null. can you try that?

Contributor

choomz commented Jul 3, 2013

Actually it does not work, because when the showAction is called, the $format is already set as 'html'. So the default parameter is not used. I tried to change html to null here :
https://github.com/FriendsOfSymfony/FOSRestBundle/blob/master/Controller/ExceptionController.php#L54

Owner

lsmith77 commented Jul 3, 2013

hmm looking into ExceptionListener .. the format is always set to $request->getRequestFormat() in the request created for the ExceptionController .. so I wonder why removing that line has any effect at all to begin with.

Contributor

choomz commented Jul 3, 2013

I don't know :) But if we want to keep it, maybe we could do something like that at the end of getFormat() method :

$format = $formatNegotiator->getBestFormat($request, $priorities, $preferExtension) ?: $format;
$request->attributes->set('_format', $format);
return $format;

Please tell me what you want, and maybe we can make a fix today ?

Owner

lsmith77 commented Jul 3, 2013

yeah that makes sense .. does that solve the issue for you?

Contributor

choomz commented Jul 3, 2013

yes it's ok for me, let's go for a PR ?

Owner

lsmith77 commented Jul 3, 2013

yes .. please do

Contributor

choomz commented Jul 3, 2013

@lsmith77 ping ? :)

@lsmith77 lsmith77 closed this in #492 Jul 3, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment