[TwigBundle] remove cache warmers when Twig cache is disabled #28029
Conversation
xabbuh
commented
Jul 23, 2018
| Q | A |
|---|---|
| Branch? | 2.8 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #28009 |
| License | MIT |
| Doc PR |
| @@ -132,6 +132,11 @@ public function load(array $configs, ContainerBuilder $container) | |||
|
|
|||
| $container->getDefinition('twig')->replaceArgument(1, $config); | |||
|
|
|||
| if (false === $config['cache']) { | |||
| $container->removeDefinition('twig.cache_warmer'); | |||
| $container->removeDefinition('twig.template_cache_warmer'); | |||
There was a problem hiding this comment.
Does someone remember why we have two cache warmers? Both classes aim to solve the same problem. Is it still necessary to keep both?
There was a problem hiding this comment.
I looked at this a LONG time ago for #24608, but I can't remember what my conclusion was. Iirc, they DO indeed do the same thing, but maybe one has one slightly additional feature. Not too helpful, I know :)
There was a problem hiding this comment.
IIRC TemplateCacheCacheWarmer is the dedicated warmer for templating layer (if enabled), and it's necessary while this layer exists. In the other hand, TemplateCacheWarmer is the new version without templating layer/dependecy.
|
This part should be updated as well: |
| $paths[$coreThemePath] = null; | ||
| $container->getDefinition('twig.cache_warmer')->replaceArgument(2, $paths); | ||
| } | ||
|
|
||
| $container->getDefinition('twig.template_iterator')->replaceArgument(2, $paths); |
There was a problem hiding this comment.
variable $paths is undefined if twig.cache_warmer doesn't exist, it must be included in the new if condition.
| $paths = $container->getDefinition('twig.cache_warmer')->getArgument(2); | ||
| $paths[$coreThemePath] = null; | ||
| $container->getDefinition('twig.cache_warmer')->replaceArgument(2, $paths); | ||
| $container->getDefinition('twig.template_iterator')->replaceArgument(2, $paths); |
There was a problem hiding this comment.
either this service should be also removed when cache is disabled, or its configuration here should not be disabled when disabling the cache warming.
|
@xabbuh Can you finish this PR? |
|
comments should be addressed now |
|
Thank you @xabbuh. |
…led (xabbuh) This PR was merged into the 2.8 branch. Discussion ---------- [TwigBundle] remove cache warmers when Twig cache is disabled | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28009 | License | MIT | Doc PR | Commits ------- ef1f7ff remove cache warmers when Twig cache is disabled