Problem/Motivation

As found in #3032695: Manually test Drupal 8 with Twig 2 Twig 2.7 introduced deprecations for the old style Twig namespace classes and uses proper namespaces now.

Proposed resolution

We need to either silence these deprecations for now (they will only take effect in Twig 3) or start using the new namespaces. We need to consider how type-hinting for contrib code works in the later case.

As of #26, we're using the new namespaces. #40 shows that we already were using the new namespaces in quite a few places. Probably we were using the old namespaces specifically for historical reasons (older versions of Twig didn't have the newer namespaces, and #2600154: Update our Twig code to be ready for Twig 2.x already put did initial Twig 2.x compatibility work).

There is one deprecation error we cannot fix without incurring a performance regression — see #44 + #47 + #48. A follow-up has been created for that: #3091378: Investigate \Drupal\Core\Template\TwigEnvironment::getTemplateClass(): check if it still provides value, and if so, contribute it upstream.

Remaining tasks

Discuss. Patch. Commit.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

Drupal 9 has been updated to Twig 2. The changes for PHP developers and template creators are listed at https://www.drupal.org/docs/9/how-to-prepare-your-drupal-7-or-8-site-for...

CommentFileSizeAuthor
#52 3041076-52.patch75.58 KBWim Leers
#52 interdiff.txt1.42 KBWim Leers
#50 3041076-2-50.patch75.58 KBalexpott
#46 3041076-46.patch74.92 KBalexpott
#44 3041076-44.patch74.92 KBalexpott
#44 42-46-interdiff.txt1.96 KBalexpott
#42 reroll_diff_39-42.txt3.22 KBshaal
#42 3041076-42.patch76.31 KBshaal
#41 reroll_diff_39-41.txt638.28 KBshaal
#41 3041076-41.patch688.68 KBshaal
#39 3041076-39.patch76.32 KBalexpott
#39 37-39-interdiff.txt745 bytesalexpott
#37 3041076-37.patch76.54 KBalexpott
#37 26-37-interdiff.txt7.86 KBalexpott
#26 3041076-26.patch69.18 KBalexpott
#26 16-26-interdiff.txt62.47 KBalexpott
#16 3041076-update-twig-namespace-use--drupal9--16.patch9.69 KBshaal
#15 3041076-update-twig-namespace-use-14.patch10.02 KBshaal
#14 3041076-8.patch5.05 KBGábor Hojtsy
#8 3041076-8.patch5.05 KBlauriii
#7 3041076-7.patch4.74 KBlauriii
#3 3032695-13.patch2.31 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy created an issue. See original summary.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Needs review » Needs work

The last submitted patch, 3: 3032695-13.patch, failed testing. View results

Gábor Hojtsy’s picture

Note that I am not planning to work on this just posted the above patch to demonstrate the failures. I don't know what would be a good backwards compatible way to resolve this.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.74 KB

I'm not sure if there would be a way for us to provide future compatibility to these changes so that we could start upgrading code prior to being able to rely on Twig 2. I started adding these deprecation notices to the list of silenced deprecation errors.

lauriii’s picture

Gábor Hojtsy’s picture

@laurii: Twig itself uses class_alias() to alias classes. We can start PHP "use"-ing the new namespace versions, the question is what would that do to contrib extensions? I don't know. Twig 1 hit a problem with class aliases earlier this year as well due to similar problems, but I don't understand that problem fully.

Status: Needs review » Needs work

The last submitted patch, 8: 3041076-8.patch, failed testing. View results

Gábor Hojtsy’s picture

Looks like these two repeating multiple times:

  • Using an "if" condition on "for" tag is deprecated since Twig 2.10.0, use a "filter" filter or an "if" condition inside the "for" body instead (if your condition depends on a variable updated inside the loop). in FieldUIIndentationTest::testIndentation from Drupal\Tests\field_ui\Functional
  • The spaceless tag is deprecated since Twig 2.7, use the spaceless filter instead. in FieldUIIndentationTest::testIndentation from Drupal\Tests\field_ui\Functional
alexpott’s picture

Yeah I'm not sure that there is another way. We have the some problem as contrib has with core deprecations we need the ability to ignore deprecations for things we haven't got to yet. At least we have the advantage of being able to use \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Gábor Hojtsy’s picture

Title: [Twig 3] Update Twig namespace use or silence namespace deprecations with Twig 2.7 onwards » [Twig 3] Update Twig namespace use or silence namespace deprecations with Twig 2.7 onwards in Drupal 9
Version: 8.9.x-dev » 9.0.x-dev
Status: Needs work » Needs review
FileSize
5.05 KB

Now onto Drupal 9 here. Uploading #8 again for dedicated test results :)

shaal’s picture

re-created patch #8 since it wouldn't apply

shaal’s picture

I had to reroll patch #15 in order to apply it for Drupal9

Berdir’s picture

Shouldn't we be able to update to twig 2 and stop using the deprecated aliases in D9?

Gábor Hojtsy’s picture

@Berdir I think that would be a release manager / framework manager discussion estimating the disruptions.

Gábor Hojtsy’s picture

Title: [Twig 3] Update Twig namespace use or silence namespace deprecations with Twig 2.7 onwards in Drupal 9 » Update Drupal 9 to Twig 2

We need to figure out what to do with the Twig 2 to 3 deprecations when we commit the update to Twig 2 as those will fail our tests. So I don't think it makes sense to do the update and this separately. Therefore closed #3032695: Manually test Drupal 8 with Twig 2 as duplicate. Adding credit to catch.

The last submitted patch, 15: 3041076-update-twig-namespace-use-14.patch, failed testing. View results

Status: Needs review » Needs work
Gábor Hojtsy’s picture

Most if not all of the fails are about the spaceless tag. A small sample:

3x: The spaceless tag in "core/themes/classy/templates/form/dropbutton-wrapper.html.twig" at line 13 is deprecated since Twig 2.7, use the spaceless filter instead.
1x in BlockContentTypeTest::testBlockContentTypeCreation from Drupal\Tests\block_content\Functional
1x in BlockContentTypeTest::testBlockContentTypeEditing from Drupal\Tests\block_content\Functional
1x in BlockContentTypeTest::testsBlockContentAddTypes from Drupal\Tests\block_content\Functional

2x: The spaceless tag in "core/themes/stable/templates/admin/block-content-add-list.html.twig" at line 15 is deprecated since Twig 2.7, use the spaceless filter instead.
1x in BlockContentTypeTest::testBlockContentTypeEditing from Drupal\Tests\block_content\Functional
1x in BlockContentTypeTest::testsBlockContentAddTypes from Drupal\Tests\block_content\Functional

1x: The spaceless tag in "core/themes/classy/templates/form/select.html.twig" at line 13 is deprecated since Twig 2.7, use the spaceless filter instead.
1x in BlockContentTypeTest::testsBlockContentAddTypes from Drupal\Tests\block_content\Functional

Gábor Hojtsy’s picture

Looks like we need to stop using these and replace with a filter or just live with the whitespace:

$ git grep spaceless
core/modules/block_content/templates/block-content-add-list.html.twig:{% spaceless %}
core/modules/block_content/templates/block-content-add-list.html.twig:{% endspaceless %}
core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig:{% spaceless %}
core/modules/ckeditor/templates/ckeditor-settings-toolbar.html.twig:{% endspaceless %}
core/modules/link/templates/link-formatter-link-separate.html.twig:{% spaceless %}
core/modules/link/templates/link-formatter-link-separate.html.twig:{% endspaceless %}
core/modules/system/templates/dropbutton-wrapper.html.twig:  {% spaceless %}
core/modules/system/templates/dropbutton-wrapper.html.twig:  {% endspaceless %}
core/modules/system/templates/select.html.twig:{% spaceless %}
core/modules/system/templates/select.html.twig:{% endspaceless %}
core/modules/toolbar/templates/toolbar.html.twig:        {% spaceless %}
core/modules/toolbar/templates/toolbar.html.twig:        {% endspaceless %}
core/themes/claro/templates/form/input.html.twig:{% spaceless %}
core/themes/claro/templates/form/input.html.twig:{% endspaceless %}
core/themes/claro/templates/pager.html.twig:        {% spaceless %}
core/themes/claro/templates/pager.html.twig:        {% endspaceless %}
core/themes/claro/templates/pager.html.twig:        {% spaceless %}
core/themes/claro/templates/pager.html.twig:        {% endspaceless %}
core/themes/claro/templates/pager.html.twig:        {% spaceless %}
core/themes/claro/templates/pager.html.twig:        {% endspaceless %}
core/themes/claro/templates/pager.html.twig:        {% spaceless %}
core/themes/claro/templates/pager.html.twig:        {% endspaceless %}
core/themes/claro/templates/pager.html.twig:        {% spaceless %}
core/themes/claro/templates/pager.html.twig:        {% endspaceless %}
core/themes/claro/templates/views/views-mini-pager.html.twig:        {% spaceless %}
core/themes/claro/templates/views/views-mini-pager.html.twig:        {% endspaceless %}
core/themes/claro/templates/views/views-mini-pager.html.twig:        {% spaceless %}
core/themes/claro/templates/views/views-mini-pager.html.twig:        {% endspaceless %}
core/themes/classy/templates/field/link-formatter-link-separate.html.twig:{% spaceless %}
core/themes/classy/templates/field/link-formatter-link-separate.html.twig:{% endspaceless %}
core/themes/classy/templates/form/dropbutton-wrapper.html.twig:  {% spaceless %}
core/themes/classy/templates/form/dropbutton-wrapper.html.twig:  {% endspaceless %}
core/themes/classy/templates/form/select.html.twig:{% spaceless %}
core/themes/classy/templates/form/select.html.twig:{% endspaceless %}
core/themes/classy/templates/navigation/toolbar.html.twig:        {% spaceless %}
core/themes/classy/templates/navigation/toolbar.html.twig:        {% endspaceless %}
core/themes/stable/templates/admin/block-content-add-list.html.twig:{% spaceless %}
core/themes/stable/templates/admin/block-content-add-list.html.twig:{% endspaceless %}
core/themes/stable/templates/admin/ckeditor-settings-toolbar.html.twig:{% spaceless %}
core/themes/stable/templates/admin/ckeditor-settings-toolbar.html.twig:{% endspaceless %}
core/themes/stable/templates/field/link-formatter-link-separate.html.twig:{% spaceless %}
core/themes/stable/templates/field/link-formatter-link-separate.html.twig:{% endspaceless %}
core/themes/stable/templates/form/dropbutton-wrapper.html.twig:  {% spaceless %}
core/themes/stable/templates/form/dropbutton-wrapper.html.twig:  {% endspaceless %}
core/themes/stable/templates/form/select.html.twig:{% spaceless %}
core/themes/stable/templates/form/select.html.twig:{% endspaceless %}
core/themes/stable/templates/navigation/toolbar.html.twig:        {% spaceless %}
core/themes/stable/templates/navigation/toolbar.html.twig:        {% endspaceless %}

I don't have capacity to solve this now, help welcome :)

Gábor Hojtsy’s picture

alexpott’s picture

Status: Needs work » Needs review
FileSize
62.47 KB
69.18 KB

Looking at https://twig.symfony.com/doc/2.x/filters/spaceless.html we need to do a find and replace here...

Patch attached also replaces all of our usages of Twig_ classes. We need to do this because we have lots of instanceof checks and in fact I think most of the .php file changes should be considered for backport to 8.x since they are relevant for modules and themes who want to be truly D9 and D8 compat. The current version of Twig in D8 has all the new classes it's only that the only ones are not deprecated.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
@@ -108,13 +120,14 @@ protected function compileString(\Twig_Node $body) {
-        if (get_class($node) === 'Twig_Node' && $node->getNode(0) instanceof \Twig_Node_SetTemp) {
-          $node = $node->getNode(1);
-        }
+        // @todo Twig_Node_SetTemp was @internal in Twig 1 :( missing in Twig 2.
+        // if ($node instanceof Node && $node->getNode(0) instanceof \Twig_Node_SetTemp) {
+        //    $node = $node->getNode(1);
+        // }

So this is the only really odd thing - there is no equivalent of Twig_Node_SetTemp in Twig2. \Twig\Node\SetTemp exists in Twig1 but I guess as it was @internal they've done away with it.

Chi’s picture

I wonder if we still need to maintain trans Twig tag.
Theoretically it could be replaced by apply tag plus t filter.
https://twig.symfony.com/doc/2.x/tags/apply.html

Gábor Hojtsy credited xim.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Priority: Normal » Critical

Critical as per carried over status from #2568181: [META] Update to Twig 2.x in Drupal 9. I carried over credits above.

alexpott’s picture

@Chi well we certainly need to maintain it for D9.

Here's are some more deprecation fixes.

The last submitted patch, 26: 3041076-26.patch, failed testing. View results

alexpott’s picture

+++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
@@ -108,13 +120,14 @@ protected function compileString(\Twig_Node $body) {
-        if (get_class($node) === 'Twig_Node' && $node->getNode(0) instanceof \Twig_Node_SetTemp) {
-          $node = $node->getNode(1);
-        }
+        // @todo Twig_Node_SetTemp was @internal in Twig 1 :( missing in Twig 2.
+        // if ($node instanceof Node && $node->getNode(0) instanceof \Twig_Node_SetTemp) {
+        //    $node = $node->getNode(1);
+        // }

So \Twig_Node_SetTemp objects are created in \Twig\NodeVisitor\OptimizerNodeVisitor::doLeaveNode() in Twig 1... the code has no equivalent in Twig 2 so we can remove this.

Gábor Hojtsy’s picture

Wow this shows how much of a mix we used before in-between the old and new class naming schemes:

diff --git a/core/lib/Drupal/Core/Template/Loader/StringLoader.php b/core/lib/Drupal/Core/Template/Loader/StringLoader.php
 use Twig\Loader\ExistsLoaderInterface;
+use Twig\Loader\LoaderInterface;
 use Twig\Loader\SourceContextLoaderInterface;
 use Twig\Source;
[...]
-class StringLoader implements \Twig_LoaderInterface, ExistsLoaderInterface, SourceContextLoaderInterface {
+class StringLoader implements LoaderInterface, ExistsLoaderInterface, SourceContextLoaderInterface {

Patch looks good on a quick scan but I did not verify each change.

shaal’s picture

I rerolled patch #39, and added reroll-diff file

Edit: please ignore this one.

shaal’s picture

FileSize
76.31 KB
3.22 KB

oops. not sure what happened on #41 (please ignore it...)

Here is the real reroll for #39 (and a diff_reroll)

Status: Needs review » Needs work

The last submitted patch, 42: 3041076-42.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.96 KB
74.92 KB

Fixing the deprecations. There's not much we can do about the The "Twig\Environment::getTemplateClass()" method is considered internal. It may change without further notice. You should not extend it from "Drupal\Core\Template\TwigEnvironment". over than remove \Drupal\Core\Template\TwigEnvironment::getTemplateClass() which exists for performance reasons. I guess we could try to get the perf improvement upstream.

Gábor Hojtsy’s picture

Noting for posterity that there was some discussion that Drupal 8 would need changes for better preparation to Twig 2 compatibility as well. However in #3089301: Prepare Drupal 8 for Twig2 @alexpott found that nothing needs to change in core for that in Drupal 8. That should unblock this getting in :)

alexpott’s picture

Rerolled on top of HEAD

star-szr’s picture

Hi folks! Regarding getTemplateClass please see this old upstream issue: https://github.com/twigphp/Twig/pull/1071

alexpott’s picture

/me waves at @Cottser - thanks!
So upstream tried this and it broke stuff - that's useful info. I don't know how we're going to solve the The "Twig\Environment::getTemplateClass()" method is considered internal. It may change without further notice. You should not extend it from "Drupal\Core\Template\TwigEnvironment". deprecation. But I'm not sure that that matters for getting Twig2 done. Maybe I'll try a PR to remove the @internal or add a flag to enable this caching.

catch’s picture

fwiw I think this can go ahead suppressing that deprecation then we can spin-off an issue to try to address it more permanently.

alexpott’s picture

Rerolled on top of recent composer changes.

Wim Leers’s picture

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.42 KB
75.58 KB
  1. 👍 Verified that the namespace conversion is 100% done by grepping for Twig_ in core/ and finding lots of hits in HEAD, and zero with the patch applied.
  2. +++ b/core/lib/Drupal/Core/Template/TwigEnvironment.php
    @@ -6,16 +6,17 @@
    - *
    - * @see core\vendor\twig\twig\lib\Twig\Environment.php
      */
    -class TwigEnvironment extends \Twig_Environment {
    +class TwigEnvironment extends Environment {
    

    👍 At first I was confused that we're deleting this @see. But … it makes sense! The @see was necessary before to address the confusing aspect of the classname not matching the filename. In Twig 2 that's no longer an issue.

  3. +++ b/core/lib/Drupal/Core/Template/TwigNodeTrans.php
    @@ -108,13 +120,9 @@ protected function compileString(\Twig_Node $body) {
    -        if (get_class($node) === 'Twig_Node' && $node->getNode(0) instanceof \Twig_Node_SetTemp) {
    -          $node = $node->getNode(1);
    -        }
    

    👍 This removal makes sense — see #27 and #39.

  4. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
    @@ -193,6 +193,9 @@ public static function getSkippedDeprecations() {
    +      // The following deprecation is listed for Twig2 compatibility when unit
    

    🤓 Übernit: Twig2Twig 2.

  5. +++ b/core/themes/classy/templates/dataset/forum-list.html.twig
    @@ -60,7 +60,7 @@
    -        {% for i in 1..forum.depth if forum.depth > 0 %}</div>{% endfor %}
    +          {% if forum.depth > 0 %}{% for i in 1..forum.depth %}</div>{% endfor %}{% endif %}
    

    🤓✅ Indentation change, but this is actually a fix!

  6. ✅I reviewed the entire patch. It's all trivial changes (renames) for the change in namespaces and trivial changes for the changed for loop syntax in Twig 2 versus Twig 1.

Because of point 1 and point 6 I feel comfortable RTBC'ing this despite not being a theme system maintainer.

Wim Leers’s picture

Issue summary: View changes
Issue tags: +Needs release note

I think we still need a release note for the for … if syntax change in Twig 2 versus 1.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -Needs release note
+++ b/core/modules/book/templates/book-export-html.html.twig
@@ -36,12 +36,12 @@
 
-  {% for i in 1..depth-1 if depth > 1 %}
+  {% if depth > 1 %}{% for i in 1..depth-1 %}
     <div>
-  {% endfor %}
+  {% endfor %}{% endif %}
   {{ contents }}
-  {% for i in 1..depth-1 if depth > 1 %}
+      {% if depth > 1 %}{% for i in 1..depth-1 %}
     </div>
-  {% endfor %}
+  {% endfor %}{% endif %}

Ok so these are the for if changes Wim mentioned. I went to look at our Twig 2 does and the root Twig 2 docs at https://twig.symfony.com/doc/2.x/tags/for.html and neither explained this being not possible anymore.

BUT it does seem like the if syntax should work within a for for filtering the item itself. However, we are not using it for that. We are using it to try to cancel the whole for loop which this was not designed for it seems.

Still is it required to fix it here due to changes not documented in the Twig docs? @alexpott you introduced this in #37 without further explanation. Looks like this needs to be part of https://www.drupal.org/docs/9/how-to-prepare-your-drupal-7-or-8-site-for... at least because it is not part of the Twig official docs. I don't think this should be in a release note, since the extent of change with this patch is all documented in this docs page. I wrote a brief release note snippet for this pointing to those docs.

Gábor Hojtsy’s picture

Ok so Twig 2 deprecated if conditions in for in https://github.com/twigphp/Twig/pull/2998 (thanks for the link @alexpott). We are way ahead of merely doing Twig 2 support here, this is prep for Twig 3. I don't think we should overwhelm our people with those yet unnecessary changes as people don't need to do this to update to Drupal 9, unless they want to prepare for Drupal 10.

  • Gábor Hojtsy committed 5a143e7 on 9.0.x
    Issue #3041076 by alexpott, shaal, Gábor Hojtsy, lauriii, Wim Leers,...
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

All right, committed! Thanks for the fantastic attention to detail on this folks! One step closer to Drupal 9 alpha1 :)

catch’s picture

Issue tags: +9.0.0 release notes

Status: Fixed » Closed (fixed)

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

Krzysztof Domański’s picture

I created and published CR Twig updated from 1.x to 2.x in Drupal 9.

Chi’s picture

That CR should probably be published after #3095099: Update Twig to 3.5.0 as these changes may break tests with lowest dependencies.

Krzysztof Domański’s picture

@Chi You're right. Changes from Twig 2.x may break BC. I changed CR but left it published. IMO CR should be published due to Twig 1.x deprecation. Feel free to edit this CR if you want. What do you think about it:

Preparation for use of Twig 3 are not yet required. Some features that were deprecated in Twig 2.x are removed in Twig 3.0. Be careful when using new changes in contrib modules. These changes may break tests with lowest dependencies. See Twig change log for more information.

Chi’s picture

@Krzysztof Domański it looks good. Thank you.

Pasqualle’s picture