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.

Sign up for GitHub

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

Jump to bottom

Twig is required unnecessary #1887

Closed
Preclowski opened this issue Apr 25, 2018 · 7 comments
Closed

Twig is required unnecessary #1887

Preclowski opened this issue Apr 25, 2018 · 7 comments

Comments

@Preclowski
Copy link

Preclowski commented Apr 25, 2018 •
edited

Hi, I think twig dependency should be moved from require-dev to require or made optional some way.

Even if we use custom exception controller, there is registered service fos_rest.exception.twig_controller with dependency to templating.engine.twig.

        <service id="fos_rest.exception.twig_controller" class="FOS\RestBundle\Controller\TwigExceptionController" parent="fos_rest.exception.controller">
            <argument type="service" id="templating.engine.twig" />
        </service>

It would be nice if twig will not be required to use fosrest 😄
@stof
Copy link
Member

stof commented Apr 25, 2018

but is this service used when Twig is not available ?

@Preclowski
Copy link
Author

I dont think so, fos_rest.exception.controller should be used by default. But it's declared and symfony throws exception

The service "fos_rest.exception.twig_controller" has a dependency on a non-
existent service "templating.engine.twig".

@xabbuh
Copy link
Member

xabbuh commented Apr 25, 2018

Which version of FOSRestBundle do you use?

@xabbuh
Copy link
Member

xabbuh commented Apr 25, 2018

And what does your config look like?

@Preclowski
Copy link
Author

symfony 3.4.8, fosrest 2.3.1, twig removed from dependencies

# FOS Rest
fos_rest:
    exception:
        enabled: true
        exception_controller: 'fos_rest.exception.controller:showAction'

@magnetik
Copy link
Contributor

Might be related : #1772

@LLCDBDBBSysUser
Copy link

I have the same problem. Fix is available in #1945. Would be nice if it would get merged.

@Tobion Tobion mentioned this issue Jul 30, 2019
Closed
xabbuh added a commit that referenced this issue Jul 31, 2019
@xabbuh
bug #2012 Fix removal of fos_rest.exception.twig_controller when temp…
46df4f3 
…lating/twig is not enabled (Tobion)

This PR was squashed before being merged into the 2.5-dev branch (closes #2012).

Discussion
----------

Fix removal of fos_rest.exception.twig_controller when templating/twig is not enabled

Fixes #1772, #1887, #1945, #2002

With that you can remove

```
framework:
    templating:
        engines: twig
```

which fixes the deprecations and it will use `\FOS\RestBundle\Controller\ExceptionController` instead of `\FOS\RestBundle\Controller\TwigExceptionController` correctly. This in turn means it does not use the TwigBundle error rendering but the implementation using the Serializer (Symfony or JMS). But the error response stays the same because of `\FOS\RestBundle\Serializer\Normalizer\ExceptionHandler::convertToArray` (or the equivalent for Symfony serializer).
So people can disable templating and twig and it should still be BC. If people use TwigBundle but want to disable SF templating part to get rid of deprecations, they need to set

```
fos_rest:
    exception:
        exception_controller: 'fos_rest.exception.controller::showAction'
```

because otherwise the unwanted twig controller get's used again: https://github.com/FriendsOfSymfony/FOSRestBundle/blob/6013d5131f94cd7367c423e52bdfb83c4f0613f9/DependencyInjection/FOSRestExtension.php#L360-L367

Commits
-------

a2c8639 Fix removal of fos_rest.exception.twig_controller when templating/twig is not enabled
@xabbuh xabbuh closed this as completed Jul 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@magnetik @stof @xabbuh @Preclowski @LLCDBDBBSysUser