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

Drop Twig Dependency #1149

Closed
ismailbaskin opened this issue May 31, 2017 · 23 comments
Closed

Drop Twig Dependency #1149

ismailbaskin opened this issue May 31, 2017 · 23 comments

Comments

@ismailbaskin
Copy link
Contributor

I think this library mostly aims API first or API only projects. Twig is an unnecessary dependency with enable_swagger_ui: false but still needed for api_platform.swagger.action.ui service.

@teohhanhui
Copy link
Contributor

I believe we can drop Twig entirely even when using the Swagger UI by ensuring that it's fully client-side.

@ismailbaskin
Copy link
Contributor Author

In fact I think separating "Swagger UI for Symfony" project can be more reasonable.

@teohhanhui
Copy link
Contributor

teohhanhui commented Jun 1, 2017

Yes, I see no reason for this project to include any JS and CSS, etc.

It should live in a separate frontend project, like https://github.com/api-platform/admin and https://github.com/api-platform/generate-crud. It should be included as part of our Docker Compose setup. And with Symfony Flex it should be pretty easy to advise the user to install it separately (or even auto-install?).

@teohhanhui
Copy link
Contributor

@api-platform/core-team One guiding principle for web APIs is the separation of the frontend and the backend. I believe we need to set a good example ourselves.

@dunglas
Copy link
Member

dunglas commented Jun 1, 2017

I definitely agree with that, but it's easiest solution right now. We can work to do it in 2.2!

@pierre-H
Copy link
Contributor

pierre-H commented Jun 6, 2017

What if we use EasyAdminBundle or another bundle which requires Twig ?
Personally, I don't use api-platform-admin but EasyAdminBundle because my frontend project is in typescript with react. So I can't add easily api-platform-admin. And I don't want to have two projects : one for the app in typescript and another one for the admin with js.

@soyuka
Copy link
Member

soyuka commented Jun 6, 2017

@pierre-H this doesn't mean that you can't have twig as a dependency in your project :). This is just a proposal to drop the hard soft dependency on twig on the core library.

@dunglas
Copy link
Member

dunglas commented Jun 6, 2017

There is no hard dependency on Twig in core. It's just a soft dep.

@fgrandjean
Copy link

Actually the Symfony bridge's service "api_platform.swagger.action.ui" introduce a hard dependency with Twig (still true is the 2.2-beta).
The "cleanest" fix i could think of, would be to remove that service using a compiler pass when the twig service definition doesnt exists ?

@Simperfit
Copy link
Contributor

@fgrandjean Do you have some time to implement this ?

@fgrandjean
Copy link

@Simperfit I can, but i do need some direction : Do we go for a clearer exception that tell end user that when used with Symfony they are required to add twig as a deps or do we silently toggle on/off the Swagger ui based on deps (as the Sf FrameworkBundle does) ?

@Simperfit
Copy link
Contributor

@fgrandjean I think we need to toggle silently off when twig is not part of the project.

WDYT @api-platform/core-team ?

@teohhanhui
Copy link
Contributor

teohhanhui commented Apr 9, 2018

api_platform.swagger.action.ui should be moved to swagger-ui.xml. This way if enable_swagger_ui is false, it'd not be loaded.

@fgrandjean
Copy link

Taking a closer look it seems swagger-ui is not the only hard dependency on twig. "api_platform.graphql.action.entrypoint" also introduce a hard dependency.
So, i guess that if TwigBundle is not available we should toggle "enable_swagger_ui" and "graphql" to false in config ?

@soyuka
Copy link
Member

soyuka commented Apr 9, 2018

It's the same for graphiql it's ui should be loaded separatly

@meyerbaptiste
Copy link
Member

We should replace Twig by the Symfony Templating component and its PHP templating engine! 😋

@soyuka
Copy link
Member

soyuka commented Apr 7, 2019

Twig dependency is dev-only now so I guess this can be closed.

@shouze
Copy link

shouze commented Aug 14, 2020

looks like it's not closed... 😭

Symfony version: 5.1.3
api-platform/core v2.5.6

Here's what I get

Script cache:clear returned with error code 1
!!  
!!   // Clearing the cache for the prod environment with debug                      
!!   // false                                                                       
!!  
!!  
!!  In CheckExceptionOnInvalidReferenceBehaviorPass.php line 86:
!!                                                                                 
!!    The service "api_platform.swagger.action.ui" has a dependency on a non-exis  
!!    tent service "twig".                                                         
!!                                                                                 
!!  
!!  cache:clear [--no-warmup] [--no-optional-warmers] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-e|--env ENV] [--no-debug] [--] <command>
!!  
!!  
Script @auto-scripts was called via post-install-cmd

I can help making the PR next week if somebody can guide me a bit about what to do.

@shouze
Copy link

shouze commented Aug 14, 2020

I guess that about swagger ui action we can somehow conditionnaly load the swagger-ui.xml file.

But then... if I comment it to test I'm facing the same issue but this time with graphiql.

Executing script cache:clear [KO]
 [KO]
Script cache:clear returned with error code 1
!!  
!!  In CheckExceptionOnInvalidReferenceBehaviorPass.php line 86:
!!                                                                                 
!!    The service "api_platform.graphql.action.graphiql" has a dependency on a no  
!!    n-existent service "twig".                                                   
!!                                                                                 
!!  
!!  
Script @auto-scripts was called via post-install-cmd

I've not find at the moment where I could fix this one, can anybody help me?

@shouze
Copy link

shouze commented Aug 14, 2020

I finally wiped out the errors but this is a lot of things to conditionnally load depending if twig is installed or not. Especially at the moment looks like it's not easy when graphql is enabled to not install twig as a production requirement.

@shouze
Copy link

shouze commented Aug 14, 2020

So... should I throw exceptions when twig is not available but graphql enabled or swagger ui enabled? 🤔

@shouze
Copy link

shouze commented Aug 14, 2020

Ok, so I've found this PR that has never been merged and that looks like a good starting point: #1829

@shouze
Copy link

shouze commented Aug 18, 2020

@dunglas @teohhanhui I've found a fix by overriding swagger ui action service in my own application:

services:
  # this service is not lazy and requires twig so let's override it
  api_platform.swagger.action.ui:
    class: ApiPlatform\Core\Bridge\Symfony\Bundle\Action\SwaggerUiAction

So... if you want I can make a PR but I see at least 2 to do things:

  1. make api_platform.swagger.action.ui lazy.
  2. like in Fix. Symfony integration when starting with the micro Flex application. #1829, put class_exists() calls about twig and throw exceptions to inform during compiler passes to install twig when you enable some service (swagger ui, graphql) that requires it.

Which one do you prefer? (Alternatives accepted).

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

No branches or pull requests

9 participants