Problem/Motivation

Twig 2.0 has been released.
Twig 2 requires PHP 7. Drupal 8 will support PHP 5.6 until at least early 2019 (Drupal 8.7 or higher). #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7

Proposed resolution

Update it.
Create a abstraction layer for twig 1 and 2.
Revert changes to composer.lock and update twig to ~1.3

Remaining tasks

  • Make patch to change version string in core/composer.json to ^1.23.1|^2
  • Review
  • Commit

User interface changes

n/a

API changes

Should be none.

Data model changes

n/a

CommentFileSizeAuthor
#59 2572605-59.patch19.69 KBJo Fitzgerald
#59 interdiff-57-59.txt2.55 KBJo Fitzgerald
#57 2572605-57.patch19.95 KBJo Fitzgerald
#54 2572605-54.patch63.32 KBaleevas
#50 2572605-50.patch23.13 KBJo Fitzgerald
#50 interdiff-48-50.txt4.17 KBJo Fitzgerald
#48 2572605-48.patch23.62 KBjoelpittet
#42 update_twig2-php7_only-2572605-42.patch23.76 KBJo Fitzgerald
#32 interdiff.txt2.29 KBslasher13
#32 update_twig2-php7_only-2572605-32.patch24.3 KBslasher13
#30 interdiff.txt859 bytesslasher13
#30 update_twig2-php7_only-2572605-30.patch24.21 KBslasher13
#28 update_twig2-php7_only-2572605-28.patch24.28 KBslasher13
#28 interdiff.txt7.47 KBslasher13
#25 interdiff.txt7.11 KBslasher13
#25 update_twig2-php7_only-2572605-25.patch31.75 KBslasher13
#23 update_twig2-php7_only-2572605-23.patch21.04 KBslasher13
#23 interdiff-18-23.txt470 bytesslasher13
#21 interdiff-18-21.txt2.11 KBvaplas
#21 2572605-21.patch22.59 KBvaplas
#18 interdiff.txt5.3 KBslasher13
#18 update_twig2-php7_only-2572605-18.patch21.06 KBslasher13
#16 update_twig2-php7_only-2572605-16.patch19.2 KBslasher13
#11 update_to_twig_2_0_0-2572605-11.patch38.49 KBskyredwang
#9 update_twig2-php7_only-2572605-6.patch38.49 KBskyredwang
#6 update_twig2-php7_only-2572605-5.patch38.53 KBslasher13
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Cottser created an issue. See original summary.

joelpittet’s picture

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

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.

joelpittet’s picture

Note to self, this is postponed on the release of Twig 2.x

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.

slasher13’s picture

Status: Needs review » Needs work

The last submitted patch, 6: update_twig2-php7_only-2572605-5.patch, failed testing.

joelpittet’s picture

Looks like the error was whitespace related give it another try @slasher13?

skyredwang’s picture

Removed the unnecessary trailing whitespace in the previous patch.

Status: Needs review » Needs work

The last submitted patch, 9: update_twig2-php7_only-2572605-6.patch, failed testing.

skyredwang’s picture

vaplas’s picture

all 'time' properties need update too :)

Status: Needs review » Needs work

The last submitted patch, 11: update_to_twig_2_0_0-2572605-11.patch, failed testing.

slasher13’s picture

Is testbot incompatible with composer 1.3? Got these time property changes after update to composer 1.3

vaplas’s picture

Looks like this patch based on #2840596: Update Symfony components to ~2.8.16, but it revert now.

slasher13’s picture

Status: Needs work » Needs review
FileSize
19.2 KB

rebased & composer self-update --rollback

Status: Needs review » Needs work

The last submitted patch, 16: update_twig2-php7_only-2572605-16.patch, failed testing.

slasher13’s picture

slasher13’s picture

+ /usr/local/bin/composer install --no-progress --no-suggest
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Nothing to install or update

What's wrong with composer.lock file? Twig should be updated on PHP7 environments.

Status: Needs review » Needs work

The last submitted patch, 18: update_twig2-php7_only-2572605-18.patch, failed testing.

vaplas’s picture

Status: Needs review » Needs work

The last submitted patch, 21: 2572605-21.patch, failed testing.

slasher13’s picture

Status: Needs review » Needs work

The last submitted patch, 23: update_twig2-php7_only-2572605-23.patch, failed testing.

slasher13’s picture

Status: Needs review » Needs work

The last submitted patch, 25: update_twig2-php7_only-2572605-25.patch, failed testing.

joelpittet’s picture

Thanks for tackling this @slasher13! It looks like there are some changes that are not needed in here. Can you tell my why the changes to core/lib/Drupal/Core/Field/BaseFieldDefinition.php were made?

PHP 7 tests seem to get closer to 0

slasher13’s picture

Status: Needs review » Needs work

The last submitted patch, 28: update_twig2-php7_only-2572605-28.patch, failed testing.

slasher13’s picture

Status: Needs review » Needs work

The last submitted patch, 30: update_twig2-php7_only-2572605-30.patch, failed testing.

slasher13’s picture

alexpott’s picture

@slasher13 this change is not going to work :(

We can't update the composer.lock whilst still supporting PHP 5.5.9

slasher13’s picture

But I don't know how to test PHP 7 only. After PHP 7 is green I will update to twig 1.31. So we are compatible with twig 2 and have no problems with composer.lock.

alexpott’s picture

@slasher13 the problem with changing the requirements to ^1.23.1|^2 is that we can't change the composer.lock only for PHP7 so we have no idea of whether all the bridge code works.

joelpittet’s picture

@alexpott couldn't we have the testbot build the composer per environment?

slasher13’s picture

But we fix it in composer.lock. If someone uses composer to handle dependencies. You have to verify it yourself or you have to wait until Drupal drops support of PHP 5.5. and PHP 5.6. So I think it's worth trying to be compatible with twig 2.

You can stay with ^1.23.1, too. Then I have to change it myself.

Status: Needs review » Needs work

The last submitted patch, 32: update_twig2-php7_only-2572605-32.patch, failed testing.

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.

jibran’s picture

Cottser’s picture

Issue tags: +Needs reroll
Jo Fitzgerald’s picture

Status: Needs review » Needs work

The last submitted patch, 42: update_twig2-php7_only-2572605-42.patch, failed testing.

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.

xjm’s picture

Title: Update to Twig 2.0.0 » Prepare Twig 2.0.0 update (while supporting Twig 1.x if possible)
Issue summary: View changes
Related issues: +#2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7

Someone understood this issue to be an expectation that 8.4.x or 8.5.x would actually require Twig 2 and PHP 7. Retitling the issue to be less scary and clarifying in the summary.

joelpittet’s picture

Thanks @xjm, nice title change. We are hoping to have both work for BC.

jibran’s picture

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
23.62 KB

Here's a re-roll of the last patch, I explicitly changed semver to ^2.4.0, hope that's ok?

Mostly applied, except for the composer.lock file and Drupal\twig_extension_test\TwigExtension\TestExtension which I fixed the conflict manually.

Status: Needs review » Needs work

The last submitted patch, 48: 2572605-48.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
4.17 KB
23.13 KB

Corrected the test failures and rectified some coding standards errors.

Status: Needs review » Needs work

The last submitted patch, 50: 2572605-50.patch, failed testing. View results

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.

jibran’s picture

Issue tags: +Needs reroll
aleevas’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
63.32 KB

I'm rerolled patch from #50
Please review

alexpott’s picture

@aleevas your patch contains the patch - core/2572605-50.patch - can you generate the patch without that file? Thanks.

andypost’s picture

Status: Needs review » Needs work
Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
19.95 KB

Remove .patch from patch in #54.

joelpittet’s picture

Status: Needs review » Needs work

I'm

+++ b/core/tests/Drupal/Tests/Core/Template/TwigExtensionTest.php
@@ -177,10 +193,11 @@ public function testActiveThemePath() {
-    $loader = new \Twig_Loader_String();
+    $name = sprintf('__string_template__%s', hash('sha256', uniqid(mt_rand(), TRUE), FALSE));
+    $loader = new \Twig_Loader_Array([$name => '{{ active_theme_path() }}']);

These look a bit sketch, I wonder if we can get away without having to generate a name like this.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
19.69 KB

Use a hard-coded string for $name (in multiple locations).