FOSRestBundle icon indicating copy to clipboard operation
FOSRestBundle copied to clipboard

View default format is 'html'

Open yyaremenko opened this issue 4 years ago • 6 comments

steps to reproduce:

  • install FOSRestBundle and a serializer symfony/serializer-pack
  • don't modify the default fos_rest.yaml config file
  • follow the documentation example, create a controller which returns $this->handleView($this->view())

expectation:

the (default) Response format is json

reality:

the (default) Response format is html

reason

the FOSRestBundle View has no default format set, and upon being handled in FOS\RestBundle\View\ViewHandler::handle() the format is determined in line 303 $format = $view->getFormat() ?: $request->getRequestFormat();

in turn, Symfony\Component\HttpFoundation\Request::getRequestFormat() has the following signature public function getRequestFormat($default = 'html')

(I could argue here that any string / numeric values must be moved to constants like const REQUEST_FORMAT_DEFAULT = 'html'; and accessed like public function getRequestFormat($default = self::REQUEST_FORMAT_DEFAULT) but from my previous experience of communicating with core Symfony developers I can say they prefer copy-pasting raw scalar values throughout the code (thus violating the DRY principle), instead of moving such values to constants and referring to such constants)

so, 'html' is the default format, which is fine in terms of a regular web app; but since we are working with a REST bundle, it is rather logical to expect REST-related format rather than 'html'

please consider fixing this by auto-setting a default format to the View object upon its construction or somewhere you find suitable

it really took me a ton of time to get the reason why I'm getting the error

TypeError: Argument 1 passed to Twig\Environment::getTemplateClass() must be of the type string, null given

yyaremenko avatar Apr 27 '20 12:04 yyaremenko

Changing the default format would be a BC break. So we would have to deprecate something first and then probably introduce a new config option to opt-in to the new behaviour. I doubt that's something that is really helpful here.

Maybe a good idea would be to update the documentation so that it contains a route definition which sets the format to json so that this will be returned by the Request::getRequestFormat() method.

xabbuh avatar Apr 27 '20 13:04 xabbuh

thank you for the quick reply,

yes, I see your point; I would gladly update the docs, but, as I described here https://github.com/FriendsOfSymfony/FOSRestBundle/pull/2166 there are currently no docs regarding routing generation

also, I would allow myself to be a bit rude by saying current FOSRestBundle documentation in general does not look quite consistent, and one has to look through a bunch of (often outdated) blogs out there, to actually learn how to use the bundle;

I could fix that by converting the docs into a step-by-step sort of tutorial, but I have the fears my work will be rejected - sadly, I have some negative experience with Symfony developers;

so, we can either agree on some guidelines for me to refactor the docs - which will guarantee my work would not be rejected - or I will just have to wait for somebody out there to fix the stuff

yyaremenko avatar Apr 27 '20 15:04 yyaremenko

Well, you said you were following some documentation example. Looks like that one could be the one to be updated.

xabbuh avatar Apr 27 '20 16:04 xabbuh

Regarding the documentation: I am for improving the state of it as I agree that finding the right pieces isn't always easy. However, I am currently working hard on making 2.8 and 3.0 releases to finally ship Symfony 5 support. I don't think it makes much sense to invest too much time before as we wouldn't be able to give proper feedback.

xabbuh avatar Apr 27 '20 17:04 xabbuh

okay, I see, thank you for the info

yyaremenko avatar Apr 27 '20 17:04 yyaremenko

@xabbuh but why removing the HTML support, that was the best part. Having a self documented API following the rest of the website (it was a perfect case for some of my sites).

ls-philippe-gamache avatar Aug 24 '20 22:08 ls-philippe-gamache