Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[TwigBundle] remove cache warmers when Twig cache is disabled#28029
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
xabbuh commentedJul 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 |
| if (false ===$config['cache']) { | ||
| $container->removeDefinition('twig.cache_warmer'); | ||
| $container->removeDefinition('twig.template_cache_warmer'); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
IIRCTemplateCacheCacheWarmer is the dedicated warmer fortemplating layer (if enabled), and it's necessary while this layer exists. In the other hand,TemplateCacheWarmer is the new version withouttemplating layer/dependecy.
yceruto commentedJul 23, 2018
This part should be updated as well: symfony/src/Symfony/Bundle/TwigBundle/DependencyInjection/Compiler/ExtensionPass.php Lines 52 to 55 ine7f5076
|
| $container->getDefinition('twig.cache_warmer')->replaceArgument(2,$paths); | ||
| } | ||
| $container->getDefinition('twig.template_iterator')->replaceArgument(2,$paths); |
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.
variable$paths is undefined iftwig.cache_warmer doesn't exist, it must be included in the newif 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
either this service should be also removed when cache is disabled, or its configuration here should not be disabled when disabling the cache warming.
fabpot commentedSep 4, 2018
@xabbuh Can you finish this PR? |
xabbuh commentedSep 4, 2018
comments should be addressed now |
fabpot commentedSep 4, 2018
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