Problem/Motivation

We are currently using some deprecated code that will be removed or not workable in Twig 2.0.

Proposed resolution

Remaining tasks

If/when Twig 2.x requires PHP 5.6, we will need to set the d.o packager to use 1.x and allow composer users to choose 2.x via #platform https://getcomposer.org/doc/06-config.md#platform
@see https://github.com/twigphp/Twig/pull/2229

User interface changes

n/a

API changes

TBD

Data model changes

Should be none.

Files: 

Comments

Cottser created an issue. See original summary.

Cottser’s picture

Status: Active » Needs review
FileSize
10.91 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View

This should be all the changes needed but it'll be good to get a patch that actually updates to Twig 2.x with these changes also :)

Cottser’s picture

Issue summary: View changes
FileSize
393.36 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Actually there should be other changes needed for 2.x, I forgot about the unit testing stuff I found when testing.

Here's a patch with Twig 2.x that should show some fails.

If need be this can be split up but I don't see it growing to be a massive size. This patch is just #2 plus updating composer.json and running composer update twig/twig. The idea of this issue is to be able to get these changes in ahead of when we update to Twig 2.x.

Cottser’s picture

FileSize
10.91 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,898 pass(es). View
393.37 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch twig-2.x_make_necessary_changes-2568181-4.patch. Unable to apply patch. See the log in the details link for more information. View
940 bytes

Oops. New 1.x and 2.x patches.

The last submitted patch, 2: make_necessary_changes-2568181-2.patch, failed testing.

The last submitted patch, 3: twig-2.x_make_necessary_changes-2568181-3.patch, failed testing.

The last submitted patch, 2: make_necessary_changes-2568181-2.patch, failed testing.

The last submitted patch, 3: twig-2.x_make_necessary_changes-2568181-3.patch, failed testing.

Cottser’s picture

FileSize
401.34 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch twig-2.x_make_necessary_changes-2568181-8.patch. Unable to apply patch. See the log in the details link for more information. View
8.8 KB

Right so the Twig 2.x testing patch also needs to incorporate our new cache class otherwise it will fail badly. Getting very messy now, will be nice to get #2568171: Upgrade to Twig 1.22 and implement our own cache class in first.

Interdiff shows adding those cache class changes to the 2.x version.

dawehner’s picture

Status: Needs review » Postponed

Yeah let's wait on that first ...

Cottser’s picture

FileSize
400.93 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch twig_2.x_make_necessary_changes-2568181-10.patch. Unable to apply patch. See the log in the details link for more information. View
1.17 KB

getTemplateClassPrefix() was also removed. Not sure any better way than this, we could add back in our own getter of course.

We override getTemplateClass() for performance reasons but it was reverted upstream because it broke some things for certain use cases outside of Drupal.

Cottser’s picture

Status: Postponed » Needs review

Just NR to get that last one tested, then I can move back to a testing issue to iterate on this.

Cottser’s picture

Status: Needs review » Postponed

The last submitted patch, 4: twig-2.x_make_necessary_changes-2568181-4.patch, failed testing.

The last submitted patch, 9: twig-2.x_make_necessary_changes-2568181-8.patch, failed testing.

Status: Postponed » Needs work

The last submitted patch, 11: twig_2.x_make_necessary_changes-2568181-10.patch, failed testing.

The last submitted patch, 4: twig-2.x_make_necessary_changes-2568181-4.patch, failed testing.

The last submitted patch, 9: twig-2.x_make_necessary_changes-2568181-8.patch, failed testing.

The last submitted patch, 11: twig_2.x_make_necessary_changes-2568181-10.patch, failed testing.

Cottser’s picture

Status: Needs work » Postponed
znerol’s picture

Issue summary: View changes
Cottser’s picture

Status: Postponed » Needs work
Cottser’s picture

Status: Needs work » Needs review
Related issues: +#2571817: Remove race condition handling in TwigEnvironment::loadTemplate(), it's fixed upstream now
FileSize
12.31 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,719 pass(es). View
377.64 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,687 pass(es), 1 fail(s), and 0 exception(s). View
2.37 KB

Here is some progress, still some failures in the 2.x version.

The interdiff is applicable to both 1.x and 2.x patches, but there was a reroll involved too.

#2571817: Remove race condition handling in TwigEnvironment::loadTemplate(), it's fixed upstream now is highly related and should probably go in first.

Status: Needs review » Needs work

The last submitted patch, 23: twig_2.x-make_necessary_changes-2568181-23.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
16.11 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
381.44 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Setup environment: Test cancelled by admin prior to completion. View
4.05 KB

Moving the without filter callback to where it should live and removing the now unneeded require_once from TwigEnvironment, hooray!

Status: Needs review » Needs work

The last submitted patch, 25: twig_2.x-make_necessary_changes-2568181-25.patch, failed testing.

The last submitted patch, 23: twig_2.x-make_necessary_changes-2568181-23.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
16.63 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,733 pass(es). View
381.96 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,704 pass(es). View
845 bytes

Thanks @joelpittet for the hint here, I just find it odd that we never had to do this before. Haven't figured out why just yet.

The last submitted patch, 25: twig_2.x-make_necessary_changes-2568181-25.patch, failed testing.

The last submitted patch, 25: make_necessary_changes-2568181-25.patch, failed testing.

Cottser’s picture

FileSize
16.69 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php. View
389.41 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php. View

Rerolls. The 1.x patch was auto-merged, the 2.x patch was generated by taking the 1.x patch, updating core/composer.json (thanks to @webflo on IRC for the help, need to add @dev because the root composer.json has a different minimum-stability rule), and running composer update twig/twig from the root. loadTemplate() from TwigEnvironment doesn't need to be removed in the 2.x patch anymore because #2571817: Remove race condition handling in TwigEnvironment::loadTemplate(), it's fixed upstream now is in now.

The last submitted patch, 31: make_necessary_changes-2568181-31.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 31: twig_2.x-make_necessary_changes-2568181-31.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
16.46 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,916 pass(es). View
389.18 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
532 bytes

Got too excited by the auto-merge.

For reviewers the Twig 2.x patches are just here to make sure they're green, not to be committed or even really reviewed necessarily.

Status: Needs review » Needs work

The last submitted patch, 34: twig_2.x-make_necessary_changes-2568181-34.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
16.82 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,940 pass(es). View
389.54 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 115,882 pass(es), 1 fail(s), and 0 exception(s). View
664 bytes

#2416857: Add an active_theme_path twig function added a usage of \Twig_Loader_String, this fixes it.

The last submitted patch, 34: twig_2.x-make_necessary_changes-2568181-34.patch, failed testing.

dawehner’s picture

I would suggest in general to split off all changes which would still work with the twig version we use in core in HEAD, so the remaining bits of this patch is as minimal as possible.

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -551,4 +551,37 @@ public function safeJoin(\Twig_Environment $env, $value, $glue = '') {
+   *
+   * @return array
+   *   The filtered renderable array.
+   */
+  public function withoutFilter($element) {
+    if ($element instanceof \ArrayAccess) {

+++ b/core/themes/engines/twig/twig.engine
@@ -118,36 +118,3 @@ function twig_render_template($template_file, array $variables) {
-/**
- * Removes child elements from a copy of the original array.
- *
- * Creates a copy of the renderable array and removes child elements by key
- * specified through filter's arguments. The copy can be printed without these
- * elements. The original renderable array is still available and can be used
- * to print child elements in their entirety in the twig template.
- *
- * @param array|object $element
- *   The parent renderable array to exclude the child items.
- * @param string[] $args, ...
- *   The string keys of $element to prevent printing.
- *
- * @return array
- *   The filtered renderable array.
- */
-function twig_without($element) {
-  if ($element instanceof ArrayAccess) {
-    $filtered_element = clone $element;
-  }
-  else {
-    $filtered_element = $element;
-  }
-  $args = func_get_args();
-  unset($args[0]);
-  foreach ($args as $arg) {
-    if (isset($filtered_element[$arg])) {
-      unset($filtered_element[$arg]);
-    }
-  }
-  return $filtered_element;
-}

Why is this particular change needed. Does twig no longer supports to call to simple functions?

Cottser’s picture

@dawehner thanks for taking a look! That's what the first patch in all these comments is, it's all the changes for HEAD and shouldn't need to be split further IMO because all those changes work for HEAD. The only difference between the 2.x versions of the patch is composer and vendor to prove that things pass and work.

If you can help us figure out why we needed to move twig_without() that would be great. It was moved for unit testing reasons. @joelpittet and I tried to figure it out in BCN to no avail. You can see the fail I'm talking about in the second patch on #23.

Status: Needs review » Needs work

The last submitted patch, 36: twig_2.x-make_necessary_changes-2568181-36.patch, failed testing.

The last submitted patch, 36: twig_2.x-make_necessary_changes-2568181-36.patch, failed testing.

Cottser’s picture

Now we have some similar hell it looks like, looking quickly I think it's with the format_date filter and not having the dateFormatter for unit testing.

So it looks like Twig 2.x is doing something different with extensions maybe.

Cottser’s picture

Was poking around the other day and found initRuntime() for Twig extensions where we can load things. Not sure if that makes it any more unit testable though, maybe it does if we call that from our unit test? I don't know :(

joelpittet’s picture

@Cottser moved the twig_without() move for better unit-testability to it's own child issue of this to help narrow this scope a bit.

#2591515: Move twig_without() to the TwigExtension where all the other filters are and deprecate

effulgentsia’s picture

I discussed this with the other committers, and yes to removing Drupal core usages of @deprecated Twig 1.x APIs being "rc target" material.

Can we do this in a few child issues though:

  1. One for the removal of Twig_NodeInterface usages: "rc target".
  2. One for fixing ThemeRegistryLoader to only throw an exception when told to: "rc target".
  3. One for all of the test code improvements: "rc eligible".

Per #2591515-5: Move twig_without() to the TwigExtension where all the other filters are and deprecate, let's not do the changes that require moving twig_without() during RC. I think we can wait until 8.1 for that, since a Twig 2.0 release doesn't appear to be imminent (correct me if I'm wrong).

Cottser’s picture

Okay cool we have #2194155: Replace deprecated Twig_NodeInterface with recommended Twig_Node now as one child, getting kicked out of sprint venue but will make sure other issues get created.

Cottser’s picture

Title: Make necessary changes to be compatible with Twig 2.x » Get ready to upgrade to Twig 2.x
Category: Task » Plan
Status: Needs work » Active
xjm’s picture

Title: Get ready to upgrade to Twig 2.x » [meta] Get ready to upgrade to Twig 2.x
joelpittet’s picture

FYI this is a bit of a pain at the moment. jcalderonzumba/mink-phantomjs-driver is explicitly depending on Twig 1.x. I had to remove it to actually test this.

joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev
jibran’s picture

Status: Active » Needs review
FileSize
14.06 KB
334.27 KB

Status: Needs review » Needs work

The last submitted patch, 51: meta_get_ready_to-2568181-51.patch, failed testing.

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

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now 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.

xjm’s picture

Issue tags: -rc target

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

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now 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.

joelpittet’s picture

Issue summary: View changes

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.

Cottser’s picture

Issue summary: View changes
Status: Needs work » Active

Saving commit credits and hiding patches, patches should be worked on in the child issues. Thanks!