deprecated the String loader

[fabpot]

Even if the String loader should never be used, people keep trying to use it. It's too confusing to keep and we don't really need it in tests. So, I propose to deprecate it in 1.x and remove it in 2.x

[ninsuo]

Hi, can you elaborate the reason why it is deprecated? I did not noticed this anywhere.

[fabpot]

I want to deprecate it is too easy to make mistakes when using it. Basically, it does not do what people expect it to do and as such (and because there are better ways), I prefer to remove it. I think I receive at least an email a month about this topic ;)

[stof]

👍

Btw, it would be great to fix this intermittent test failure with the profiler dumper test

[stof]

@ninsuo you should use $env->createTemplate($string) to do what people generally think that the Twig_Loader_String will do. The Twig_Loader_String cannot work properly by design (the first version of the template_from_string function was using it, and it has been changed because it was broken)

[ninsuo]

@stof I do not understand, is the following code potentially invalid?

$env = new \Twig_Environment(new \Twig_Loader_String());
echo $env->render('Hello, {{ name }}', array('name' => 'Bob')); // Hello, Bob

I understand the bad practice, but what it the technical problem here?

[stof]

@ninsuo your example is probably the only code snippet where the usage of string loader will work. But as soon as you make it more complex, it breaks:

• if your code tries to include or extend another template (or any other feature involving other templates, like {% embed %}, {% use %} or importing macros for instance), using the string loader will break everything
• if you enable the twig cache, using the string loader will not play with it fine

And doing the same in a working way is not that difficult:

$env = new \Twig_Environment(new \Twig_Loader_Array());
$template = $env->createTemplate('Hello, {{ name }}');
echo $env->render($template, array('name' => 'Bob')); // Hello, Bob

[ninsuo]

Got it, thanks 👍

[dawehner]

Drupal basically needs the dynamicness of \Twig_Loader_String for some special usecases,
but yeah I understand why it should be removed. Its a bad idea to start with.

See https://www.drupal.org/node/2563821

[ninsuo]

You can do the same as Twig_Loader_String with Twig_Loader_Array, using @stof's syntax above (just put an array inside the constructor). This is really as flexible as Twig_Loader_String... The quick fix you provided to Drupal will not take any mentioned advantages.

[dawehner]

@ninsuo Sure, we could provide workarounds but its tricky, given that we use twig via a generic render system and have the environment stored in the container

[wouterj]

@stof your code does not work ("Catchable Fatal Error: Object of class __TwigTemplate_dd0d0e2c37a2d0f8b67dcdcd30ecb79029586ee23cd1c76f343d6878b2fdbb35 could not be converted to string"). Transforming this into:

$twig = new \Twig_Environment(...);
$template = $twig->createTemplate('Hello {{ name }}!');
echo $template->render(['name' => 'Bob']);

.. suffers from the same problem as Twig_Loader_String, as it will create a cache entry each time you call it. Is there really no way to implement this in a nicer way? (e.g. creating a cache name in createTemplate based on the contents of the template, so the same contents results in the same cache name?)

[fabpot]

@wouterj If you need a (hopefully) solid implementation, have a look at Twig_Environment::createTemplate(): https://github.com/twigphp/Twig/blob/1.x/lib/Twig/Environment.php#L408-L428

[wouterj]

Thanks @fabpot. I discovered that I mixed up the location of the caching (Env::loadTemplate() vs in a loader). Things indeed work as expected using createTemplate.

[fabpot]

I've added a note in b5c7f5c about createTemplate().

[wouterj]

👍

[fabpot]

I've just pushed a PR that adds a recipe about this topic as this seems to be a question that people ask all the time: #1826