Update our Twig code to be ready for Twig 2.x

Reviewed the test changes and they all look great.

1. StringLoader replaces the removal of the Twig_Loader_String().
2. Tests need access to some non-unittestable functions for t() and file_create_url()
3. Twig_SimpleFilter replaces Twig_Filter_Function
4. And cache file name comes from generateKey() method.

Thanks @Cottser.

1. +++ b/core/modules/system/tests/modules/twig_extension_test/src/TwigExtension/TestExtension.php
   @@ -12,7 +12,7 @@
   -class TestExtension extends TwigExtension {
   +class TestExtension extends \Twig_Extension {
   

   This means we are no longer using use Drupal\Core\Template\TwigExtension;

   Also is this really correct - what ware we testing now?

2. +++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
   @@ -241,3 +243,16 @@ public function __toString() {
   +namespace {
   +  if (!function_exists('t')) {
   +    function t($string, array $args = []) {
   +      return strtr($string, $args);
   +    }
   +  }
   +  if (!function_exists('file_create_url')) {
   +    function file_create_url() {}
   +  }
   +}
   

   Why do we have to add this? Commenting this out the tests still pass.

@alexpott the first thing is showing how a contrib module should be extending twig and that we can extend twig. So to me it's testing how people should be extending Twig, beacuse it's a Test Twig Extension not an "extension" of Drupal's Twig Extension.

I was seeing fails on Scott's computer on the second part of that but only when you ran this with the upgrade. Chalked it up to Unittestible requirements and something funky in 2.x. Did you run it with the upgrade to 2.x?

@joelpittet nope I didn't - but it is worth understanding why this is necessary for 2.x

I'm not sure why those were needed after and not before... kinda a mystery that @Cottser mentioned. I'd actually expect them needed for both if they are used function in the unit tests.

FYI I tested it with 2.x again, and @alexpott yes those are definitely needed. Maybe it's just better at shielding itself from DrupalKernal::loadLegacyIncludes() or something?

Also I noticed the date format filter was not working because it's not initialized when the filters are being created so it doesn't think it's callable till runtime.

This fixes that...

-      new \Twig_SimpleFilter('format_date', array($this->dateFormatter, 'format')),
+      new \Twig_SimpleFilter('format_date', array($this, 'formatDate')),
...
+  public function formatDate($timestamp, $type = 'medium', $format = '', $timezone = NULL, $langcode = NULL) {
+    return $this->dateFormatter->format($timestamp, $type, $format, $timezone, $langcode);
+  }

Not sure which issue I should add that small shuffle around to?

@alexpott and @Cottser the reason it fails is that twig 2.0 removed a feature it had where it would register globals.

"removed the ability to register a global variable after the runtime or the extensions have been initialized"

I'm quite sure that is the reason why.

@joelpittet hmmm but functions are not global variables.

Yes right, my guess was due to that was they removed a reflection hunk... Closest thing I could find to why. I put a breakpoint in common.inc and it ran just the same. It could be something else but that looked most probable to me.

Can someone update the issue summary with steps of how to test with Twig 2.0.x - I wanna find out what is really going on here.

@alexpott I'll try in a bit but it's a PITA because of composer dependencies and https://github.com/jcalderonzumba/MinkPhantomJSDriver is requiring ~1.8.

You have to remove it from the core/composer.json, update composer in the root, then update twig in core/composer.json, and update composer in the root again. I don't know composer well enough to know of an easier way.

I'm currently running TravisCI on a forked copy of that to see how it fairs on 2.x as well.

Ok this gets around the dependency, I forked his repo. Hope that helps for testing.

Just apply this patch and run composer update from root.

So the reason why t() has to be defined is

      // Translation filters.
      new \Twig_SimpleFilter('t', 't', array('is_safe' => array('html'))),
      new \Twig_SimpleFilter('trans', 't', array('is_safe' => array('html'))),

And the reason why file_create_url() has to be defined is

      new \Twig_SimpleFunction('file_url', 'file_create_url'),

Twig 2.x has started to check that the second arguments are callable unlike Twig 1.x - I ponder what the performance cost of Twig 2.x is. I think with the t() it is worth adding a method to the TwigExtension that just returns TranslatableMarkup object.

Heads up you don't need my patch or fork in #20. My pull request was accepted and the composer update works now without it:)

#10 needs a re-roll for 8.4.x

Rerolled and also removed the deprecations that this fixes. I think we should do this regardless of #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1 since doing this first helps minimise what that issue has to do.

The patch introduces some unnecessary code.

Why not remove all the deprecations related to twig 2.0?

@jibran then we need to merge this and #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1 together. Let's merge them.

Actually as that issue is changing composer.json let's leave that to do that and this issue should do all the deprecations. Changing composer.json and fixing the depreecations are unrelated activities.

So this patch removes all the twig skipped deprecations and removes the legacy group from a load of twig tests where it shouldn't be. Let's see where we are.

Crediting people from #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1

One deprecation to fix.

Trying to remove all unnecessary change to any run-time code.

All the twig deprecations are gone now code changes look good as well but I'll leave RTBC for someone who is more familiar with theme system.

Rerolled due to changes in \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations().

Just spotted this got some action, here's a review, mostly a big +1, thanks @alexpott!

1. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
   @@ -18,12 +18,17 @@ class TwigNodeTrans extends \Twig_Node {
   +    $nodes['body'] = $body;
   +    if (NULL !== $count) {
   +      $nodes['count'] = $count;
   +    }
   +    if (NULL !== $plural) {
   +      $nodes['plural'] = $plural;
   +    }
   +    if (NULL !== $options) {
   +      $nodes['options'] = $options;
   +    }
   +    parent::__construct($nodes, [], $lineno, $tag);
   

   Why is this change needed? And out of curiosity why is the value NULL in front of the condition? Feels a bit less natural to read, is there an advantage?

2. +++ b/core/modules/system/tests/modules/twig_extension_test/src/TwigExtension/TestExtension.php
   @@ -21,7 +21,7 @@ class TestExtension extends \Twig_Extension {
   -      'testfunc' => new \Twig_Function_Function(['Drupal\twig_extension_test\TwigExtension\TestExtension', 'testFunction']),
   +      new \Twig_SimpleFunction('testfunc', [$this, 'testFunction']),
   
   @@ -39,7 +39,7 @@ public function getFunctions() {
   -      'testfilter' => new \Twig_Filter_Function(['Drupal\twig_extension_test\TwigExtension\TestExtension', 'testFilter']),
   +      new \Twig_SimpleFilter('testfilter', [$this, 'testFilter']),
   

   +1

3. +++ b/core/modules/system/tests/src/Functional/Theme/TwigSettingsTest.php
   @@ -96,7 +96,11 @@ public function testTwigCacheOverride() {
   -    $cache_filename = $this->container->get('twig')->getCacheFilename($template_filename);
   

   +1

4. +++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
   @@ -336,7 +331,8 @@ public function testRenderVarWithGeneratedLink() {
   -    $loader = new StringLoader();
   +    $name = '__string_template_test_1__';
   +    $loader = new \Twig_Loader_Array([$name => "{% for iteration in iterations %}<div{{ create_attribute(iteration) }}></div>{% endfor %}"]);
   

   Couldn't this stay a StringLoader?

1. To avoid the deprecation notice in \Twig_Node::__construct()
4. It appears it can. Reverted the changes. Pretty sure they came from #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1

1. I couldn't find where the Twig_Node deprecation notice would occur due to the previous code but I trust you ran into it and had to make this change.
4. Cool, yeah it looks like it did come from there. Thanks for reverting, it looks more straight forward with stringloader.

Slightly tweaking the issue summary and title to reflect that there are a couple of run-time (non test) changes.

@joelpittet re #53.1 see \Twig_Node::__construct() especially:

        foreach ($nodes as $name => $node) {
            if (!$node instanceof Twig_NodeInterface) {
                @trigger_error(sprintf('Using "%s" for the value of node "%s" of "%s" is deprecated since version 1.25 and will be removed in 2.0.', is_object($node) ? get_class($node) : null === $node ? 'null' : gettype($node), $name, get_class($this)), E_USER_DEPRECATED);
            }
        }

Bumping to critical given https://twitter.com/fabpot/status/1024222511746166784

@alexpott I see now, sorry wasn't looking at the args change in the parent! Thanks for pointing it out again

Rerolled now that #2991080: Remove skipped deprecations no longer triggered by tests has landed.

Committed/pushed to 8.7.x, thanks!

While I agree it's critical to make Drupal 8 compatible with Twig 2 in the same way #2937984: [META] Symfony 4.0 compatibility is critical, this doesn't seem urgent in that there's no indication of Twig 1 support being dropped soon - and not necessarily a way for us to update from Twig 1 to Twig 2 within 8.x either. So leaving in 8.7.x

🚀shipped! Thanks

Given that this is in #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1 attempts to test an update to Twig 2, but it still fails and needs further modifications. Would be lovely if people could jump over there and help :) Thanks!

Here is version of patch for 8.6.x for people which would like to use.