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.

CommentFileSizeAuthor
#51 meta_get_ready_to-2568181-51.patch334.27 KBjibran
#51 meta_get_ready_to-2568181-51-core-do-not-test.patch14.06 KBjibran
#36 interdiff.txt664 bytesstar-szr
#36 twig_2.x-make_necessary_changes-2568181-36.patch389.54 KBstar-szr
#36 make_necessary_changes-2568181-36.patch16.82 KBstar-szr
#34 interdiff.txt532 bytesstar-szr
#34 twig_2.x-make_necessary_changes-2568181-34.patch389.18 KBstar-szr
#34 make_necessary_changes-2568181-34.patch16.46 KBstar-szr
#31 twig_2.x-make_necessary_changes-2568181-31.patch389.41 KBstar-szr
#31 make_necessary_changes-2568181-31.patch16.69 KBstar-szr
#28 interdiff.txt845 bytesstar-szr
#28 twig_2.x-make_necessary_changes-2568181-28.patch381.96 KBstar-szr
#28 make_necessary_changes-2568181-28.patch16.63 KBstar-szr
#25 interdiff.txt4.05 KBstar-szr
#25 twig_2.x-make_necessary_changes-2568181-25.patch381.44 KBstar-szr
#25 make_necessary_changes-2568181-25.patch16.11 KBstar-szr
#23 interdiff.txt2.37 KBstar-szr
#23 twig_2.x-make_necessary_changes-2568181-23.patch377.64 KBstar-szr
#23 make_necessary_changes-2568181-23.patch12.31 KBstar-szr
#11 interdiff-twig-2.x.txt1.17 KBstar-szr
#11 twig_2.x_make_necessary_changes-2568181-10.patch400.93 KBstar-szr
#9 interdiff-twig-2.x.txt8.8 KBstar-szr
#9 twig-2.x_make_necessary_changes-2568181-8.patch401.34 KBstar-szr
#4 interdiff.txt940 bytesstar-szr
#4 twig-2.x_make_necessary_changes-2568181-4.patch393.37 KBstar-szr
#4 make_necessary_changes-2568181-4.patch10.91 KBstar-szr
#3 twig-2.x_make_necessary_changes-2568181-3.patch393.36 KBstar-szr
#2 make_necessary_changes-2568181-2.patch10.91 KBstar-szr
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cottser created an issue. See original summary.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
Status: Active » Needs review
FileSize
make_necessary_changes-2568181-2.patch10.91 KB

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

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
Issue summary: View changes
FileSize
twig-2.x_make_necessary_changes-2568181-3.patch393.36 KB

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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
FileSize
make_necessary_changes-2568181-4.patch10.91 KB
twig-2.x_make_necessary_changes-2568181-4.patch393.37 KB
interdiff.txt940 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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
FileSize
twig-2.x_make_necessary_changes-2568181-8.patch401.34 KB
interdiff-twig-2.x.txt8.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

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Postponed

Yeah let's wait on that first ...

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
FileSize
twig_2.x_make_necessary_changes-2568181-10.patch400.93 KB
interdiff-twig-2.x.txt1.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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
Status: Needs work » Postponed
znerol’s picture

znerol CreditAttribution: znerol commented
Issue summary: View changes
star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Postponed » Needs work

Working on this a bit now that #2568171: Upgrade to Twig 1.22 and implement our own cache class is in.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs work » Needs review
Related issues: +#2571817: Remove race condition handling in TwigEnvironment::loadTemplate(), it's fixed upstream now
FileSize
make_necessary_changes-2568181-23.patch12.31 KB
twig_2.x-make_necessary_changes-2568181-23.patch377.64 KB
interdiff.txt2.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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs work » Needs review
FileSize
make_necessary_changes-2568181-25.patch16.11 KB
twig_2.x-make_necessary_changes-2568181-25.patch381.44 KB
interdiff.txt4.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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
Status: Needs work » Needs review
FileSize
make_necessary_changes-2568181-28.patch16.63 KB
twig_2.x-make_necessary_changes-2568181-28.patch381.96 KB
interdiff.txt845 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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
FileSize
make_necessary_changes-2568181-31.patch16.69 KB
twig_2.x-make_necessary_changes-2568181-31.patch389.41 KB

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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
Status: Needs work » Needs review
FileSize
make_necessary_changes-2568181-34.patch16.46 KB
twig_2.x-make_necessary_changes-2568181-34.patch389.18 KB
interdiff.txt532 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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented
Status: Needs work » Needs review
FileSize
make_necessary_changes-2568181-36.patch16.82 KB
twig_2.x-make_necessary_changes-2568181-36.patch389.54 KB
interdiff.txt664 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

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

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?

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented

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

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented

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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer and at Northern Commerce commented

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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented

@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

effulgentsia CreditAttribution: effulgentsia at Acquia commented

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

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented

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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr as a volunteer commented
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

Making this a plan. The patch here has been split between all the child issues:

#2194155: Replace deprecated Twig_NodeInterface with recommended Twig_Node
#2591515: Move twig_without() to the TwigExtension where all the other filters are and deprecate
#2600152: Make ThemeRegistryLoader only throw an error when $throw is true
#2600154: Update our Twig code to be ready for Twig 2.x

And I also made #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1 a child of this issue to maybe keep it simpler.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: Get ready to upgrade to Twig 2.x » [meta] Get ready to upgrade to Twig 2.x
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented

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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Version: 8.0.x-dev » 8.1.x-dev
jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Active » Needs review
FileSize
meta_get_ready_to-2568181-51-core-do-not-test.patch14.06 KB
meta_get_ready_to-2568181-51.patch334.27 KB

Patch after https://github.com/jcalderonzumba/MinkPhantomJSDriver/commit/782892dbea4...

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

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
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

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
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.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr at Northern Commerce commented
Issue summary: View changes
Status: Needs work » Active

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

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

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.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.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.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.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Issue tags: +Drupal 9
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Title: [meta] Get ready to upgrade to Twig 2.x » [META] Update to Twig 2.x in Drupal 9
Version: 8.7.x-dev » 9.x-dev
Parent issue: #2555243: Upgrade path / plan to Twig 2.x aka 2.0 » #3009213: [META] Update / reconsider PHP dependencies for Drupal 9

Hm, so looking at these issues, my understanding is #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1 is to do all that needs to be done to update Drupal 8 to support Twig 2 as well as the only remaining child issue at #2591515: Move twig_without() to the TwigExtension where all the other filters are and deprecate and this issue would be updating to Twig 2 itself?

Reparenting to an issue that is not a duplicate of this issue :D

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Last child commited #2591515: Move twig_without() to the TwigExtension where all the other filters are and deprecate
I think this meta could be closed in favor of #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

@andypost: this issue is about updating to Twig 2 in Drupal 9. #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1 is about supporting Twig 2 in Drupal 8. While the later is a requirement for this issue to happen, this issue has its own goal on top of #2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

#2572605: Make Drupal 8 optionally working with Twig 2, while keeping its dependency on Twig 1 landed, so this is unblocked in terms of making the actual change on 9.x once 9.x is open.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

There is a testing issue now at #3032695: Manually test Drupal 8 with Twig 2 that is used to test how it goes.

Version: 9.x-dev » 9.0.x-dev

The 9.0.x branch will open for development soon, and the placeholder 9.x branch should no longer be used. Only issues that require a new major version should be filed against 9.0.x (for example, removing deprecated code or updating dependency major versions). New developments and disruptive changes that are allowed in a minor version should be filed against 8.9.x, and significant new features will be moved to 9.1.x at committer discretion. For more information see the Allowed changes during the Drupal 8 and 9 release cycles and the Drupal 9.0.0 release plan.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Priority: Major » Critical

This is a hard blocker for releasing 9.0.0-alpha1, so promoting to critical.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

@Gábor Hojtsy closed the issue in #67 as a duplicate of #3041076: Update Drupal 9 to Twig 2.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

@Gábor Hojtsy closed the issue in #67 as a duplicate of #3041076: Update Drupal 9 to Twig 2.

We need a patch here now for Twig 2 in 9.0.x core!

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Priority: Critical » Major
Status: Active » Closed (duplicate)

Closing in favour of #3041076: Update Drupal 9 to Twig 2 since all other children are now done and that is where the patch is. Carried over credits! :)