Problem/Motivation
Twig 2 was released in early 2017. While Drupal 8 should keep Twig 1 for backwards compatibility, we should optionally support running with Twig 2 in preparation to Drupal 9. #2600154: Update our Twig code to be ready for Twig 2.x made most of the required changes to support Twig 2 but not all of them.
Proposed resolution
Fix all remaining issues with Twig 2 compatibility. Prove with tests that it works.
Remaining tasks
Review. Commit.
User interface changes
N/A.
API changes
None.
Data model changes
None.
Release notes snippet
Drupal 8.7 core now supports optional use of Twig 2 in place of Twig 1. This does not by itself mean contributed modules and themes will work outright. See https://twig.symfony.com/doc/1.x/deprecated.html for a complete list of deprecated functionality from Twig 1, in case you need to make changes. Drupal core's use of Twig did not change APIs on Drupal's API surface.
Comment | File | Size | Author |
---|---|---|---|
#121 | 2572605-121-twig2.patch | 10.25 KB | andypost |
#121 | 2572605-121.patch | 7.79 KB | andypost |
#121 | interdiff.txt | 915 bytes | andypost |
Comments
Comment #2
joelpittetComment #4
joelpittetNote to self, this is postponed on the release of Twig 2.x
Comment #6
slasher13Comment #8
joelpittetLooks like the error was whitespace related give it another try @slasher13?
Comment #9
skyredwangRemoved the unnecessary trailing whitespace in the previous patch.
Comment #11
skyredwangLet's try again.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedall 'time' properties need update too :)
Comment #14
slasher13Is testbot incompatible with composer 1.3? Got these time property changes after update to composer 1.3
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedLooks like this patch based on #2840596: Update Symfony components to ~2.8.16, but it revert now.
Comment #16
slasher13rebased & composer self-update --rollback
Comment #18
slasher13Comment #19
slasher13What's wrong with composer.lock file? Twig should be updated on PHP7 environments.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #23
slasher13test again after https://www.drupal.org/node/2842443 and https://www.drupal.org/node/2840596.
Comment #25
slasher13Comment #27
joelpittetThanks 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
Comment #28
slasher13wrong patch
Comment #30
slasher13Comment #32
slasher13Comment #33
alexpott@slasher13 this change is not going to work :(
We can't update the composer.lock whilst still supporting PHP 5.5.9
Comment #34
slasher13But 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.
Comment #35
alexpott@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.Comment #36
joelpittet@alexpott couldn't we have the testbot build the composer per environment?
Comment #37
slasher13But 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.Comment #40
jibranCreated #2864031: Update twig/twig from v1.25.0 to v1.32.0 for minor version upgrade in 8.3.x.
Comment #41
star-szrComment #42
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #45
xjmSomeone 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.
Comment #46
joelpittetThanks @xjm, nice title change. We are hoping to have both work for BC.
Comment #47
jibranNeeds reroll after #2912542: Update twig/twig from v1.32.0 to v1.35.0.
Comment #48
joelpittetHere'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.Comment #50
jofitz CreditAttribution: jofitz at ComputerMinds commentedCorrected the test failures and rectified some coding standards errors.
Comment #53
jibranComment #54
aleevasI'm rerolled patch from #50
Please review
Comment #55
alexpott@aleevas your patch contains the patch - core/2572605-50.patch - can you generate the patch without that file? Thanks.
Comment #56
andypostComment #57
jofitz CreditAttribution: jofitz at ComputerMinds commentedRemove .patch from patch in #54.
Comment #58
joelpittetI'm
These look a bit sketch, I wonder if we can get away without having to generate a name like this.
Comment #59
jofitz CreditAttribution: jofitz at ComputerMinds commentedUse a hard-coded string for $name (in multiple locations).
Comment #60
mfernea CreditAttribution: mfernea at AmeXio commentedJust a little help with cs issues.
Comment #61
markhalliwellComment #62
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll.
Comment #64
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-roll without redundant file(!)
Comment #66
jofitz CreditAttribution: jofitz at ComputerMinds commentedAfter 2(!) empty patches, perhaps this re-roll will work.
Comment #67
alexpott@Jo Fitzgerald perhaps you can review #2600154: Update our Twig code to be ready for Twig 2.x which takes care of some of the test stuff and deprecations. It'll make doing this one much easier.
Comment #68
alexpottBody can't be NULL it's not optional.
These changes are unnecessary aren't needed.
This change looks unnecessary.
In some ways I think it'd be good to split the TwigNodeTrans into a separate issue and remove the related deprecations in \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations()
Comment #69
jibranIt is important to test the patch on both php5 and php7 to make sure tests are passing on twig 1 and 2.
Comment #70
alexpottFixing the deprecations should be the aim of #2600154: Update our Twig code to be ready for Twig 2.x. This issue should only be about the possible change to composer.json to support twig 2.0.
Comment #71
alexpottThis issue has never actually tested Twig 2.0 it has just permitted a project to install it. Before we change the constraint we need a patch on this issue that forces testbot to run a test with Twig 2. All the other work to stop us triggering deprecated code should take place in #2600154: Update our Twig code to be ready for Twig 2.x
Comment #72
alexpottPostponing on #2600154: Update our Twig code to be ready for Twig 2.x
Comment #73
alexpottComment #75
voleger#2600154: Update our Twig code to be ready for Twig 2.x is fixed
Comment #76
fgmSince we're no longer going to be supporting PHP 5.6 in 8.7, is there any reason at all to keep the Twig 1 support now that this issue targets 8.7 instead of 8.6 ?
Comment #77
effulgentsia CreditAttribution: effulgentsia at Acquia commentedTo not force BC breaks onto contrib modules/themes that use Twig 1.x features that were removed in 2.x.
Comment #78
andypostreroll after #2591515: Move twig_without() to the TwigExtension where all the other filters are and deprecate & #2600154: Update our Twig code to be ready for Twig 2.x
Comment #79
andypostCheck for upgrade
Comment #81
star-szrI'm rusty but let's try this.
Attaching patches for 2.x and 1.x (untested).
Interdiff code grabbed from our Twig string loader.
Comment #83
Gábor HojtsyAre the new problems found new in Twig 2.x since #2600154: Update our Twig code to be ready for Twig 2.x or are we finding them because we did not have these tests to prove it worked?
Either way it sounds like we'll need a testing issue at least ongoing to prove we keep compatibility since we cannot just include a test in core to keep it tested (patch composer files and then build a test environment) can we?
Comment #84
catchI think we should open an issue like #3020303: Consider using Symfony flex to allow switching between major Symfony versions to make an install with Twig 2 possible, this will then allow regular core tests against Twig 2 once we have min/max composer testing. Although an ongoing issue with a patch we can run tests against would be good until then.
Comment #85
andypostIr looks like most of them are new, not clear why testloader fails and why it was not updated early
Comment #86
Gábor HojtsyFYI #2976394: Allow Symfony 4.4 to be installed in Drupal 8 found deprecated API use for removal in Twig 3. I am surprised that did not come up here as fails since they are already marked deprecated without updating twig at all.
Comment #87
Berdir@Gabor: That is because those deprecation messages are again from the debug classloader that's new in Symfony 4 and not triggered by Twig itself. Also I guess those are more examples that we just need to ignore, as I assume that whatever's the replacement for that interface isn't available yet in Twig 1.x?
Comment #88
Gábor Hojtsy@berdir: yeah the concrete issue is #3030494: [Twig 3] The "Drupal\Core\Template\Loader\StringLoader" class implements "Twig_ExistsLoaderInterface" and "Twig_SourceContextLoaderInterface" that are deprecated since 1.12 and 1.27 (to be removed in 3.0). where I proposed to ignore them but that did not work :/ There are still genuine fails here that would be great to fix as soon as we can so we can say people can try out Drupal 8.7 with Twig 2 :)
Comment #89
larowlanTrying to fix the fails
Comment #90
Gábor HojtsyYay, thanks @larowlan.
Since you did not post an interdiff, I compared the patches and looks like the effective change you made is this:
Patch in #81:
Patch in #89:
On the other hand you added 50k worth of additional composer file changes, so not only updating twig anymore but also a bunch of other things, including phpunit and adding various new vendors. We should cut back the testing patch to only updating to twig 2 unless some dependency updates are required to do alongside it.
Comment #91
Gábor HojtsyOk I removed the composer changes from #89 and rolled a patch with just that. Also rolled in the composer changes from #81 that were actually limited to twig2 updates for testing. This should better represent that #89 is working (or not).
I merely spliced together patches, did not change anything :)
Comment #92
jibranHere we go. No interdiff cuz it only reverts composer.lock changes.
Comment #94
Gábor HojtsyHm, there is zero diff between @jibran's
twig-2572605-2.x-91.patch
and my2572605-89-with-composer-twig2-from-81.patch
and yet mine failed inDrupal\Tests\system\Functional\Module\InstallUninstallTest
which seemed suspiciously unrelated anyway. Random fail I assume. Ok now we need reviews so we can get this small fix in ASAP :)I opened the Twig 2 counterpart of #3020303: Consider using Symfony flex to allow switching between major Symfony versions at #3031639: [PP-1] Relax composer.json requirements to allow Drupal 8 to be installed with Twig 2 but marked it postponed on the Symfony 4 one as the mechanics and concerns may be the same and are still under discussion there.
Comment #95
Gábor HojtsyReuploading the patch to review for clarity. It prepares core to Twig 2 but makes no changes to which twig we ship with. That is the goal of the issue. The patches with composer changes were merely to test.
Comment #96
andypostany reason to add this commented?
Comment #97
alexpottThis variable should have a comment.
As we are changing the argument we should also fix the missing docs.
Comment #98
Gábor HojtsyRe #96/@andypost:
Removed debug code.
Re #97/@alexpott:
1. Yay!
2a. I don't think that variable is needed at all, it is inherited either way from Twig_Environment and it existed prior to this patch as well with the same value. Based on https://github.com/twigphp/Twig/blob/2.x/lib/Twig/Environment.php this property or its value did not change in Twig 2 either. Just removed it.
2b. While I was there I added docs to the single undocumented variable above it which is our own code. I copied the text verbatim from TwigPhpStorageCache
3. We are not changing the argument, Twig 1 was/is also passing $token as the same type. I noticed we copied the method names from the 'for' tag, and Twig otherwise uses different method naming based on the tag name, so interesting... Either way, not a good opportunity to change now.
4. Removed as per above.
Comment #99
alexpott@Gábor Hojtsy thanks for addressing the feedback. This looks good to go now.
Comment #101
Gábor HojtsyHere is a version with the twig2 composer changes only for testing. Also uploading #98 again for sake of committer clarity.
Also updating issue summary heavily.
Comment #102
Gábor HojtsyAdding Drupal 9 tag and further clarify the title.
Comment #104
Gábor HojtsyGlad we had the testing patch :) TwigEnvironment::$templateClassPrefix became private (from protected) between 1.x and 2.x
Comment #105
alexpottPatch attached addresses the fails in #101 by calling
parent::getTemplateClass()
so we no longer need to access the private.Whilst making this change I noticed that
\Drupal\Core\Template\TwigEnvironment::invalidate()
was trying to set\Twig_Environment::$loadedTemplates
to an empty array. This will no longer work because it is also private. PHP does not complain because it just creates a public property of the same name. However, setting this to an empty array has always been bogus because this property is filled by doing:As there is no way of unloading code in PHP once for the same value of $cls PHP will never return something different. So I think we should remove the
$this->loadedTemplates = [];
line.Comment #106
Wim LeersIsn't this a BC break?
Comment #107
alexpott@Wim Leers I think Twig is allowed to break its APIs between version 1 and 2.
Comment #108
larowlanThis change is in the 1.x patch, so perhaps needs further investigation?
Comment #109
Gábor HojtsyWell let's see what happens without that change. The following tests after where that change was still test the .class so that should work as well as before. The other test changes made only affect how the test template loading happens which should not affect this.
There is existing use of attributes.class in tests and some modules, neither of which needed to change so I am hopeful that this was not necessary to change in the test.
Comment #111
Gábor HojtsyOk this fails with the following error:
Directly this is because
Drupal\Core\Template\Attribute::hasClass()
expects a class name and returns true if that class is set on the attributes object, otherwise returns FALSE. But why is that called in the first place?From https://twig.symfony.com/doc/2.x/templates.html
Why is this a new problem with Twig 2? Because Twig 1 does not have the hasBar() part. See https://twig.symfony.com/doc/1.x/templates.html:
So a method we have on Drupal\Core\Template\Attribute is now in the middle of Twig's expected extension API.
Removing testing of this is certainly not the solution :D It would also not be a solution to make this method return TRUE when there was some class (and make the $class argument optional) because that would change the twig output from the empty output we expect (based on an expected null return value).
The sole reason we have this hasClass() method seems to be to test attributes. At least according to this grep:
So the question is how dangerous would it be to rename this method at this point on Drupal\Core\Template\Attribute or make some ugly workaround to make it work like an offsetGet('class') if no argument was given and add a todo to remove this ugly thing in D9 :)
Comment #112
Gábor HojtsyHere is the ugly workaround solution, let's discuss.
Comment #113
lauriiiI don't think changing this API to specifically support Twig 2 in this way is nice. I also don't think we should be deprecating this method since it was added for a reason and I don't think that has changed. I assume this problem will occur in other places as well, and we don't want to suggest this being an ideal solution for the problem. I suggest we try to solve this by an if clause that would allow us to specifically check if
->class
has been set.Comment #114
alexpottI think we can fix this in a more elegant way. I'm not sure that I agree with hassers being getters especially with respect to Twig - it was added in https://github.com/twigphp/Twig/pull/1969 - bascially to make Twig more like https://symfony.com/doc/current/components/property_access.html#using-ha... - but making a has* return anything other than a bool is really really odd so I don't think twig_get_attribute or \Twig_Template::getAttribute() should use them. Will open an issue against Twig later.
The more elegant fix it to implement getClass() to get the class attribute and call offsetGet() in it. That way it will take precedence over our hasClass() implementation which I don't think we should be changing - hasClass() is used a lot in contrib - see http://grep.xnddx.ru/search?text=-%3EhasClass%28&filename=
Comment #115
Gábor HojtsyDuh, I should have just added a getClass() method based on the twig lookup list, yeah :D
Comment #116
alexpottOpened https://github.com/twigphp/Twig/issues/2833 to address the hassers as getters.
Comment #117
jibranAs per #2887179-5: Fix the typehinting in CurrentPathStack
As this method doesn't exist on an interface an can be used directly I agree this is a BC break.
Comment #118
andypostI'd better used
\Twig_Environment::class
instead of string for better code navigationComment #119
Gábor Hojtsy@jibran: these are callbacks passed onto the Twig parser, I don't believe there is any meaningful result if called otherwise, they are merely public so the Twig parser can invoke them. They also already requires the argument to be an object instance with a test method on it that takes a string or array even if we don't add the typehint.
Comment #120
Gábor Hojtsyre @jibran, I looked through Twig 2.6.2's Parser class code and there only seems to be an expectation that the callable's class be an instanceof Twig_TokenParserInterface and even that only in an error condition. That said, Twig 2 does typehint its callables this way but there may not be a reason to force us to do so. (Again I did not write this patch, and I am contributing by poking at things :D).
So it does not hurt to try to not to add the typehint for safety sake. And see what happens then.
Comment #121
andypostFixed #118, IMO it's ready to go
Comment #122
jibranThis looks good now.
Comment #124
catchAgreed this looks really solid now, good to see all the iterations.
Committed 9ec3433 and pushed to 8.7.x. Thanks!
Comment #125
Gábor HojtsyNext step is to #3031639: [PP-1] Relax composer.json requirements to allow Drupal 8 to be installed with Twig 2 (which is dependent on some process discussions in another issue, see there) and #2976407: Use drupalci.yml for composer dependency min/max testing - DO NOT COMMIT to ensure there are no regressions in the future.
I opened #3032695: Manually test Drupal 8 with Twig 2 for occassional manual testing for now.
Comment #127
webchickSeems worthy of calling this out!