Problem/Motivation

Twig 2.0.0 will be released in the near future.

Proposed resolution

Get ready for it, and eventually update as soon as we can.

Or we can upgrade in 8.1.0 or something.

Remaining tasks

  1. Wait until Twig 1.22.2 is released: https://github.com/twigphp/Twig/releases
  2. Remove our loadTemplate() override: #2571817: Remove race condition handling in TwigEnvironment::loadTemplate(), it's fixed upstream now
  3. Merge the 2.x compatibility issue (1.x patch): #2568181: [meta] Get ready to upgrade to Twig 2.x
  4. …then we can update to 2.0.0 when it's released: #2572605: Update to Twig 2.0.0

User interface changes

n/a

API changes

TBD

Data model changes

None

Files: 
CommentFileSizeAuthor
#35 2555243-35.patch8.06 KBdawehner
#18 interdiff.txt3.66 KBCottser
#18 upgrade_path_plan_to-2555243-18.patch354.14 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#17 interdiff.txt1.32 KBCottser
#17 upgrade_path_plan_to-2555243-17.patch351.79 KBCottser
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#14 interdiff.txt1.5 KBdawehner
#14 2555243-14.patch350.47 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#11 interdiff.txt5.29 KBdawehner
#11 2555243-11.patch363.53 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
#9 2555243-9.patch362.16 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
#9 interdiff.txt3.92 KBdawehner

Comments

Rene Bakx created an issue. See original summary.

attiks’s picture

I think we should upgrade, but if there's a reason not to we should at least make sure that all core twig will work with twig2 so site owners can at least upgrade.

attiks’s picture

rachel_norfolk’s picture

First question that springs to mind is "do the beta rules allow this?" But maybe this is a bigger thing than simply following the rules.

Do you have an indication as to how many hours work would be involved in the change?

dawehner’s picture

Well, I honestly would start trying it out. This allows us to properly judge whether this would cause any regression.

Rene Bakx’s picture

Rachel, if what Fabien wrote in that news item is really the case, I do think this is doable in a few hour with testing. It only applies to the actual engine itself. All the energy that went into moving the PHPTemplate / Theme layer to Twig is conserved. The actual syntax of Twig hasn't changed at all, so my best guess is, that on that level there will be no impact. The only impact is in the Code base of the core itself. To me it feels the same as upgrading the Guzzle system for example.

Cottser’s picture

Issue tags: +Twig

Thanks for this @Rene Bakx!

I think at this point the real question is: are we forwards compatible? It looks like we're close, but not quite.

As for whether we actually upgrade to Twig 2.x before release, I'm not sure we need to unless there is a big benefit (security, performance, or other). I believe the Symfony 3.x upgrade is planned for a minor release (8.1.0 or 8.2.0 for example), so in theory we could do that here as well. But if the impact is close to zero here then it might be worth trying to get it in.

But to reiterate: as long as we are forwards compatible upgrading the actual library can happen now, or happen later. Based on Fabien's blog post it looks like we need to at least change those three lines in core/lib/Drupal/Core/Template/TwigNodeVisitor.php so we're compatible :)

Cottser’s picture

Title: Upgrade path / plan to Twig 2.x » Upgrade path / plan to Twig 2.x aka 2.0
dawehner’s picture

Status: Active » Needs review
FileSize
3.92 KB
362.16 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View

Let's try it out.

Status: Needs review » Needs work

The last submitted patch, 9: 2555243-9.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
363.53 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
5.29 KB

Status: Needs review » Needs work

The last submitted patch, 11: 2555243-11.patch, failed testing.

catch’s picture

Priority: Normal » Major
Issue tags: +rc target
dawehner’s picture

Status: Needs work » Needs review
FileSize
350.47 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
1.5 KB

Just a reroll

Status: Needs review » Needs work

The last submitted patch, 14: 2555243-14.patch, failed testing.

The last submitted patch, 14: 2555243-14.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
FileSize
351.79 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
1.32 KB

Just moving it forward a little bit.

Cottser’s picture

FileSize
354.14 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
3.66 KB

Found a couple more changes (some toolbar and stuff renders now!) then I'm hands off for a while. Interdiff is from #14. At least on the default homepage this seems to be down to notices and warnings now.

The last submitted patch, 17: upgrade_path_plan_to-2555243-17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: upgrade_path_plan_to-2555243-18.patch, failed testing.

Cottser’s picture

I'm planning on working on this again in a couple hours but I'll probably throw some patches up on a testing issue as I make progress rather than continually posting here :)

The last submitted patch, 17: upgrade_path_plan_to-2555243-17.patch, failed testing.

The last submitted patch, 18: upgrade_path_plan_to-2555243-18.patch, failed testing.

Cottser’s picture

Making progress just a bit stuck right now on something relating to autoescape strategies. Unrelated to that, I opened an issue upstream to try to resolve what looks to me like a blocker - https://github.com/twigphp/Twig/issues/1811

Edit: See #2426563-40: Ignore: Patch testing issue for the latest patch. Note that I hacked the vendor lib there temporarily.

Cottser’s picture

Okay I think I sorted the autoescape thing, posted a new patch on the test issue.

Once we have a green patch here we should be able to spin up child issue(s) to get these changes in independently of the 2.0 update.

fabpot’s picture

I think the PR I've just made on Twig https://github.com/twigphp/Twig/pull/1812 should fix your issue about Twig 2.0 compatibility. Basically, it allows Drupal to override the filesystem cache strategy with a class instead of copying the code from Twig into Drupal (in Twig 1.x and 2.0).

I've just had a quick look at your code and I think it should work pretty well. It that works for you, I will merge the PR, release Twig 1.22, merge it into master (aka 2.0) ASAP. Does it sound like a plan?

Cottser’s picture

@fabpot that sounds great to me, thank you!

fabpot’s picture

The Twig PR has now been merged into the 1.x and master branch. Can you test it to see if everything goes fine for you before I release 1.22?

Cottser’s picture

@fabpot yes I'll try to test both 1.x and 2.x as soon as possible but may not get to it until later today, hope that's okay. I'm not sure if you're in a hurry to release it or not.

fabpot’s picture

No hurry, take your time :)

Cottser’s picture

Excellent, thanks.

Cottser’s picture

@fabpot after taking a closer look it may take some more time and testing from other folks to confirm implementing our own cache class without extending loadTemplate() will work. We are using a different storage method and we also have some code to try to handle race conditions which I'm not sure at this moment if all that can be encapsulated in a cache class. I will poke at it tonight to see what is possible.

We'll keep you posted on this issue with any updates.

fabpot’s picture

https://github.com/twigphp/Twig/pull/1822 should fix the remaining issues you have.

Cottser’s picture

I've just posted on #2429659-134: Race conditions in the twig template cache asking for help testing the race condition to see if it still needs special handling or not. #2426563-93: Ignore: Patch testing issue contains a cache class TwigPhpStorageCache that implements PhpStorage caching for Twig 1.22+. While we're waiting on that I'll start to create some more child issues here to split up the patch on the patch testing issue into logical pieces that get us 2.x compatible.

dawehner’s picture

FileSize
8.06 KB

Urgs, here is my version of the twig php storage cache.

fabpot’s picture

The last patch looks great!

I'm wondering if your implementation of getTemplateClass() should not be the default in Twig.
Not sure about renderInline(), have you had a look at createTemplate()?

fabpot’s picture

One more thing: the implementation of getTimestamp() seems wrong to me as it should return a timestamp, not a Boolean.

Cottser’s picture

@dawehner at a glance that looks similar to mine but probably better :). See #2426563-95: Ignore: Patch testing issue.

The getTimestamp() implementation seems right to me, it should always return a timestamp either from cache or the current request time but maybe I'm reading it wrong.

Cottser’s picture

Here's what I'm seeing as child/highly related issues:

  • Upgrade to Twig 1.22.0
  • New cache class + TwigEnvironment changes, can be committed once Twig 1.22.0 is out.
  • General 2.0 compatibility updates, probably no need to split those up further. Only changes a handful of files and not blocked on Twig 1.22.0.
dawehner’s picture

@dawehner at a glance that looks similar to mine but probably better :). See #2426563-95: Ignore: Patch testing issue.

Well, your one certainly actually works :)

The getTimestamp() implementation seems right to me, it should always return a timestamp either from cache or the current request time but maybe I'm reading it wrong.

Yeah that === FALSE is jus wrong, I wasn't sure what we should do here, there might a chance that we loose this particular cache entry. Not sure whether PHP storage actually allows you to ask for the underlying filename, which we could leverage for the timestamp, given that this is an implementation detail.

Cottser’s picture

Oops. Didn't realize 1.22.0 is now out and didn't see #2568085: Lock Twig on ~1.21.2 until we are able to upgrade to 1.22.0.

Here's a new issue to upgrade to Twig 1.22.0 and add our own cache class: #2568171: Upgrade to Twig 1.22 and implement our own cache class I'll post a patch there shortly based on my previous work and @dawehner feel free to update it to be more like yours :)

Cottser’s picture

Status: Needs work » Active

From #39 we need to combine those first two into one issue. So there are now two new child issues:

#2568171: Upgrade to Twig 1.22 and implement our own cache class
#2568181: [meta] Get ready to upgrade to Twig 2.x

Setting back to active to make it a plan/meta again.

Cottser’s picture

#2568171: Upgrade to Twig 1.22 and implement our own cache class is in now!

Edit: Moving plan to issue summary for updating and stuff.

Cottser’s picture

Assigned: Rene Bakx » Unassigned
Issue summary: View changes
fabpot’s picture

Twig 1.22.2 is out now.

catch’s picture

Status: Active » Closed (duplicate)
Cottser’s picture

Status: Closed (duplicate) » Active

It's a plan issue so I'd rather keep it open until we've finished the child issues personally.

Cottser’s picture

Status: Active » Closed (duplicate)

Never mind, making #2568181: [meta] Get ready to upgrade to Twig 2.x into a plan :)