[2.8]Fix optional cache warmers are always instantiated whereas they should be lazy-loaded #23014
Conversation
|
For the record, passing a |
romainneutron
force-pushed
the
fix-twig-template-cache
branch
from
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 |
romainneutron
force-pushed
the
fix-twig-template-cache
branch
from
70cd98f to
2050e03
Compare
|
PR updated, I've fixed the three optional cache warmers |
romainneutron
changed the title
fabpot
left a comment
fabpot
left a comment
There was a problem hiding this comment.
For the two additional cache warmers, you forgot to update the XML definitions.
romainneutron
force-pushed
the
fix-twig-template-cache
branch
from
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. |
| 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
romainneutron
force-pushed
the
fix-twig-template-cache
branch
2 times, most recently
from
8f7bf58 to
b93d61b
Compare
|
PR updated comments addressed |
romainneutron
force-pushed
the
fix-twig-template-cache
branch
from
b93d61b to
97bdf40
Compare
| public function __construct($container) | ||
| { | ||
| $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?
romainneutron
force-pushed
the
fix-twig-template-cache
branch
from
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
4 participants
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