Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#65 | 3094493-65.patch | 18.81 KB | alexpott |
| |||
#65 | not-an-interdiff.txt | 2.21 KB | alexpott |
#60 | 3094493-60.patch | 18.39 KB | Gábor Hojtsy |
| |||
#59 | 3094493-59.patch | 18.39 KB | Gábor Hojtsy |
#55 | 3094493-55.patch | 18.39 KB | longwave |
Comments
Comment #2
Chi CreditAttribution: Chi commentedMink Phantom driver does not support Twig 3 yet.
https://github.com/jcalderonzumba/MinkPhantomJSDriver/blob/v0.3.3/compos...
Comment #3
shaal... aaand Twig 3.0 was officially released:
https://github.com/twigphp/Twig/releases/tag/v3.0.0
Comment #4
shaalComment #5
Chi CreditAttribution: Chi commentedThere are at least two records in the Twig 3 change log that present serious BC issues.
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.
Comment #6
Chi CreditAttribution: Chi commentedMy guess it was removed in favor of spaceless filter.
The rationale https://github.com/twigphp/Twig/pull/2998#issuecomment-490881818
Comment #7
Chi CreditAttribution: Chi commentedBoth
spaceless
andfor 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.
Comment #8
Chi CreditAttribution: Chi commentedIt 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?Comment #9
catchAdding 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.
Comment #10
alexpott@Chi as far as I can see both
{% spaceless %}
andfor 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.
Comment #11
alexpott@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
Comment #12
Krzysztof DomańskiSee 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.Comment #13
alexpottSo 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:
Comment #14
Chi CreditAttribution: Chi commented@Krzysztof Domański We could simply copy
spaceless
implementation to Drupal 9.The more tricky task is how to support deprecated
if
condition on afor
tag.Comment #15
Fabianx CreditAttribution: Fabianx at Tag1 Consulting commentedI 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.
Comment #16
Krzysztof DomańskiDocumentation for Twig:
Comment #17
fgmJust 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
Comment #18
alexpottIt'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
Comment #19
Chi CreditAttribution: Chi commented@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
Comment #20
catchI think we should re-title to this and bump to major.
Comment #21
alexpottThe 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.
Comment #22
catchSo 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.
Comment #23
Chi CreditAttribution: Chi commentedIn 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.
Comment #24
xjmIf 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!
Comment #25
Gábor HojtsyComment #27
Gábor HojtsyComment #28
Gábor Hojtsy@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
Comment #29
Gábor HojtsyI 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.Comment #30
longwaveEdit core/composer.json to allow Twig 3, then
composer update drupal/core twig/twig
worked for me.Comment #31
longwaveAt least some of the Twig unit and kernel tests pass with these changes, let's see what happens with the full set.
Comment #32
longwavenull > NULL
Comment #33
longwaveAlso fix HelpTopicTwigLoader
Comment #35
andypost@longwave Great job! only 10 failed tests!
So how to maintain 2 different versions same time as interfaces now using typed arguments?
It will be tricky to maintain both versions, will it work with twig2?
Comment #36
longwaveNope, doesn't work with Twig 2. If I roll back to Twig 2 but keep the rest of the changes:
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.
Comment #37
alexpottFor 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.
Comment #38
xjmI 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.
Comment #39
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDiscussed 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.
Comment #40
catchI 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).
Comment #42
catchThis 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.
Comment #43
andypostfor 9.3.x
Comment #44
naveen433 CreditAttribution: naveen433 at Valuebound for Valuebound commentedI am working on this
Comment #45
naveen433 CreditAttribution: naveen433 at Valuebound for Valuebound commentedrerolled
Comment #47
Gábor Hojtsy@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
Comment #48
catchTrying 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.
Comment #49
BerdirLooking 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.
Comment #50
longwaveAttempted to fix all the test fails. The biggest break is
loadTemplate()
is nowload()
, otherwise this is mostly newly added type declarations.Comment #51
longwaveLinting fix.
Comment #52
longwave#51 was pinned to v3.3.0 of Twig, we should just specify ^3.0.
Comment #53
andypostcould use separate issue as the module is not stable
Comment #54
Gábor HojtsyReparenting to #3251854: [META] Requirements for tagging Drupal 10.0.0-alpha1.
Comment #55
longwaveReroll for 10.0.x.
Comment #56
daffie CreditAttribution: daffie commentedFrom 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.
Comment #57
catchI'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.
Comment #58
catchNeeds a re-roll though.
Comment #59
Gábor HojtsyStraight 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.
Comment #60
Gábor HojtsyRan 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.Comment #62
Gábor HojtsyComment #64
catchBack to RTBC.
Comment #65
alexpottUpdating 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.
Comment #67
catchCommitted 837b829 and pushed to 10.0.x. Thanks!
Good to get this one in!
Comment #68
Taran2LProbably I'm late to the party, but:
why not
Comment #69
alexpottAdding note to issue summary about impact on installing Drush on Drupal 10...
Comment #70
Gábor HojtsyI came here to add the same note to the change record but @alexpott already did that too.
Comment #71
shaalWhen/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
Comment #72
andypost@Taran2L it's 10.0.x (just commited) so please file follow-up for #68 - it makes sense