Problem/Motivation
Twig 1.23.0 is out. Drupal 8 currently uses Twig 1.22. And they have now released Twig 1.23.1
Twig packages

Proposed resolution
Update it

Remaining tasks
Do it

User interface changes
None

API changes
None

Data model changes
None

Comments

TJacksonVA created an issue. See original summary.

TJacksonVA’s picture

Title: Update Twig to 1.23.0 » Update Twig to 1.23.1
Issue summary: View changes

And they have now released Twig 1.23.1.

Cottser’s picture

Issue tags: +Twig

Thanks! I've been tracking the changes more closely and I don't think I saw any red flags.

joelpittet’s picture

@tjacksonva feel free to post a patch with the updated composer and twig code.

TJacksonVA’s picture

@joelpittet, unfortunately, I'm currently on the road and cannot get to it at this point.

andypost’s picture

Status: Active » Needs review
Issue tags: +rc eligible
FileSize
102.89 KB

Here's a patch, should be critical according #2400407: [meta] Ensure vendor (PHP) libraries are on latest stable release

2 new interfaces

vendor/twig/twig/lib/Twig/Extension/GlobalsInterface.php
vendor/twig/twig/lib/Twig/Extension/InitRuntimeInterface.php

no security bugs in commits found

hussainweb’s picture

I see this difference in the patch when I try to create it myself.

diff --git a/vendor/composer/autoload_classmap.php b/vendor/composer/autoload_classmap.php
index fac9eab..6d782e0 100644
--- a/vendor/composer/autoload_classmap.php
+++ b/vendor/composer/autoload_classmap.php
@@ -437,6 +437,7 @@
     'SebastianBergmann\\Environment\\Runtime' => $vendorDir . '/sebastian/environment/src/Runtime.php',
     'SebastianBergmann\\Exporter\\Exporter' => $vendorDir . '/sebastian/exporter/src/Exporter.php',
     'SebastianBergmann\\GlobalState\\Blacklist' => $vendorDir . '/sebastian/global-state/src/Blacklist.php',
+    'SebastianBergmann\\GlobalState\\CodeExporter' => $vendorDir . '/sebastian/global-state/src/CodeExporter.php',
     'SebastianBergmann\\GlobalState\\Exception' => $vendorDir . '/sebastian/global-state/src/Exception.php',
     'SebastianBergmann\\GlobalState\\Restorer' => $vendorDir . '/sebastian/global-state/src/Restorer.php',
     'SebastianBergmann\\GlobalState\\RuntimeException' => $vendorDir . '/sebastian/global-state/src/RuntimeException.php',

This block is not there when I run composer update. I also made sure by checking for the file $vendorDir . '/sebastian/global-state/src/CodeExporter.php' and it was not present. I suspect this came in because of other updates?

I am attaching a patch without this change.

joelpittet’s picture

Status: Needs review » Needs work

Thanks both of you for tackling this. One thing that still needs to change

core/composer.json

-    "twig/twig": "^1.22.2",
+    "twig/twig": "^1.23.1",

I think that is all that would be needed further here then I'll RTBC.

hussainweb’s picture

@joelpittet: I am not sure if I am missing something. Why would we need that constraint? Drupal would work with 1.22.2 version, right? Is there a reason we can't work with 1.22.2?

joelpittet’s picture

Oh I wasn't seeing it as a constraint I saw it as the indicator of what version to install with composer install.

Maybe that's more an npm thing or I'm just mistaken.

We are preparing it for 2.x so it may be still worth updating the constraint.

joelpittet’s picture

Until release anyways it may good to keep things as fresh as possible.

andypost’s picture

@hussainweb strange, because the patch is what I get on clean 8.0.x after composer update twig/*

andypost’s picture

Status: Needs work » Needs review

@joelpittet until core shipped with libraries bundled we have to update them, at least until #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead

joelpittet’s picture

@andypost re #13 are you agreeing with me then re #8?

andypost’s picture

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Excellent:) I usually sound like I don't know what I'm talking about;)

hussainweb’s picture

@andypost: Re #13, as an application, Drupal will still get the version we are updating to because of the lock file. In any case, we are updating to the version in the code itself. Tightening the constraint only makes Drupal difficult to use in a composer workflow where another constraint might clash.

Now, I don't imagine this constraint could cause any problems as the earlier constraint was so close. Generally, however, the principle is that the composer should specify minimum (and if necessary, maximum) versions of the components with which Drupal can work. We had discussion related to this earlier. See #2400407-53: [meta] Ensure vendor (PHP) libraries are on latest stable release and following.

andypost’s picture

@hussainweb thanx for explanation, so what is you suggestion? keep the minimal working version or make it higher?

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 6caf112 on 8.0.x
    Issue #2609110 by andypost, hussainweb: Update Twig to 1.23.1
    
hussainweb’s picture

@andypost: It's already committed. :)

But just for the record, I'd prefer the minimum version with which Drupal works. It just gives maximum flexibility. In this case, it is not a problem because the difference between two constraints is really small. There are other cases where the version is not specified as it should be (we only need PHPUnit 4.6, but we require ~4.8, and there could be more).

We can let this be and we can really focus on the version constraints in a follow-up issue after we have #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead.

catch’s picture

Yes I should have noted, I don't think this matters much until #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead, but then it will matter a lot when we do that.

  • catch committed 6caf112 on 8.1.x
    Issue #2609110 by andypost, hussainweb: Update Twig to 1.23.1
    

Status: Fixed » Closed (fixed)

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