Problem/Motivation

Twig_NodeInterface is deprecated and will be removed in Twig 2.x but we still use it.

Why this should be an RC target

This is part of making our Twig code compatible with Twig 2.x which was deemed RC eligible in #2568181-45: [meta] Get ready to upgrade to Twig 2.x. There should be no disruption involved in making these changes.

Per @effulgentsia in #2568181-45: [meta] Get ready to upgrade to Twig 2.x:

I discussed this with the other committers, and yes to removing Drupal core usages of @deprecated Twig 1.x APIs being "rc target" material.

Proposed resolution

Fix it.

Remaining tasks

Patch
Review

User interface changes

n/a

API changes

n/a

Data model changes

n/a

Comments

cosmicdreams’s picture

FileSize
3.37 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
cosmicdreams’s picture

Status: Active » Needs review
Issue tags: +Twig

Status: Needs review » Needs work

The last submitted patch, 1: 2194155.patch, failed testing.

tatarbj’s picture

Assigned: Unassigned » tatarbj

I'm on it.

tatarbj’s picture

FileSize
49.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,927 pass(es). View

I've rescaned the whole core and looked throuth all of places of using of Twig_NodeInterface class. Because just the Twig_Node class extends this interface, i think in all of place when used the parent class we can use the implemented class, and the Twig_NodeIntercase will be deprecated we have to use the Twig_Node in all of our codes. This patch replaces all of Twig_NodeInterface to Twig_Node in codes and documentations.

tatarbj’s picture

Status: Needs work » Needs review
cosmicdreams’s picture

Status: Needs review » Needs work

It appears that the changes you made were overly aggressive. You shouldn't modify the imported libraries of Assetic or Twig. If those changes need to made they should be contributed as upstream patches.

cosmicdreams’s picture

Status: Needs work » Needs review
FileSize
3.2 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 37,878 pass(es), 17,271 fail(s), and 6,744 exception(s). View

Here's a patch that attempts to modify only the files within Drupal.

Status: Needs review » Needs work

The last submitted patch, 8: 2194155_8.patch, failed testing.

tatarbj’s picture

Status: Needs work » Needs review

I think if the Twig_NodeInterface class is a deprecated that means the concrate calls of this will be wrong if once will be deleted, so we have to modify the code not just in the drupal core but in assetics and the twig classes, otherwise our patches will be failed. So i think the comment #5 is a good solution for that problem.

cosmicdreams’s picture

Status: Needs review » Postponed

This likely means that we need to postpone this issue until we can resolve the upstream issues. As noted above we are not allowed to make changes to external libraries within the scope of Drupal development. If we did, those changes would be overwritten whenever we would update those external libraries anyway so there isn't a point in updating those external libraries anyway.

That would be like hacking core when you're making a module.

Cottser’s picture

Cottser’s picture

Status: Postponed » Needs review

8: 2194155_8.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: 2194155_8.patch, failed testing.

Cottser’s picture

Assigned: tatarbj » Unassigned
Issue tags: +Novice

Just want to note that these changes are included in #2568181: [meta] Get ready to upgrade to Twig 2.x, so this might end up being a duplicate.

It might be a good novice task to just create a new patch (create a new patch, don't try to reroll) that applies and see what the testbot thinks (see if any tests fail). Just to see if we could commit this separately potentially. But just a forewarning that it's very possible nothing will get committed here. Even still it could be good practice for rolling patches :)

Cottser’s picture

cosmicdreams’s picture

Cool to see this issue come back to life. Did a quick check on the state of core and it seems that only 3 files need to be changed, all in drupal/core/lib/Drupal/Core/Template/TwigNodeVisitor.php
drupal/core/lib/Drupal/Core/Template/TwigTransTokenParser.php
drupal/core/lib/Drupal/Core/Template/TwigNodeTrans.php

The only problem with the changes in drupal/core/lib/Drupal/Core/Template/TwigNodeVisitor.php which implements an interface that expects Twig_NodeInterace to type the variables used in enterNode() and leaveNode()

I've uploaded two patches one that will likely fail and one that will work to illustrate.

cosmicdreams’s picture

Status: Needs work » Needs review

The last submitted patch, 17: 2194155_17.patch, failed testing.

subhojit777’s picture

Version: 8.0.x-dev » 8.1.x-dev
subhojit777’s picture

Status: Needs review » Needs work

The only problem with the changes in drupal/core/lib/Drupal/Core/Template/TwigNodeVisitor.php which implements an interface that expects Twig_NodeInterace to type the variables used in enterNode() and leaveNode()

Then I guess it should be moved to needs work.

cosmicdreams’s picture

It appears that this is the proper solution for removing ALL of the Twig_NodeInterface references :
http://symfony.com/blog/twig-how-to-upgrade-to-2-0-deprecation-notices-t...

Cottser has a patch that does this work in #2568181: [meta] Get ready to upgrade to Twig 2.x, but it looks like they want to split that issue into easy to commit parts.

So let's do that work here.

subhojit777’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
3.82 KB
Cottser’s picture

Version: 8.1.x-dev » 8.0.x-dev
Status: Needs review » Needs work

This is almost certainly RC target per #2568181-45: [meta] Get ready to upgrade to Twig 2.x, back to 8.0.x.

+++ b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php
@@ -51,7 +51,8 @@ public function leaveNode(\Twig_NodeInterface $node, \Twig_Environment $env) {
-        // Store that we have a filter active already that knows how to deal with render arrays.
+        // Store that we have a filter active already that knows how to deal
+        // with render arrays.

Out of scope change, please remove. Thanks!

Cottser’s picture

Issue tags: +rc target triage
Cottser’s picture

Priority: Normal » Major
Cottser’s picture

  1. +++ b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php
    @@ -16,19 +16,19 @@
    -class TwigNodeVisitor implements \Twig_NodeVisitorInterface {
    +class TwigNodeVisitor implements \Twig_BaseNodeVisitor {
    

    Shouldn't this be 'extends' instead of 'implements'? \Twig_BaseNodeVisitor is an abstract class, not an interface.

  2. +++ b/core/lib/Drupal/Core/Template/TwigNodeVisitor.php
    @@ -16,19 +16,19 @@
    -  public function enterNode(\Twig_NodeInterface $node, \Twig_Environment $env) {
    +  public function doEnterNode(\Twig_Node $node, \Twig_Environment $env) {
    ...
    -  public function leaveNode(\Twig_NodeInterface $node, \Twig_Environment $env) {
    +  public function doLeaveNode(\Twig_Node $node, \Twig_Environment $env) {
    

    We should be changing these to protected methods per the interface.

xjm’s picture

Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. You can add a section <h3>Why this should be an RC target</h3> to the summary.

In this case, it can probably just quote @effulgentsia's comment on the other issue. :)

Cottser’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Thanks @xjm!

subhojit777’s picture

Status: Needs work » Needs review
FileSize
3.32 KB
948 bytes

This covers #24 and #27.1
@Cottser can you clarify #27.2? Thanks.

Cottser’s picture

Just summarizing some discussion from IRC here for reference:

The \Twig_BaseNodeVisitor base class makes enterNode and leaveNode from the interface that we were previously implementing final to make sure that the nodes passed in are instances of \Twig_Node and then calls doEnterNode and doLeaveNode respectively. So when we extend that base class we are implementing the do* methods and those are protected on the base class so it makes sense to keep them protected.

Hopefully that makes some sense, it's late.

subhojit777’s picture

Thanks @Cottser ;)

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @subhojit777, this looks good now! Updates all our usages and doesn't touch vendor.

The last submitted patch, 23: replace_deprecated-2194155-23.patch, failed testing.

The last submitted patch, 17: 2194155_17.patch, failed testing.

alexpott’s picture

Issue tags: -rc target triage +rc target

@catch and I agree we should be ready for Twig 2.0

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 65dd859 and pushed to 8.0.x. Thanks!

  • alexpott committed 65dd859 on 8.0.x
    Issue #2194155 by cosmicdreams, subhojit777, tatarbj, Cottser: Replace...

Status: Fixed » Closed (fixed)

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