Problem/Motivation

Our tests use things that are deprecated in Twig 2.x or that we discourage people in contrib and so on from doing.

Why this is RC eligible

It only changes tests.

Proposed resolution

Fix them.

Remaining tasks

Patch
Review

User interface changes

n/a

API changes

None

Data model changes

None

Comments

Cottser created an issue. See original summary.

Cottser’s picture

Status: Active » Needs review
FileSize
7.82 KB

Something like this.

Cottser’s picture

Assigned: Cottser » Unassigned
Cottser’s picture

Issue summary: View changes
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

joelpittet’s picture

@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?

alexpott’s picture

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

joelpittet’s picture

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.

Cottser’s picture

Status: Needs work » Needs review
FileSize
7.92 KB
618 bytes

Thank you both!

It's a mystery to me indeed why those changes seem to be needed only for Twig 2.x and not before - like @joelpittet said it seems like if it's a unit test it should be failing to find those procedural functions on HEAD. Someone with more PHPUnit knowledge and experience could probably figure it out.

This fixes #6.1. #6.2 that change could I guess happen in another issue, or maybe in #2591515: Move twig_without() to the TwigExtension where all the other filters are and deprecate because it's similar to that but might also make the scope there a bit weird. Any opinion on these options?

  1. Keep the change here and figure out why it is needed.
  2. Split the change out into its own issue.
  3. Move the change to #2591515: Move twig_without() to the TwigExtension where all the other filters are and deprecate.

Status: Needs review » Needs work

The last submitted patch, 10: update_our_twig_tests-2600154-10.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

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?

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

@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.

The last submitted patch, 2: update_our_twig_tests-2600154-2.patch, failed testing.

alexpott’s picture

@joelpittet hmmm but functions are not global variables.

joelpittet’s picture

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: update_our_twig_tests-2600154-10.patch, failed testing.

alexpott’s picture

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.

joelpittet’s picture

@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.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
1.17 KB

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.

alexpott’s picture

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.

joelpittet’s picture

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Version: 8.3.x-dev » 8.4.x-dev
Issue tags: -rc eligible +Needs reroll

#10 needs a re-roll for 8.4.x

Cottser’s picture

Quick attempted reroll, didn't check it very thoroughly so might be broken.

Cottser’s picture

This issue might become redundant if #2572605: Update to Twig 2.0.0 is taking care of the test changes. Originally, the plan was to get this in first since the test updates work on Twig 1.x and 2.x.