Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Smarter default for framework.annotations#20749
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| ->info('annotation configuration') | ||
| ->canBeDisabled() | ||
| ->children() | ||
| ->booleanNode('enabled')->defaultValue(class_exists('Doctrine\Common\Annotations\Annotation'))->end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You can useAnnotation::class here.
dunglas commentedDec 5, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
👍 (travis error not related) |
| ->children() | ||
| ->arrayNode('annotations') | ||
| ->info('annotation configuration') | ||
| ->canBeDisabled() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should be removed I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, I can't because I still need the logic insidecanBeDisabled 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.phpindex 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hum...The two codes slightly differ. With the patch above:
annotations: cache: php_array
will properly enable annotations whereas the current implementation doesn't (which is wrong).
So I'll apply the patch.
…f doctrine/annotations is installed
chalasr commentedDec 7, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
👍 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 |
ogizanagi commentedDec 13, 2016
@fabpot What do you think about this one? It'll be great to have it for 3.2.2 IMHO. |
stof commentedDec 13, 2016
this indeed makes sense |
fabpot commentedDec 14, 2016
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).