Problem/Motivation

When a template is "not found" and we are using the string loader in the Twig loader chain, instead of a useful error/exception you will just see the name of the template you called on screen.

Supplementary to that concern:

Newer versions of Twig say this about the Twig_Extension_StringLoader class:

This loader should only be used for unit testing as it has many limitations (for instance, the include or extends tag does not make any sense for a string loader).

The main reason this was added was to support inline templating: #2289999: Add an easy way to create HTML on the fly without having to create a theme function / template, see also #2317557: renderInline not compatible with twig_auto_reload.

However it very likely presents some issues the way we are currently using it, some of which are highlighted in this upstream issue: https://github.com/symfony/symfony/issues/10865

Proposed resolution

Discuss how we can factor out the string loader from the loader chain or be smarter about how we use it in the chain.

Remaining tasks

  • Discuss
  • Patch

User interface changes

n/a

API changes

TBD

CommentFileSizeAuthor
#24 interdiff.txt566 bytesstar-szr
#24 2369981-24.patch4.09 KBstar-szr
#21 interdiff.txt1.17 KBstar-szr
#21 2369981-21.patch3.53 KBstar-szr
#18 interdiff.txt933 bytesstar-szr
#18 2369981-18.patch3.13 KBstar-szr
#12 2369981-12.patch2.89 KBstar-szr
#12 2369981-12-testonly.patch1.03 KBstar-szr
#11 2369981-11.patch1.86 KBstar-szr

Comments

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented

I'm game for removing this loader from the chain and just adding and removing it when using the inlineRender method:)

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

I also agree that we should only load Twig_Extension_StringLoader class when we actually use InlineRender method. I might start working on this in few days if I have time.

dawehner’s picture

dawehner
Primary language German
commented

This loader should only be used for unit testing as it has many limitations (for instance, the include or extends tag does not make any sense for a string loader).

... just curious, do we have an actual problem with it? Both examples given in that comment are kind of pointless, given how we use inline templates.
They are simple templates anyway, nothing which would use extends or includes.

But sure, if this cleans up the code, just add it dynamically, we do have pretty good test coverage now, so we can refactor the internal details.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue tags: +DX (Developer Experience), +TX (Themer Experience)

The bigger issue is this (from the IS):

…when a template is not found and you are using the string loader in the Twig loader chain, instead of a useful error/exception you will just see the name of the template you called on screen.

joelpittet’s picture

joelpittet
he/him
Primary language English
Location Vancouver
commented
Title: Refactor usage of Twig's string loader » Refactor Twig's string loader in the loader chain
Issue summary: View changes

Moved that concern to the top of the Issue Summary, rewrote a few things and re-titled for a bit of clarity on what we are trying to solve, hopefully that is of some help?

star-szr’s picture

star-szr
he/him
Primary language English
commented

Just want to give this a bit of a bump now that #2291449: Add Twig template inheritance based on the theme registry, enable adding Twig loaders is in.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Priority: Normal » Major

We need to show real errors/exceptions when calling templates that don't exist, bumping to major.

fabianx’s picture

fabianx commented

There is a very simple fix possible.

It is kinda a hack, but it is very simple and should work well:

Have every inline template, thing calling directly to the string loader, be start with:

'{% ILT %}' . $template

It is unlikely any filename will ever start with that or any other MAGIC string.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Issue tags: +Needs tests

Interesting, something like that could certainly work so that we can properly differentiate inline templates. Nice way of framing the problem at the very least. Thanks @Fabianx :)

star-szr’s picture

star-szr
he/him
Primary language English
commented
Assigned: Unassigned » star-szr

Playing around with this today.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Assigned: star-szr » Unassigned
Status: Active » Needs review
StatusFileSize
new 2369981-11.patch1.86 KB

Initial patch to test #8 using a special comment: {# inline_template_start #}

It's worth keeping in mind that the string loader is last in the chain, so even if you happened to put this comment string in a template file, it won't go through the string loader. All this patch does is whitelist inline templates only when they start with {# inline_template_start #}.

Regardless of the fix we agree on, we need a test here to ensure the "Template … is not defined" Twig_Error_Loader exception message shows up, I'll work on that while testbot chews on this one.

star-szr’s picture

star-szr
he/him
Primary language English
commented
StatusFileSize
new 2369981-12-testonly.patch1.03 KB
new 2369981-12.patch2.89 KB

Test, interdiff = test-only patch.

The last submitted patch, 12: 2369981-12-testonly.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
commented

Bit of a follow-up to improve things further now that I am seeing the exceptions: #2430981: Unnecessary notices when twig_render_template() catches \Twig_Error_Loader exceptions

fabianx’s picture

fabianx commented
Title: Refactor Twig's string loader in the loader chain » Not found templates are displayed literally instead of throwing an Exception due to string loader
Category: Task » Bug report
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Nice, that works for me :).

RTBC, does this need beta eval? (I re-classified as a bug, because that is what it is).

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Template/Loader/StringLoader.php
@@ -0,0 +1,25 @@
+
+class StringLoader extends \Twig_Loader_String {

Missing a class docblock that explains why we are doing this.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Assigned: Unassigned » star-szr

:)

star-szr’s picture

star-szr
he/him
Primary language English
commented
Assigned: star-szr » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2369981-18.patch3.13 KB
new interdiff.txt933 bytes

Here's the docs update. Thanks @alexpott for catching that!

fabianx’s picture

fabianx commented
Status: Needs review » Needs work

This is nit picking, but the comment does not make sense when not saying this is intended to be used within a chain loading fashion.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Assigned: Unassigned » star-szr

@Fabianx, sure - that was in the back of my mind when writing it. I'll get that updated. Thanks for reviewing!

star-szr’s picture

star-szr
he/him
Primary language English
commented
Assigned: star-szr » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 2369981-21.patch3.53 KB
new interdiff.txt1.17 KB

Spent some time with the docs, hopefully this makes more sense!

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

RTBC, docs look great now!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 21: 2369981-21.patch, failed testing.

star-szr’s picture

star-szr
he/him
Primary language English
commented
Status: Needs work » Needs review
StatusFileSize
new 2369981-24.patch4.09 KB
new interdiff.txt566 bytes

Funny, that is showing the notices that #2430981: Unnecessary notices when twig_render_template() catches \Twig_Error_Loader exceptions takes care of.

A bit of digging reveals that #2378883-32: Convert existing drupal_render() KernelTestBase tests to PHPUnit tests deleted common-test-render-element.html.twig stating "that is now mocked behavior", but that doesn't seem to be the case, at least not fully. So adding that template back should make this come back green.

fabianx’s picture

fabianx commented
Status: Needs review » Reviewed & tested by the community

Nice that this bug fix now showed that we did indeed hide bugs with this in HEAD.

Back to RTBC!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 0bf30e2 and pushed to 8.0.x. Thanks!

  • alexpott committed 0bf30e2 on 8.0.x 
    Issue #2369981 by Cottser: Not found templates are displayed literally...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.