Fix. Symfony integration when starting with the micro Flex application. #1829

fgrandjean · 2018-04-09T10:31:29Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#1149
License	MIT
Doc PR

The goal here is to make it failsafe to install api-platform/core on a symfony/skeleton project.
It remove hardcoded dependencies for doctrine/doctrine-bundle and symfony/twig-bundle.
Corresponding features get toggle back on when the package are added to the project.

soyuka · 2018-04-09T12:12:07Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php

-        if (class_exists(Annotation::class)) {
+        // The "annotation_reader" service is only available if DoctrineBundle is loaded.
+        $bundles = $container->getParameter('kernel.bundles');
+        if (class_exists(Annotation::class) && isset($bundles['DoctrineBundle'])) {


Is this check on bundles really mandatory? I think it'd look cleaner with class_exists but maybe I'm wrong

I know it's a bit weird but in theory it's possible to require "doctrine/annotation" without "doctrine/doctrine-bundle", in that case the class exists but not the service definition.

No problem, can we replace this check by class_exists(DoctrineBundle::class) then?

At first it does look cleaner but it's not as reliable. Again, in theory, you can install a bundle with composer but not register it in Symfony's kernel. But since it's an edge case, maybe it's acceptable to simply check for class existance?

I'd need some expert hands to answer this as I'm not qualified enough :p

I agree with @fgrandjean it should not be tested alone, since it could lead to unexpected behaviours (as the bundle may not be in the Kernel). So I guess it's better to test that the bundle is loaded.

I'm for keeping the current code too.

Simperfit · 2018-04-10T09:36:31Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php

        $loader->load('swagger.xml');

-        if ($config['enable_swagger_ui']) {
+        $bundles = $container->getParameter('kernel.bundles');


do you really need the variable $bundles ?

Simperfit · 2018-04-10T09:36:44Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php


        $loader->load('graphql.xml');
+
+        $bundles = $container->getParameter('kernel.bundles');


do you really need the variable $bundles ?

dunglas · 2018-05-02T22:06:37Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php

-        if (class_exists(Annotation::class)) {
+        // The "annotation_reader" service is only available if DoctrineBundle is loaded.
+        $bundles = $container->getParameter('kernel.bundles');
+        if (class_exists(Annotation::class) && isset($bundles['DoctrineBundle'])) {


I'm for keeping the current code too.

dunglas · 2018-05-02T22:08:28Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php


-        if (class_exists(Annotation::class)) {
+        // The "annotation_reader" service is only available if DoctrineBundle is loaded.
+        $bundles = $container->getParameter('kernel.bundles');


$bundles is already defined here: https://github.com/fgrandjean/core/blob/a6bb41d21f37898d80daae52336418e5310d4daf/src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php#L118

You can just pass it as parameter of all register* methods.

dunglas · 2018-05-02T22:10:07Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php


-        if ($config['enable_swagger_ui']) {
+        $bundles = $container->getParameter('kernel.bundles');
+        if ($config['enable_swagger_ui'] && isset($bundles['TwigBundle'])) {


Maybe should we just throw if enable_swagger_ui is true but Twig isn't available. It would be easier to debug (the exception's message should recommend to install twig-bundle.

The default value of enable_swagger_ui can be false if twig isn't available btw (in the Configuration class). As you'll not be able to access to the installed bundle list in this class, you can fallback on checking if class_exists(TwigBundle::class).

…g but is missing the Twig dependency.

dunglas · 2018-05-03T22:13:52Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php


        $loader->load('graphql.xml');
+
+        if (isset($bundles['TwigBundle'])) {


It looks wrong to me. It means that you can't get GraphQL support without Twig.

As for Swagger UI, you should throw if api_platform.graphql.graphiql.enabled is true but Twig isn't installed.

dunglas · 2018-05-03T22:14:47Z

src/Bridge/Symfony/Bundle/Resources/config/graphql-ui.xml

@@ -0,0 +1,21 @@
+<?xml version="1.0" ?>


This should be reverted. This action isn't here for GraphiQL in the first place. It is the main GraphQL entrypoint.

Throw exception when graphql is turn on but Twig is missing. Fixing tests to cover exceptions.

teohhanhui · 2018-05-04T09:26:47Z

src/Bridge/Symfony/Bundle/DependencyInjection/ApiPlatformExtension.php


-        if (class_exists(Annotation::class)) {
+        // The "annotation_reader" service is only available if DoctrineBundle is loaded.
+        if (class_exists(Annotation::class) && isset($bundles['DoctrineBundle'])) {


We should use

$container->hasExtension('doctrine')

There is no need to pass the $bundles.

I mean, they're not strictly the same thing. But we can safely assume that the doctrine extension must be registered if DoctrineBundle is loaded.

WDYT @dunglas? I think we can simplify the existing code too...

To me it's mostly the same thing, except that we check for TwigBundle using isset($bundles['TwigBundle']) it made sens to do it the same way for doctrine to keep the code easy to read. But I let @dunglas decide.

fgrandjean added 2 commits April 9, 2018 12:21

Fix. Symfony integration when starting with the micro Flex application.

1ca6f83

Fixing tests to match changes.

a6bb41d

soyuka reviewed Apr 9, 2018

View reviewed changes

Simperfit requested a review from dunglas April 10, 2018 09:34

Simperfit reviewed Apr 10, 2018

View reviewed changes

Simperfit added the [WFC] Waiting for Change label Apr 11, 2018

dunglas requested changes May 2, 2018

View reviewed changes

fgrandjean added 4 commits May 3, 2018 17:00

Removed unecessary parameters bag lookup for loaded Bundles array.

b165208

Upd. Throw a clear exception if user have turn swagger-ui on in confi…

9ab6fe0

…g but is missing the Twig dependency.

Merge branch 'master' into master

222baca

Fix. code Styles.

8060455

dunglas requested changes May 3, 2018

View reviewed changes

Revert Graphql-Ui config.

a562803

Throw exception when graphql is turn on but Twig is missing. Fixing tests to cover exceptions.

teohhanhui suggested changes May 4, 2018

View reviewed changes

fgrandjean added 3 commits May 4, 2018 11:36

Oops, only load swagger_ui if it's enabled.

ef6f84a

Fixing codeStyle.

cb20f2b

Fixing test for GraphQL intergration without Twig.

e84b9d8

fgrandjean closed this Oct 2, 2018

shouze mentioned this pull request Aug 14, 2020

Drop Twig Dependency #1149

Closed


		$loader->load('graphql.xml');

		$bundles = $container->getParameter('kernel.bundles');


		$loader->load('graphql.xml');

		if (isset($bundles['TwigBundle'])) {

Uh oh!

Fix. Symfony integration when starting with the micro Flex application. #1829

Fix. Symfony integration when starting with the micro Flex application. #1829

Uh oh!

Conversation

fgrandjean commented Apr 9, 2018

Uh oh!

soyuka Apr 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dunglas May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

soyuka Apr 9, 2018 •

edited

Loading

Simperfit Apr 10, 2018 •

edited

Loading

Simperfit Apr 10, 2018 •

edited

Loading

dunglas May 3, 2018 •

edited

Loading