[FrameworkBundle] Ignore AnnotationException exceptions in the AnnotationsCacheWarmer #20959
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Coding standard issues are unrelated to my change, should I still apply them ? |
fabpot
reviewed
Dec 17, 2016
| @@ -75,6 +76,8 @@ public function warmUp($cacheDir) | |||
| $this->readAllComponents($reader, $class); | |||
| } catch (\ReflectionException $e) { | |||
| // ignore failing reflection | |||
| } catch (AnnotationException $e) { | |||
|
|
|||
There was a problem hiding this comment.
You should add a comment explaining why
|
@fabpot I added a short explanation and a link that reference this PR. Also I will ask again, should I apply the suggested coding standard changes even if they are not related to my code ? |
|
@fancyweb No, please ignore them. |
| * environment but is always added to the annotations.map file by some Symfony default behaviors, | ||
| * and you always end up with a not found Annotation. | ||
| * | ||
| * cf https://github.com/symfony/symfony/pull/20959 for a longer explanation. |
There was a problem hiding this comment.
this should be removed, we don't add links to github: the git history (git blame) should have everything instead (which it has)
|
Isn't this a bug fix? |
|
Thank you @fancyweb. |
nicolas-grekas
added a commit
that referenced
this pull request
Dec 27, 2016
… the AnnotationsCacheWarmer (fancyweb) This PR was squashed before being merged into the 3.2 branch (closes #20959). Discussion ---------- [FrameworkBundle] Ignore AnnotationException exceptions in the AnnotationsCacheWarmer | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | I ran into a case that led to an `AnnotationException` in the `AnnotationsCacheWarmer`. I deduced that these exceptions should be ignored to not break the cache warming process. The case : * You have a controller in your `AppBundle\Controller\` directory. * You have an annotation in this controller that will never be found (because you made a mistake in the class / annotation name) or not every time : for example your controller is only used in dev env and it uses an annotation that is defined in a vendor bundle that is only under the `require-dev` key in your `composer.json` because you don't need it in your production deployment. So in production, your controller exists but will never be used. However the vendor bundle does not exist. * You dump your autoload files with composer using the `--optimize` option. * You end up with an `AnnotationException`. Why : * In the [`FrameworkExtension`](https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L189), the `**Bundle\Controller\` pattern is added as an "annotated class to compile". * In the [`AddClassesToCachePass`](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/DependencyInjection/AddClassesToCachePass.php#L50), all the classes defined in the `autoload_classmap.php` file are matched against the previous pattern. * Your controller will match. So even if you don't use it at all in your application, the annotation will still be read and the exception will be thrown. Commits ------- 6749569 [FrameworkBundle] Ignore AnnotationException exceptions in the AnnotationsCacheWarmer
|
Yes it is :) |
|
Sorry, I missed the fact that the class wasn't present in 3.1. |
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I ran into a case that led to an
AnnotationExceptionin theAnnotationsCacheWarmer. I deduced that these exceptions should be ignored to not break the cache warming process.The case :
AppBundle\Controller\directory.require-devkey in yourcomposer.jsonbecause you don't need it in your production deployment. So in production, your controller exists but will never be used. However the vendor bundle does not exist.--optimizeoption.AnnotationException.Why :
FrameworkExtension, the**Bundle\Controller\pattern is added as an "annotated class to compile".AddClassesToCachePass, all the classes defined in theautoload_classmap.phpfile are matched against the previous pattern.