LiipThemeBundle icon indicating copy to clipboard operation
LiipThemeBundle copied to clipboard

Add custom twig loader to workaround cache issue

Open xanido opened this issue 10 years ago • 14 comments

Proof of concept solution for liip/LiipThemeBundle#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

xanido avatar Feb 09 '15 21:02 xanido

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 avatar Feb 10 '15 08:02 lsmith77

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

lsmith77 avatar Feb 10 '15 08:02 lsmith77

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

stof avatar Feb 10 '15 08:02 stof

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 avatar Feb 10 '15 11:02 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.

xanido avatar Feb 10 '15 22:02 xanido

@stof can you check once more?

lsmith77 avatar Feb 19 '15 22:02 lsmith77

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

lsmith77 avatar Mar 14 '15 06:03 lsmith77

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

dantleech avatar Mar 14 '15 07:03 dantleech

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

xanido avatar Mar 15 '15 23:03 xanido

ping @xanido

dbu avatar Mar 24 '15 12:03 dbu

anyone have time to wrap this up?

lsmith77 avatar Apr 13 '15 06:04 lsmith77

it wasn't fixed with #123 ?

ahilles107 avatar May 28 '15 08:05 ahilles107

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

lsmith77 avatar Aug 28 '15 18:08 lsmith77

ping

lsmith77 avatar Oct 27 '15 18:10 lsmith77