Add custom twig loader to workaround cache issue #116
Conversation
|
one of the unit tests needs to be fixed. also there are some minor CS issues (specifically missing space after @stof any feedback here? |
|
also it would be good to have a functional test to illustrate that this works. |
| @@ -23,6 +23,8 @@ public function process(ContainerBuilder $container) | |||
|
|
|||
| $container->setAlias('templating.cache_warmer.template_paths', 'liip_theme.templating.cache_warmer.template_paths'); | |||
|
|
|||
| $container->setAlias('twig.loader', 'liip_theme.twig.loader.filesystem'); | |||
There was a problem hiding this comment.
this is wrong. You should not replace all loaders by the liip_theme one, because it breaks things for people using more than the Twig filesystem loader
There was a problem hiding this comment.
I knew that this wasn't ideal when I did it. I've had a go at doing this in a more flexible way. Do you have any thoughts on this?
|
This won't work as is, because the class name for the compiled template still does not take the theme into account, so the Twig_Environment will not load the template a second time |
|
Ah! Good point, I hadn't considered that. I'll have a look and see what changes would be necessary to solve the compiled class name issue. |
|
@stof, I believe you are incorrect in your diagnosis. When determining the class cache name, Twig_FilesystemLoader is an ancestor of our new loader so we have no problem, because findTemplate() returns the full path to the template, including the theme name (e.g. 'themes/themename'). I will fix the CS, the broken test, and create a new test for this feature. |
|
@stof can you check once more? |
|
alternatively @dantleech do you have time to look at this PR? |
| @@ -23,9 +23,34 @@ public function process(ContainerBuilder $container) | |||
|
|
|||
| $container->setAlias('templating.cache_warmer.template_paths', 'liip_theme.templating.cache_warmer.template_paths'); | |||
|
|
|||
| if (true === $container->hasDefinition('twig')) { | |||
There was a problem hiding this comment.
It would be better to delegate this to a method:
if (true == ...) {
$this->overrideTwigLoader($container);
}
|
Would be great to add some tests for the compiler pass and the Loader (using prophecy would make this task easier). |
| @@ -23,9 +23,34 @@ public function process(ContainerBuilder $container) | |||
|
|
|||
| $container->setAlias('templating.cache_warmer.template_paths', 'liip_theme.templating.cache_warmer.template_paths'); | |||
|
|
|||
| if (true === $container->hasDefinition('twig')) { | |||
| $twigLoader = $container->findDefinition('twig.loader'); | |||
| $aliasedTo = $this->resolveAlias('twig.loader', $container); | |||
There was a problem hiding this comment.
The method is called resolveContainerAlias no?
|
Thanks for taking a look @dantleech! Lots of feedback there, I'll try to address it today and update this PR. |
|
ping @xanido |
|
anyone have time to wrap this up? |
|
it wasn't fixed with #123 ? |
|
@dantleech / @xanido can you get this wrapped up? |
|
ping |
Proof of concept solution for #79.
Apologies if I have missed something, but this seems as though it can be solved relatively simply by using a custom Twig filesystem loader, as stof suggested. This solution does not solve the 'referencing a non-themed template from a themed template' issue.
Suggestions are welcome.
fix #79