Problem/Motivation

Twig 3 is here
https://github.com/twigphp/Twig/releases/tag/v3.0.0

Proposed resolution

Update Drupal 10 to Twig 3.

Installing Drush on Drupal 10

Drush 10 has a dependency that requires Twig 1 or Twig 2 so is incompatible with Drupal 10. To install Drush at the current time do:

composer require drush/drush 11.x-dev

Remaining tasks

Discuss. Patch. Commit.

User interface changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Chi created an issue. See original summary.

Chi’s picture

shaal’s picture

... aaand Twig 3.0 was officially released:

https://github.com/twigphp/Twig/releases/tag/v3.0.0

shaal’s picture

Issue summary: View changes
Chi’s picture

There are at least two records in the Twig 3 change log that present serious BC issues.

removed the "spaceless" tag

removed the "if" condition support on the "for" tag

Those are widely used in core and contrib. The workaround could be implementing them ourselves and schedule for removal in Drupal 10. Though I could not find a rationale for their removal in Twig. They both seem quite useful.

Chi’s picture

removed the "spaceless" tag

My guess it was removed in favor of spaceless filter.

removed the "if" condition support on the "for" tag

The rationale https://github.com/twigphp/Twig/pull/2998#issuecomment-490881818

Chi’s picture

Both spaceless and for if were deprecated before currently used Twig version (2.12). So the templates should have been updated as part of #3041076: Update Drupal 9 to Twig 2. We probably missed it because of lack of test coverage.

Edit: never mind, all templates in 9.0.x branch were updated.

Chi’s picture

It seems there is no way to alter definitions of Token parsers in Twig. So that providing a BC layer for for if syntax would be tricky or even impossible. Can we introduce this BC break in 9.0.0?

catch’s picture

Adding all the tags and bumping to critical.

We should probably open a spin-off for for if to fix all the cases in core, and figure out what the missing test coverage was.

Possibly another issue to see if it's possible to provide a bridging layer or not for contrib.

Then as we're looking at those figure out how much of a disruption it will be if we just update without the bridging layer for contrib.

alexpott’s picture

@Chi as far as I can see both {% spaceless %} and for if are not used at all in Drupal 9 templates.

I think updating to Twig 3 makes things very hard for Drupal 8.9 / 9.0 compatibility. Can something be is something be Twig 1 and Twig 3 compatible? I suspect that this a lot of effort for little gain in the long run. And there is a compatibility layer between Twig 1 and Twig 3 - it is called Twig 2.

alexpott’s picture

@catch and I had the following discussion about this issue.

catch: Do we know how long twig 2 will be supported? iirc there is no EOL date.
alexpott: Every time we’ve asked Fabian it always seems like the answer is very undefined
catch: If we're not using for if in core, then it's possible it's also not used in contrib - and that whole bit might be a red herring.
alexpott: Well we are using both in D8
So we definitely are in contrib and cusomt
catch: hmm.
alexpott: Because people will have copied templates left right and center
Which is why imo we need to go through Twig2 - so people can get the deprecations
catch: Right but we were supposed to have made 8.x twig 2 compatible.
alexpott: It is.,
catch: But i guess that doesn't include new twig 2 deprecations.
alexpott: These things were deprecated in Twig 2 to be removed in Tiwg 3… yep
catch: So for me it comes down to is Fabien going to drop Twig 2 support before Symfony 3 or not.
And also can we allow Twig 3 in Drupal 9 in composer.json
alexpott: I agree we can allow - but we’ll get into that interesting area
Which for me is even more difficult for themes
Because running composer update locally will move you to Twig 3… but themes will be developed against core and testing on the lock version and not add a twig constraint in their composer josn
because themes with composer json?!?! why would they bother till now or even know this was a good thing
catch: Well we can start requiring that.
Same for allowing Symfony 5 / moving to Symfony flex and similar.
alexpott: I totally think this is the way we want to go but it’s a big change and ideally we’d be using composer for all code composition before we mandated that
that being getting your dependencies correct

Krzysztof Domański’s picture

Deprecated features are kept for backward compatibility and removed in the next major release (a feature that was deprecated in Twig 2.x is removed in Twig 3.0).

See deprecated features in Twig 2.x.

E.g. the spaceless tag is deprecated in Twig 2.7. There are many modules that use the spaceless tag.

http://grep.xnddx.ru/search?text=spaceless&filename=

Including bootstrap module - over 60,000 installations.
E.g. bootstrap/blob/8.x-3.x/templates/input/input.html.twig

We have no way of reporting removed deprecation like trigger_error. Am I wrong? IMO using twig 3 in Drupal 9.x version will break BC in many places without helpful information.

alexpott’s picture

Priority: Critical » Normal

So in discussion with catch he said that the criticality of this issue was dependent on Twig 2 EOL - so I reached out to @fabpot and asked. Here's the response:

@alexpott Twig 1 is not even EOLed yet :slightly_smiling_face:
I don't have plans to EOL Twig 1 or 2
All 3 versions are maintained in //
and with time, I will drop support for older PHP versions (like I've done with Twig 1), but always very conservative
one thing I could do for instance is dropping the C ext for Twig 1, as all supported PHP versions are obsolete now

Chi’s picture

@Krzysztof Domański We could simply copy spaceless implementation to Drupal 9.
The more tricky task is how to support deprecated if condition on a for tag.

Fabianx’s picture

I agree, but I would do a tag implementation that does:

- Turn spaceless into a apply filter block
- Trigger a deprecation notice when spaceless_deprecated function is used

That makes it super simple to add those warnings.

For if we might get away with a regexp and some preprocessing and just adding an if block directly after the for?

That one is indeed trickier.

Worst case we could turn with opt-out `{% for` into `{% deprecated_for` and interpret that tag accordingly.

Not pretty but would work.

Krzysztof Domański’s picture

Documentation for Twig:

As of Twig 1.21, Twig generates deprecation notices when a template uses deprecated features. See Displaying Deprecation Notices for more information.

fgm’s picture

Just noticed Drupal appears to be on the Twig 3 watchlist: in https://github.com/twigphp/Twig/blob/3.x/.travis.yml one finds this comment

        # Drupal does not support 3.x yet
        #- stage: integration tests
        #  php: 7.3
        #  script: ./drupal_test.sh

alexpott’s picture

It'd be interesting to try to update the Twig3 branch to use Drupal 9 - which should be Twig 3 compatible. We'll need to create a PR to change https://github.com/twigphp/Twig/blob/3.x/drupal_test.sh

Chi’s picture

@fgm that test was once added to Twig 1 after a couple of regressions that caused critical bugs to Drupal.
#3039408: Updating twig/twig to v1.38.0 or v1.38.1 causes fatal error
#3051269: Updating Drupal 8.6.x to twig 1.40.0 causes twig exceptions

catch’s picture

Title: Update Drupal 9 to Twig 3 » Allow Drupal 9 to be installed with Twig 3
Priority: Normal » Major

I think we should re-title to this and bump to major.

alexpott’s picture

The problem with opening the composer constraints to allow Twig3 installation is the same as the Symfony one but probably worse because we certainly will have themes relying on Twig2 deprecations and we definitely don't have a way for a theme to say I depend on Twig 2.

catch’s picture

The problem with opening the composer constraints to allow Twig3 installation is the same as the Symfony one but probably worse because we certainly will have themes relying on Twig2 deprecations and we definitely don't have a way for a theme to say I depend on Twig 2.

So I think that means:

1. Recommend drupal-core-strict (or an equivalent) for Drupal 9
2. composer.json for themes and they should specify Twig version compatibility in there.

Chi’s picture

In Composer world extensions must specify version of Symfony/Twig they are working with. However this is not adopted in Drupal. Modules and themes typically only specify Drupal core version assuming that it will bring all other dependencies they need. And that's a problem.
We might need to update documentation and coding standards to encourage developers properly configure their dependencies in composer.json file.

xjm’s picture

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

If possible, this would be a minor-only change. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. We definitely should at least ensure that our code is forward-compatible for Twig 3, even though actually supporting multiple major versions has been fraught.

Thanks!

Gábor Hojtsy’s picture

Issue tags: -Drupal 9 +Drupal 10

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

@alexpott: re #19, Twig's Drupal test was updated to test against Twig 3.x at https://github.com/twigphp/Twig/commit/62bd3cd7de0b4df8dcd08e04aac154746... in last June. However I don't know where are there test results. Would be great to see that it actually works with Twig 3 :D

Gábor Hojtsy’s picture

I attempted to do a twig update patch on my side, but don't know how to get over this one. If I use composer 1, I can update core/composer.json manually and then requiring twig 3.3 worked. But it makes other changes and downgrades the plugin API version. I also tried --no-cache to no avail.

$ composer -W require twig/twig:3.3.0
./composer.json has been updated
Running composer update twig/twig --with-all-dependencies
> Drupal\Composer\Composer::ensureComposerVersion
Loading composer repositories with package information
Updating dependencies
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - drupal/core is locked to version 9.2.x-dev and an update of this package was not requested.
    - drupal/core 9.2.x-dev requires twig/twig ^2.12.0 -> found twig/twig[v2.12.0, ..., 2.x-dev] but it conflicts with your root composer.json require (3.3.0).

Use the option --with-all-dependencies (-W) to allow upgrades, downgrades and removals for packages currently locked to specific versions.

Installation failed, reverting ./composer.json and ./composer.lock to their original content.
longwave’s picture

Status: Active » Needs review
FileSize
3.7 KB

Edit core/composer.json to allow Twig 3, then composer update drupal/core twig/twig worked for me.

longwave’s picture

At least some of the Twig unit and kernel tests pass with these changes, let's see what happens with the full set.

longwave’s picture

longwave’s picture

Also fix HelpTopicTwigLoader

Status: Needs review » Needs work

The last submitted patch, 33: 3094493-33.patch, failed testing. View results

andypost’s picture

@longwave Great job! only 10 failed tests!

So how to maintain 2 different versions same time as interfaces now using typed arguments?

+++ b/core/lib/Drupal/Core/Template/Loader/FilesystemLoader.php
@@ -50,7 +50,7 @@ public function __construct($paths, ModuleHandlerInterface $module_handler, Them
-  public function addPath($path, $namespace = self::MAIN_NAMESPACE) {
+  public function addPath(string $path, string $namespace = self::MAIN_NAMESPACE): void {

+++ b/core/lib/Drupal/Core/Template/Loader/StringLoader.php
@@ -40,21 +40,21 @@ public function exists($name) {
-  public function getCacheKey($name) {
+  public function getCacheKey(string $name): string {
...
-  public function isFresh($name, $time) {
+  public function isFresh(string $name, int $time): bool {
...
-  public function getSourceContext($name) {
+  public function getSourceContext(string $name): Source {

It will be tricky to maintain both versions, will it work with twig2?

longwave’s picture

Nope, doesn't work with Twig 2. If I roll back to Twig 2 but keep the rest of the changes:

PHP Fatal error:  Declaration of Drupal\Core\Template\Loader\StringLoader::getSourceContext(string $name): Twig\Source must be compatible with Twig\Loader\LoaderInterface::getSourceContext($name) in /var/www/html/drupal/core/lib/Drupal/Core/Template/Loader/StringLoader.php on line 26

We can perhaps add the return types only; type widening lets us declare parameters without a type even if the parent has a type, but for return types it's the other way round - we can only go narrower. But that still has BC concerns for existing users who might have subclassed our code.

We could provide Twig 3 versions of all our classes/services and swap them in somehow, but unsure how messy that will be.

alexpott’s picture

For me this is why major versions exist in semver. It's the time where we get to make breaking changes like this. Contrib / custom code that depends on these can't be cross compatible and will need to pick a version of Drupal to be compatible with. Forward compatibility layers for things like this are not cheap - we do it for PHPUnit where we are rewriting the underlying class but that is a single class and it's all about returning voids so the extra strictness is pretty moot.

xjm’s picture

I wonder if it might be worth having two MR branches on this issue, one that has BC changes for D9, and one (non-mergeable) that goes ahead and implements the D10-compatible BC breaks? We could try to backport as much forward-compatibility as possible early, and then make a decision about how to handle the continuous-upgrade-path-unfriendly changes like typehint additions to existing methods once we have a sense of how extensive the impacts will be and what the total scope is.

It might also be worth looking more closely at Twig's documentation for the upgrade and/or even ask them how they have handled that vs. their own continuous upgrade path.

effulgentsia’s picture

Discussed this with @catch, @alexpott, and @xjm. Based on their input, here's where I landed with it; they might not fully agree, so I leave it to them to comment with disagreements, if any.

  1. PHP allows subclasses to widen parameter type hints and narrow return type hints. That means we can't add either to D9 and retain full BC. Because if we add parameter type hints, then we break on Twig 2. And if we add return type hints, then we break modules that subclass any of our classes.
  2. We should add both to D10.
  3. For modules containing subclasses of the core classes, if they want to simultaneously work on D9 and D10, they'll need to add return type hints and not add param type hints. If the module wants to drop D9 support at some time, then at that point it can add the param type hints.
  4. Given the above, we should add code to D9 that throws deprecation notices if a subclass to one of these classes doesn't have the return type hint. This will allow those modules to know about this upcoming change in D10 and address it at any time during D9 that they want. Symfony's DebugClassLoader could potentially assist with throwing this deprecation.
  5. We should also provide a patch (either on this issue or another one) that backports the param and return type hint additions to D9. That way, people who want to start using Twig 3 on D9 can do so by adding that patch to their composer.json.
catch’s picture

And if we add return type hints, then we break modules that subclass any of our classes.

I checked for contrib subclasses of the Twig classes changed by this patch, and so far found two - https://www.drupal.org/project/twig_temp https://www.drupal.org/project/graphql_twig. Only one implemented a method affected by the patch, and that would have been broken, the other appears to only implement unaffected methods so ought to be OK. Both are doing things to core's twig implementation, and could probably have been implemented via decoration or copy and paste instead of a subclass, so the likelihood of affecting custom code seems quite low if these turn out to be the only contrib examples of subclassing. I didn't go through every twig class in this patch though, just the first 2 or 3.

Given that I think we could open issues against those modules, and make the change in a minor release, since we don't guarantee bc for any random service override and overrides of any random core class methods. However, if #39.4 means we can provide a continuous upgrade path to Twig 3 in Drupal 10 without doing this, then it seems more friendly to do that (or alternatively start there and make the change in a later minor, only once those modules have been updated so that we know we won't break them).

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

catch’s picture

Priority: Major » Critical

This should be critical. We don't know exactly when Twig 2 EOL will be, but whether it forces an update in Drupal 10 or 11, we know we have to update in one or the other.

andypost’s picture

Issue tags: +Needs reroll

for 9.3.x

naveen433’s picture

I am working on this

naveen433’s picture

Status: Needs work » Needs review
FileSize
9.76 KB

rerolled

Status: Needs review » Needs work

The last submitted patch, 45: 3094493-45.patch, failed testing. View results

Gábor Hojtsy’s picture

@fabpot indicated Twig 1 is EOL now, and encorages everyone to upgrade to Twig 3 whenever possible. I think this went somewhat under the radar, but if we expect Drupal 10 to be long lasting (ie. on Symfony 6), it would be great to update to Twig 3.

https://twitter.com/fabpot/status/1463863729469235208

catch’s picture

Trying to think through this a bit now that 10.x opening is imminent:

- we need a green patch that updates Drupal 10 to twig 3.

- we should see how much of the fixes from that patch can be backported to Drupal 9 (9.4.x at this point).

- ideally we can commit the above together from this issue.

- a CR documenting the changes that we were able to make in Drupal 9 (where it'll be relevant for contrib) would be good.

- any fixes that can't be backported to Drupal 9 are obvious gaps in being able to provide a continuous upgrade path, so we should open up individual issues to try to add forwards compatibility layers (and/or at least ensure there are deprecation notices on 9.x). Also try to assess how much impact on contrib they have. However, I don't think this step should block the Twig 3 update in Drupal 10, the absolute worst case is that themes relying on features where there's no bridge have to add a version requirement for their Drupal 10 compatible release. This isn't worse than going out of security support.

Berdir’s picture

Looking at the test fails, I think this is mostly not things that will affect actual twig templates/themes, it's more about twig API classes/services that we're extending and overriding. We might be able to just update those services to work only with Twig 3 and commit that only to Drupal 10, without having to introduce breaking changes except to modules that provide their own twig functions/loaders/whatever.

longwave’s picture

Version: 9.3.x-dev » 9.4.x-dev
Status: Needs work » Needs review
FileSize
18.62 KB

Attempted to fix all the test fails. The biggest break is loadTemplate() is now load(), otherwise this is mostly newly added type declarations.

longwave’s picture

Linting fix.

longwave’s picture

FileSize
18.4 KB
3.56 KB

#51 was pinned to v3.3.0 of Twig, we should just specify ^3.0.

andypost’s picture

+++ b/core/modules/help_topics/src/HelpTopicTwigLoader.php
@@ -69,7 +69,7 @@ protected function addExtension($path) {
-  public function getSourceContext($name) {
+  public function getSourceContext(string $name): Source {

+++ b/core/modules/help_topics/tests/src/Unit/HelpTopicTwigTest.php
@@ -94,48 +96,16 @@ protected function getTwigMock() {
-      ->willReturn(new FakeTemplateWrapper(self::PLUGIN_INFORMATION['body']));
+      ->willReturn(new TemplateWrapper($twig, $template));

could use separate issue as the module is not stable

longwave’s picture

Title: Allow Drupal 9 to be installed with Twig 3 » Upgrade to Twig 3
Version: 9.4.x-dev » 10.0.x-dev
FileSize
18.39 KB

Reroll for 10.0.x.

daffie’s picture

Status: Needs review » Needs work
Issue tags: -Needs reroll +Needs change record

a CR documenting the changes that we were able to make in Drupal 9 (where it'll be relevant for contrib) would be good.

From the comment #48 from @catch. Adding the tag "needs change record".

+1 for the patch being RTBC.

The tags "needs * manager review" can go for me as this is a 10.0 issue only.

catch’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs frontend framework manager review, -Needs release manager review, -Needs framework manager review, -Needs change record

I've added a change record. Quite rare I actually have to deal with twig templates so reviews/improvements of the change record welcome.

I don't think we need to cover the Twig internals stuff in the CR - in core this is only affecting advanced help module and our base twig implementation, someone running into similar issues would be better off referring to the Twig 3 docs. Just knowing that Twig 3 is coming up and might affect things is more important. If an issue does arise, we can add pointers based on that.

Given the changes here are very straightforward, removing various tags. In the end this issue stalling against Drupal 9 doesn't seem like the worst outcome - saved ourselves some work.

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Needs a re-roll though.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
18.39 KB

Straight reroll but I have no idea how to fix the core composer lock hash reference, so will need help with that :D Also happy to learn.

Gábor Hojtsy’s picture

Issue tags: -Needs reroll
FileSize
18.39 KB

Ran a composer update --lock on it as per @lleber's suggestion. Seems to have updated the lock hash only, although I have no way to tell if that was the expected hash, I expect it is.

Gábor Hojtsy’s picture

The last submitted patch, 59: 3094493-59.patch, failed testing. View results

catch’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

Updating from v3.3.4 to v3.3.7 and fixing conflicts in composer.lock - can't make an interdiff because of conflicts... attaching patch diff.

  • catch committed 837b829 on 10.0.x
    Issue #3094493 by longwave, Gábor Hojtsy, alexpott, naveen433, Chi,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 837b829 and pushed to 10.0.x. Thanks!

Good to get this one in!

Taran2L’s picture

Probably I'm late to the party, but:

  /**
   * Finds the path to the requested template.
   *
   * @param string $name
   *   The name of the template to load.
   * @param bool $throw
   *   Whether to throw an exception when an error occurs.
   *
   * @return string|null
   *   The path to the template, or NULL if the template is not found.
   *
   * @throws \Twig\Error\LoaderError
   *   Thrown if a template matching $name cannot be found.
   */
  protected function findTemplate(string $name, bool $throw = TRUE) {

why not

  protected function findTemplate(string $name, bool $throw = TRUE): ?string {
alexpott’s picture

Issue summary: View changes

Adding note to issue summary about impact on installing Drush on Drupal 10...

Gábor Hojtsy’s picture

I came here to add the same note to the change record but @alexpott already did that too.

shaal’s picture

When/How do we update D10's core-recommended to include the twig:^3 update?
https://github.com/drupal/core-recommended/blob/10.0.x/composer.json#L65

andypost’s picture

@Taran2L it's 10.0.x (just commited) so please file follow-up for #68 - it makes sense

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.