FriendsOfSymfony / FOSRestBundle

This Bundle provides various tools to rapidly develop RESTful API's with Symfony
http://symfony.com/doc/master/bundles/FOSRestBundle/index.html
MIT License
2.8k stars 702 forks source link

Using FOS\RestBundle\Controller\Annotations\View throws exception in Symfony 4 #1812

Open matigda opened 6 years ago

matigda commented 6 years ago

I'm migrating from Sf 3 to Sf4, this is how my controller looks like


use FOS\RestBundle\Controller\Annotations\Post;
use FOS\RestBundle\Controller\Annotations as Rest;

... some code

    /**
     * @Post("/users")
     * @Rest\View(statusCode=201)
     */
    public function registerAction(Request $request)
    {
          ... some action
    }

And then I receive this exception:

Warning: ReflectionObject::__construct() expects parameter 1 to be object, null given in vendor/friendsofsymfony/rest-bundle/EventListener/ViewResponseListener.php line 162

This same code works on Symfony 3.3.10. Also - when I remove this - @Rest\View(statusCode=201) then everything works.

I'm using FOSRestBundle v2.3.0

I noticed that Sensio\Bundle\FrameworkExtraBundle\EventListener\TemplateListener::onKernelController is not being executed. This may be crucial as there is this line: $template->setOwner($controller); which - not called - causes error in ViewResponseListener. Maybe I have not configured something properly yet, I'll check it.

EDIT: Ok, so I found the cause. In Sensio\Bundle\FrameworkExtraBundle\DependencyInjection\Compiler\OptimizerPass::process there is this line:

        if (!$container->hasDefinition('twig')) {
            $container->removeDefinition('sensio_framework_extra.view.listener');
        }

which I think everyone understands by now what is doing. When I commented this out - it worked. Funny thing - due to the fact that twig was by default in Sf 3.3.10 this was not the issue. But right now FOSRest depends heavily on that. Unless there is another way of doing what I'm doing ;)

wibimaster commented 6 years ago

+1, same problem with Symfony 3.4 (installed with Symfony Flex). Installation of Twig bundle resolve the problem. But it's ugly to have to install Twig for JSON rendered ;)

matigda commented 6 years ago

Well I can actually make a PR to solve it but the question is how authors would like it to be resolved. Whether add this definition again in some compiler pass in this bundle or just create another listener which would do what should be done or ... some other solution which I haven't thought of ;)

DavidGarciaCat commented 6 years ago

Hi all,

No more activity here? I have the same problem, using SF 3.4

xabbuh commented 6 years ago

I think we should investigate if we could move/copy some logic to FOSRestBundle.

xabbuh commented 6 years ago

Can you confirm that the change proposed in #1834 will give a better error message?

DavidGarciaCat commented 6 years ago

Hi @xabbuh

Regarding PR https://github.com/FriendsOfSymfony/FOSRestBundle/pull/1834

screen shot 2018-01-04 at 23 02 40

Quick question: given the PR description is pointing the TwigBundle and FOSRestBundle is a Bundle to create RESTful APIs (that nowadays usually return XML or JSON) what's the point to force me to install and enable a Bundle that deals with templates?

Maybe I don't understand the logic behind this, but it seems useless to me to throw an error just because I don't have the TwigBundle when I have no intention to manage anything related with that Bundle or templates.

On the other hand, if TwigBundle is required, it should be included as a requirement in composer.json file to make sure no one has gets an error like this, right?

xabbuh commented 6 years ago

@DavidGarciaCat #1834 is not the solution. It's just there to provide a better feedback on how to work around the issue for the moment.

As written in https://github.com/FriendsOfSymfony/FOSRestBundle/issues/1812#issuecomment-355236147 the real solution IMO is to implement some of the logic of the TemplateListener from SensioFrameworkExtraBundle ourselves. But that requires a bit more effort.

DavidGarciaCat commented 6 years ago

Hi @xabbuh

Excellent, now I see your point!

I believe that update might help a little but, in the end, the real problem is what was described and potential solution, as you said, might be to copy&paste some code, but TBH I don't know - I didn't check the TemplateListener so I can't confirm, but if you think this is the right approach then I guess is fine.

PierrickMartos commented 6 years ago

Same issue here and really difficult to identify it whit current error message. Thanks!

rokkie commented 6 years ago

I ran into the same error today after upgrading from 2.3.0 > 2.3.1. I was a bit annoyed because I'm not using annotations (for routing) or templates, so for me it just broke.. I've decided to stay on 2.3.0 for now. Is there a way I can help fixing this?

xabbuh commented 6 years ago

@rokkie That should not happen. Can you show the configuration that you use and the error you end up with?

rokkie commented 6 years ago

@xabbuh This is all the config I could think of being relevant. This works with 2.3.0 but after upgrading I got the error as a result of ./bin/console cache:clear during install.

config/routes/api.yaml:

content_pages:
  type: rest
  prefix: /v1
  resource: App\Controller\Api\V1\PagesController

# ...

config/packages/fos_rest.yaml:

fos_rest:
  routing_loader:
    default_format: json
    include_format: false
  view:
    view_response_listener: true
    formats:
      rss: false
      xml: false
      json: true
  param_fetcher_listener:  true

config/bundles.php:

return [
    Symfony\Bundle\FrameworkBundle\FrameworkBundle::class => ['all' => true],
    Symfony\Bundle\WebServerBundle\WebServerBundle::class => ['dev' => true],
    JMS\SerializerBundle\JMSSerializerBundle::class => ['all' => true],
    FOS\RestBundle\FOSRestBundle::class => ['all' => true],
    Sensio\Bundle\FrameworkExtraBundle\SensioFrameworkExtraBundle::class => ['all' => true],
    Symfony\Bundle\MonologBundle\MonologBundle::class => ['all' => true],
    Symfony\Bundle\MakerBundle\MakerBundle::class => ['dev' => true],
    Doctrine\Bundle\DoctrineCacheBundle\DoctrineCacheBundle::class => ['all' => true],
    Doctrine\Bundle\DoctrineBundle\DoctrineBundle::class => ['all' => true],
    Doctrine\Bundle\MigrationsBundle\DoctrineMigrationsBundle::class => ['all' => true],
    Knp\Bundle\MarkdownBundle\KnpMarkdownBundle::class => ['all' => true],
    Symfony\Bundle\SwiftmailerBundle\SwiftmailerBundle::class => ['all' => true],
];

src/Controller/Api/V1/PagesController.php:

namespace App\Controller\Api\V1;

use FOS\RestBundle\Controller\Annotations\NamePrefix;
use FOS\RestBundle\Controller\FOSRestController;
use FOS\RestBundle\Routing\ClassResourceInterface;
use FOS\RestBundle\View\View;

/**
 * @NamePrefix("api_")
 */
class PagesController extends FOSRestController
  implements ClassResourceInterface
{
  // ...
}
magnetik commented 6 years ago

@rokkie if you do not load the TwigBundle, it will be removed by the container compiler optimizer pass of the SensioFrameworkExtraBundle (in \Sensio\Bundle\FrameworkExtraBundle\DependencyInjection\Compiler\OptimizerPass) so it's not available. Add the TwigBundle in your bundles.

jamalelomri commented 6 years ago

you should add populateDefaultVars in the view annotation parameters like this :

/*

Tobion commented 5 years ago

So what this issue is about: Make ViewResponseListener work without Twig and potentially also without SensioFrameworkExtraBundle.

willmorgan commented 4 years ago

This still seems to be an issue. My setup is just returning JSON for the majority of endpoints, and I need to use Twig for email template rendering.

Am I missing something, or is the only way around this setting populateDefaultVars=false on every route? Because if so that's pretty nasty, but the least worst option 😁

willmorgan commented 4 years ago

A nicer way of solving this if you're just implementing a JSON API without Flex is setting up a subscriber to disable populateDefaultVars:

<?php

namespace App\Subscriber;

use FOS\RestBundle\Controller\Annotations\View;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\ViewEvent;
use Symfony\Component\HttpKernel\KernelEvents;

class SetPopulateDefaultVarsSubscriber implements EventSubscriberInterface
{
    public static function getSubscribedEvents()
    {
        return [
            KernelEvents::VIEW => ['disablePopulateDefaultVars', 31],
        ];
    }

    public function disablePopulateDefaultVars(ViewEvent $event)
    {
        $request = $event->getRequest();
        $template = $request->attributes->get('_template');

        // no @View present
        if (null === $template) {
            return;
        }

        // we need the @View annotation object or we cannot continue
        if (!$template instanceof View) {
            throw new \InvalidArgumentException('Request attribute "_template" is reserved for @View annotations.');
        }

        $template->setPopulateDefaultVars(false);
    }
}
W0rma commented 4 years ago

So what this issue is about: Make ViewResponseListener work without Twig and potentially also without SensioFrameworkExtraBundle.

It would be really nice to remove at least the Twig dependency since the HTML rendering part of this bundle was removed in version 3

Th3Mouk commented 3 years ago

Hello @W0rma and @xabbuh !

I just give a try on this bundle on a fresh SF5.3 project where I don't need Twig. I try to figure out yesterday, what I was missing to reach this error (must enable TwigBundle), while I only want json, xml serialization.

After further investigation (i'm on FOSRestBundle 3.0.5 and SensioFrameworkBundle 6.1.5), it seems that Sensio\Bundle\FrameworkExtraBundle\EventListener\TemplateListener only handle @Template annotation. And if you don't have twig, instead of simply returning early in their methods (they already do that), they optimized via a pass compiler, removing the listener.

On our side, if the @View annotation is declared on an action, or the ViewResponseListener is configured to force, we set a response in the ViewEvent. Finally, if @Template annotation is not defined, then the sensio listener will never overwrite the response in the ViewEvent.

So there is two solutions:

IMO, the second option is the best because this condition is no longer needed (probably was there for historical reasons), but I don't have the global picture of the lib. I don't see either any BC, even for persons not using @Template annotation, and having or not @View annotation.

I still have a question, is there a use case where it is necessary to define both annotations (@View @Template) ? Because @Template seems to always override the response, they do not seem complementary, and if you confirm that, probably it will be a nice to have info in the doc.

I would be happy to help you making the PR, if this solution is suitable.