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

Files: 
CommentFileSizeAuthor
#24 interdiff.txt566 bytesCottser
#24 2369981-24.patch4.09 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,444 pass(es). View

Comments

joelpittet’s picture

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

lauriii’s picture

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

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.

Cottser’s picture

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

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?

Cottser’s picture

Cottser’s picture

Priority: Normal » Major

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

Fabianx’s picture

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.

Cottser’s picture

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 :)

Cottser’s picture

Assigned: Unassigned » Cottser

Playing around with this today.

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Active » Needs review
FileSize
1.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,209 pass(es). View

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.

Cottser’s picture

FileSize
1.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,265 pass(es), 1 fail(s), and 0 exception(s). View
2.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,174 pass(es). View

Test, interdiff = test-only patch.

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

Cottser’s picture

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

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

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.

Cottser’s picture

Assigned: Unassigned » Cottser

:)

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs work » Needs review
FileSize
3.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,302 pass(es). View
933 bytes

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

Fabianx’s picture

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.

Cottser’s picture

Assigned: Unassigned » Cottser

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

Cottser’s picture

Assigned: Cottser » Unassigned
Status: Needs work » Needs review
FileSize
3.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,435 pass(es), 0 fail(s), and 4 exception(s). View
1.17 KB

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

Fabianx’s picture

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.

Cottser’s picture

Status: Needs work » Needs review
FileSize
4.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,444 pass(es). View
566 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

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

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.