contao / core

Contao 3 → see contao/contao for Contao 4
GNU Lesser General Public License v3.0
492 stars 213 forks source link

Cache + insert_module|uncached + Custom template name = critical error #8643

Closed KaiserCh closed 7 years ago

KaiserCh commented 7 years ago

I've found an error in Contao 3.5.x in a rather odd situation.

If you use {{insert_module::*|uncached}} to insert a module (maybe displaying session-related infos) into a cached page while:

Contao will crash, the error message speaking of "template file not found".

If the template file uses the default file name but is located in a subfolder of /templates, it MIGHT load the default file from /system/modules/... Didn't test this. If the template file uses a custom file name and is located directly in /templates, everything works fine.

The source of the problem is the way template files are processed combined with Inserttag-replacing and the cache, starting at Controller::getTemplate(). In this procedure, there is a call to the $objPage global variable. In an uncached environment, this variable is set and "knows" if the template might be located in a subfolder, since it knows the theme for the page, and the theme knows its theme template folder. But in a cached environment, $objPage is never set, so calling it here just returns something bogus. Maybe NULL, maybe a stdObject... but definitly nothing knowing anything about template paths. And since $objPage->templateGroup does not return anything, the TemplateLoader looks in the autoloader and /template/ for a file with the correct file name.

One possible solution would be to enforce the existence of $objPage inside Controller::getTemplate(). For any regular situation (uncached pages or cached paged without uncached insert_modules), nothing would happen since $objPage is (or should be) filled. For the buggy situation, the additional loading time & database queries for filling $objPage would be much better than a crash.

leofeyer commented 7 years ago

This is odd indeed. @contao/developers

ausi commented 7 years ago

Is {{insert_module::*|uncached}} a supported case for modules that need theme information?

I think the uncached flag should only be used on insert-tags that don’t need $objPage.

KaiserCh commented 7 years ago

In the end, every module needs theme/template information, since every module or content element supports $this->customTpl per default. So either support |uncached for every module or for none.

Also: If I'm not mistaken, the issue also exists for content elements (and maybe more). I didn't test it, but since both Module and ContentElement use BaseTemplate::parse(), and both modules and CEs support custom templates from /template/subdirectory/, the results are predictable. The bug should be triggerable with almost everything. {{insert_article::}} calls ModuleArticle. Or take a look at {{insert_form::}}: Form uses $this->Template->parse(). While forms don't support customTpl (=> templates with names other than form.html5), it DOES support placing a custom form.html5 in a subdirectory of /template/. Also, every widget supports customTpl. So if you insert a uncached form, you'll insert the widgets uncached, triggering the bug.

If my assumption regarding ContentElement, Form, Widget and ModuleArticle is correct, than none of the {{insert_*}} tags will work with |uncached.

Toflar commented 7 years ago

In Contao 4 we could easily fix this by passing the page id to the ESI request for uncacheable insert tags and load the page again. However, this is a general caching problem. There might be a lot more than just the current page object that is needed by insert tags.

KaiserCh commented 7 years ago

Well, the conditional code starting if $objPage is not set WILL be an ugly bitch. In the end, it's half of the stuff contained in FrontendIndex::run()....

Except, if we find a nice little hack: What would happen if, on creating the frontend cache file, we add another value? The cache files already contain their timestamp. So why not also add the page id to the file, in another global variable? So if Controller::getTemplate() finds $objPage empty (or: not of type \PageModel), we just try to load the global pageid-variable and use it for a database call.

Toflar commented 7 years ago

Again, doing this with Edge Side Includes is way easier, works with reverse proxies and is the proper solution. Changing this in Contao 3.5 is not an option at all in my opinion anyway.

KaiserCh commented 7 years ago

This will lead to 3 options:

I've just tested what happens with {{insert_content::*|uncached}} and a template file with the default name but modified content in a theme subdirectory. As the code promised: the template modifications are lost. Using the default template name (in this case: ce_text.html5) does not result in a critical error, since a matching file name is found in /system/modules/core/..., but the content of the templates changes. And as administrators, who develop template modifications, are usually logged into backend and disable the cache, they would never know.

fritzmg commented 7 years ago

I always saw this as a known limitation for the |uncached parameter for insert tags.

KaiserCh commented 7 years ago

In this case, it's an undocumented limitation... and especially one, that makes all include_* - tags unreliable when used with |uncached.

aschempp commented 7 years ago

I'd consider this a known limitation as well. I never used that flag personally…

fritzmg commented 7 years ago

In this case, it's an undocumented limitation...

Indeed. At the very least it should be documented somewhere in the official Contao Docs. May be as an important note here. Can you provide a PR?

KaiserCh commented 7 years ago

I know, you meant a PR for the Contao Docs, but a fix for the bug was the more interesting challenge: #8649

leofeyer commented 7 years ago

See #8649.