[2.8]Fix optional cache warmers are always instantiated whereas they should be lazy-loaded #23014
Conversation
|
For the record, passing a |
7ac016f to
70cd98f
Compare
|
Isn't it true for any cache warmer though ? As soon as a cache warmer has deps, booting requires to instantiate the object graph |
|
@stof I don't know. What I'm sure is that is a regression from Symfony 2.7 |
|
FYI here are the services that are instanciated on cache warm: |
|
FYI the |
70cd98f to
2050e03
Compare
|
PR updated, I've fixed the three optional cache warmers |
2050e03 to
922e699
Compare
|
Woops ! It's now fixed |
|
Can you add a note on each cache warmer class to explain why we want to inject the Container instead of the real service? That would avoid PRs that remove that later on. |
| @@ -22,16 +23,18 @@ | |||
| */ | |||
| class RouterCacheWarmer implements CacheWarmerInterface | |||
| { | |||
| private $container; | |||
| protected $router; | |||
There was a problem hiding this comment.
Let's revert changes on this file, it's a BC break because of this protected property.
| * @param RouterInterface $router A Router instance | ||
| */ | ||
| public function __construct(RouterInterface $router) | ||
| public function __construct($container) |
There was a problem hiding this comment.
I would had a phpdocs with the 2 possible classes
8f7bf58 to
b93d61b
Compare
|
PR updated comments addressed |
b93d61b to
97bdf40
Compare
| { | ||
| $this->translator = $translator; | ||
| /** |
| * This is an optional cache warmer. | ||
| * It means this cache is not warm on boot but on explicit cache warm. | ||
| * To avoid loading the translator service on boot, we inject the container here. | ||
| */ |
There was a problem hiding this comment.
What about:
As this cache warmer is optional, dependencies should be lazy-loaded, that's why a container should be injected.
| private $translator; | ||
|
|
||
| public function __construct(TranslatorInterface $translator) | ||
| public function __construct($container) |
There was a problem hiding this comment.
Can you add the phpdoc about the possible classes accepted for $container?
97bdf40 to
3371db9
Compare
|
PR Updated comments addressed |
|
Thank you @romainneutron. |
…ereas they should be lazy-loaded (romainneutron) This PR was merged into the 2.8 branch. Discussion ---------- [2.8]Fix optional cache warmers are always instantiated whereas they should be lazy-loaded | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files --> | BC breaks? | no | Deprecations? | no <!-- don't forget updating UPGRADE-*.md files --> | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A Since version 2.8, if a Twig extension throws an exception in constructor, then the Kernel can not boot anymore because a service used by the Twig cache warmer instantiates Twig. As Twig cache warmer is optional, Twig should not be loaded at Kernel boot, and this patch fixes the issue Commits ------- 3371db9 Fix optional cache warmers are always instantiated whereas they should be lazy-loaded
Since version 2.8, if a Twig extension throws an exception in constructor, then the Kernel can not boot anymore because a service used by the Twig cache warmer instantiates Twig. As Twig cache warmer is optional, Twig should not be loaded at Kernel boot, and this patch fixes the issue