[FrameworkBundle] Smarter default for framework.annotations #20749
Conversation
| @@ -604,6 +604,7 @@ private function addAnnotationsSection(ArrayNodeDefinition $rootNode) | |||
| ->info('annotation configuration') | |||
| ->canBeDisabled() | |||
| ->children() | |||
| ->booleanNode('enabled')->defaultValue(class_exists('Doctrine\Common\Annotations\Annotation'))->end() | |||
There was a problem hiding this comment.
You can use Annotation::class here.
|
👍 (travis error not related) |
| @@ -604,6 +605,7 @@ private function addAnnotationsSection(ArrayNodeDefinition $rootNode) | |||
| ->info('annotation configuration') | |||
| ->canBeDisabled() | |||
There was a problem hiding this comment.
No, I can't because I still need the logic inside canBeDisabled but I'm simply overriding the boolean node and its default value.
However, if it seems less "hacky", I can replace the fix by:
diff --git a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
index 4b9af8c..4a7dfa2 100644
--- a/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
+++ b/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php
@@ -603,9 +603,8 @@ class Configuration implements ConfigurationInterface
->children()
->arrayNode('annotations')
->info('annotation configuration')
- ->canBeDisabled()
+ ->{class_exists(Annotation::class) ? 'canBeDisabled' : 'canBeEnabled'}()
->children()
- ->booleanNode('enabled')->defaultValue(class_exists(Annotation::class))->end()
->scalarNode('cache')->defaultValue('php_array')->end()
->scalarNode('file_cache_dir')->defaultValue('%kernel.cache_dir%/annotations')->end()
->booleanNode('debug')->defaultValue($this->debug)->end()
There was a problem hiding this comment.
Hum...The two codes slightly differ. With the patch above:
annotations:
cache: php_arraywill properly enable annotations whereas the current implementation doesn't (which is wrong).
So I'll apply the patch.
…f doctrine/annotations is installed
|
👍 Upgrading was hard for me too on LexikJWTAuthenticationBundle which requires the framework in 2.8+ and don't make use of annotations at all, tests was broken on 3.2.
Status: reviewed |
|
@fabpot What do you think about this one? It'll be great to have it for 3.2.2 IMHO. |
|
this indeed makes sense |
|
Thank you @ogizanagi. |
…s (ogizanagi) This PR was merged into the 3.2 branch. Discussion ---------- [FrameworkBundle] Smarter default for framework.annotations | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yesish (could be considered as a minor BC break) | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A `framework.annotations` default should be true only if `doctrine/annotations` is installed. Indeed, in #20097, the dependency on `doctrine/annotations` was removed from the framework bundle. Thus, an application can break (not talking from one actually relying on annotations) as soon as it uses the framework bundle without the `framework.annotations` key explicitly set to `false` (I had the case in a fixture application in the testsuite of a package). Commits ------- e38be09 [FrameworkBundle] framework.annotations default should be true only if doctrine/annotations is installed
framework.annotationsdefault should be true only ifdoctrine/annotationsis installed.Indeed, in #20097, the dependency on
doctrine/annotationswas removed from the framework bundle.Thus, an application can break (not talking from one actually relying on annotations) as soon as it uses the framework bundle without the
framework.annotationskey explicitly set tofalse(I had the case in a fixture application in the testsuite of a package).