Follow-up to #2864031-28: Update twig/twig from v1.25.0 to v1.32.0

Problem/Motivation

Update twig/twig to v1.35.0 see https://github.com/twigphp/Twig/blob/v1.35.0/CHANGELOG for complete detail.

Proposed resolution

Update twig/twig from v1.32.0 to v1.35.0

Remaining tasks

Review.

User interface changes

None

API changes

See the change record.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

Title: Update twig/twig from v1.32.0 to v1.32.2 » Update twig/twig from v1.32.0 to v1.35.0
Issue tags: +Needs change record

There's BC break with safe markup

jibran’s picture

jibran’s picture

Status: Needs review » Needs work

The last submitted patch, 3: update_twig_twig_from-2912542-3.patch, failed testing. View results

jibran’s picture

Status: Needs work » Needs review
Haza’s picture

+++ b/core/tests/Drupal/KernelTests/Core/Theme/TwigMarkupInterfaceTest.php
@@ -57,7 +57,7 @@ public function providerTestMarkupInterfaceEmpty() {
-      'empty SafeMarkupTestMarkup' => ['<span></span>', SafeMarkupTestMarkup::create('')],
+      'empty SafeMarkupTestMarkup' => ['', SafeMarkupTestMarkup::create('')],

Just wondering why do we have an update in this test ?

Thanks

cburschka’s picture

Just guessing, but the CR mentions that Twig's length() and empty() now treat strings and string-able non-countables as if they were arrays of characters. Basically, they work on the string length if the value doesn't implement \Countable.

That means the <span></span> value is no longer empty, so it needs to be changed into an empty string to work correctly.

jibran’s picture

Issue tags: -Needs change record

Patch is green change notice is added so this is ready for reveiw . @Haza and @cburschka please see https://github.com/twigphp/Twig/pull/2420 for more details.

joelpittet’s picture

jibran’s picture

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

Done.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @jibran, the change record is for that change. Upstream they fixed a bug where empty objects would be considered not empty string values even if that is what they returned.

I think this would be great to have during the start of the 8.5.x cycle.

alexpott’s picture

Issue tags: +8.5.0 release notes

Here are all the changes:


* 1.35.0 (2017-09-27)

 * added Twig_Profiler_Profile::reset()
 * fixed use TokenParser to return an empty Node
 * added RuntimeExtensionInterface
 * added circular reference detection when loading templates

* 1.34.4 (2017-07-04)

 * added support for runtime loaders in IntegrationTestCase
 * fixed deprecation when using Twig_Profiler_Dumper_Html

* 1.34.3 (2017-06-07)

 * fixed namespaces introduction

* 1.34.2 (2017-06-05)

 * fixed namespaces introduction

* 1.34.1 (2017-06-05)

 * fixed namespaces introduction

* 1.34.0 (2017-06-05)

 * added support for PHPUnit 6 when testing extensions
 * fixed PHP 7.2 compatibility
 * fixed template name generation in Twig_Environment::createTemplate()
 * removed final tag on Twig_TokenParser_Include
 * added namespaced aliases for all (non-deprecated) classes and interfaces
 * dropped HHVM support
 * dropped PHP 5.2 support

* 1.33.2 (2017-04-20)

 * fixed edge case in the method cache for Twig attributes

* 1.33.1 (2017-04-18)

 * fixed the empty() test

* 1.33.0 (2017-03-22)

 * fixed a race condition handling when writing cache files
 * "length" filter now returns string length when applied to an object that does
   not implement \Countable but provides __toString()
 * "empty" test will now consider the return value of the __toString() method for
   objects implement __toString() but not \Countable
 * fixed JS escaping for unicode characters with higher code points

The fixed PHP 7.2 compatibility seems really important - but turns out that the related commits are only fixing tests. The most interesting thing here is the addition of the namespaced classes to make migration to Twig 2.x much simpler.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 693aed2 and pushed to 8.5.x. Thanks!

  • alexpott committed 693aed2 on 8.5.x
    Issue #2912542 by jibran: Update twig/twig from v1.32.0 to v1.35.0
    
jibran’s picture

Thanks, @alexpott I have published the change record as well.

hass’s picture

I thought D8 is compatible with HHVM? Now this support is removed. This also means system requirements need to be updated. The people who use HHVM will be unhappy I guess.

alexpott’s picture

HHVM support is not promised at all - see https://www.drupal.org/docs/8/system-requirements/php - also there is no mention of HHVM in core code at all. We have no idea if Drupal 8 passes tests on HHVM either.

alexpott’s picture

Also we really can't afford to stay too far behind with Twig - it is our templating layer if there is a security release for it we'd have to update right away.

hass’s picture

daffie’s picture

Status: Fixed » Needs work

If I install Drupal 8.4.0 with composer:

git clone --branch 8.4.0 https://git.drupal.org/project/drupal.git
composer install

I get Twig 1.35 but not the fix for TwigMarkupInterfaceTest. And as a result the test does not passes any more.

daffie’s picture

Version: 8.5.x-dev » 8.4.0

Selecting the right version 8.4.0.

jibran’s picture

Version: 8.4.0 » 8.5.x-dev
Status: Needs work » Fixed

This is not backported to 8.4.x. For 8.4.x twig is locked v1.32.0. Please check your settings or use https://github.com/webflo/drupal-core-strict to use 8.4.x version of twig.

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: -8.5.0 release notes

Following 8.4.0, the "release notes" tags should only be used for critical information site owners need when updating their sites.