Add custom twig loader to workaround cache issue

[xanido]

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

[lsmith77]

one of the unit tests needs to be fixed. also there are some minor CS issues (specifically missing space after if and before (

@stof any feedback here?

[lsmith77]

also it would be good to have a functional test to illustrate that this works.

[stof]

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

[xanido]

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?

[stof]

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

[xanido]

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.

[xanido]

@stof, I believe you are incorrect in your diagnosis. When determining the class cache name, Twig_Environment::getTemplateClass() uses the return value from Twig_LoaderInterface::getCacheKey(). The behaviour of the default Twig_Loader_Filesystem::getCacheKey() is to simply use the value returned by findTemplate(), which returns the full path to the file.

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.

[lsmith77]

@stof can you check once more?

[lsmith77]

alternatively @dantleech do you have time to look at this PR?

[dantleech]

It would be better to delegate this to a method:

if (true == ...) {
    $this->overrideTwigLoader($container);
}

[dantleech]

Would be great to add some tests for the compiler pass and the Loader (using prophecy would make this task easier).

[dantleech]

The method is called resolveContainerAlias no?

[xanido]

Thanks for taking a look @dantleech! Lots of feedback there, I'll try to address it today and update this PR.

[dbu]

ping @xanido

[lsmith77]

anyone have time to wrap this up?

[ahilles107]

it wasn't fixed with #123 ?

[lsmith77]

@dantleech / @xanido can you get this wrapped up?

[lsmith77]

ping