Problem/Motivation

https://github.com/twigphp/Twig/releases/tag/v3.9.0

New features and some deprecations so I expect this to break tests: https://github.com/twigphp/Twig/blob/3.x/CHANGELOG

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Twig has been upgraded to v3.9.3. If you have custom code that extends Twig, you should read the v3.9.0 changelog as there are new deprecations in this version that may affect you.

CommentFileSizeAuthor
#26 3441331-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3441331

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

longwave created an issue. See original summary.

longwave opened merge request !7523

andypost’s picture

andypost
he/him
Primary language Russian
commented

The only failure https://github.com/twigphp/Twig/pull/4019 which may affect contrib as well

The ``Twig\NodeVisitor\AbstractNodeVisitor`` class is deprecated, implement the ``Twig\NodeVisitor\NodeVisitorInterface`` interface instead.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review

Swapped AbstractNodeVisitor for NodeVisitorInterface. Don't think we need to provide BC as the class names are hardcoded in TwigExtension and aren't swappable.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Maybe CR explaining this swap makes sense

andypost’s picture

andypost
he/him
Primary language Russian
commented

now 2 more
- https://github.com/twigphp/Twig/commit/e70777405dd3dd1496d46b626f0b8589e...
- https://github.com/twigphp/Twig/commit/14d38036477b93976b56b1ee7046e86a3...

andypost’s picture

andypost
he/him
Primary language Russian
commented

hope this should be enough

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work

Needs more work for yeild

longwave credited Spokje.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Thanks @Spokje over in #3441417: Bump twig/twig to 3.9.0 for finding https://github.com/twigphp/Twig/issues/4008 - we use display_end in TwigNodeVisitorCheckDeprecations which appears to be why it's stopped working.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Postponed

Postponed on the above, nothing we can do until Twig fixes this.

andypost’s picture

andypost
he/him
Primary language Russian
commented

commented our case https://github.com/twigphp/Twig/issues/4008#issuecomment-2061086856

spokje’s picture

spokje
Primary language Dutch
commented

To be fair, we can still:

- Bump twig/twig to 3.9.1 which fixes Exception: Warning: Undefined variable $blocks.
- Bump to that version in composer.json, since lower version won't pass with the yield changes.

Did that just now.

andypost’s picture

andypost
he/him
Primary language Russian
commented

@Spokje thank you, now all functional tests passed and only left is deprecation test which is blocked on twig

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

https://github.com/twigphp/Twig/pull/4028 fixes the issue for me, commented over in https://github.com/twigphp/Twig/issues/4008

spokje’s picture

spokje
Primary language Dutch
commented
Status: Postponed » Needs review

3.9.2 landed and made it all better.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Good work everyone.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Looked over the Twig release notes and compared them to the individual changes. All seem correct. Couple questions though:

What are the chances of Twig's update disrupting contrib/custom code edgecases? Some of the changes we had to make to the core tests are just replacing a deprecated implementation with a better one, but one appears to be hard-breaking (although I'm not sure exactly what the consequences of not doing it are from the Twig release notes). There are two decisions that would depend on this:

  1. Whether we add a brief release note with a link to Twig's update info about anything.
  2. What we do with the Twig version and constraint for 10.3.

Thanks!

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +10.3.0 release notes, +11.0.0 release notes

Ignoring the bug in Twig 3.9.0/3.9.1, as far as I can tell the changes are all backward-compatible.

If you were extending AbstractNodeVisitor you should swap to implementing NodeVisitorInterface directly instead.

If you were using echo or print in ::compile() you should swap to yield and add the #[YieldReady] attribute.

Contrib should not be affected by this because none of these are swappable parts; Twig should issue deprecations that they can fix in their own time. Therefore I think we should backport this to 10.3 so contrib can upgrade when they are ready.

I think we should add a release note to 10.3.0 and 11.0.0 linking to the Twig release note, added a snippet.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Needs work

Committed 973b6ec and pushed to 11.x. Thanks!

Let's get an MR for 10.3 rolling. I thought about cherry-picking and doing the composer work on commit but I think a test run is worth it.

  • alexpott committed 973b6ec1 on 11.x 
    Issue #3441331 by andypost, longwave, Spokje, xjm: Update to Twig 3.9

alexpott opened merge request !7581

alexpott closed merge request !7523

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

Decided to do what I would have done on commit in a branch and push it to the MR repo...

andypost’s picture

andypost
he/him
Primary language Russian
commented

MR needs to set back to 3.9.2 or needs followup for 11.x as 1h ago https://github.com/twigphp/Twig/releases/tag/v3.9.3

https://github.com/twigphp/Twig/blob/3.x/CHANGELOG

# 3.9.3 (2024-04-18)

* Add missing `twig_escape_filter_is_safe` deprecated function
* Fix yield usage with CaptureNode
* Add missing unwrap call when using a TemplateWrapper instance internally
* Ensure Lexer is initialized early on

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3441331-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue tags: +no-needs-review-bot

  • alexpott committed 71151b5e on 11.x 
    Issue #3441331 follow-up:wq: Update to Twig 3.9
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@andypost I just pushed a quick follow-up to get us to 3.9.3 as we've had a test run on 10.3.x and it did not cuase any issues. Gonna update the 10.3 MR to have the same minimum.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Reviewed & tested by the community

I hope it will be green

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 5a43c22 and pushed to 10.3.x. Thanks!

  • alexpott committed 5a43c22f on 10.3.x 
    Issue #3441331 by andypost, longwave, alexpott, Spokje, xjm: Update to...

Status: Fixed » Closed (fixed)

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

quietone closed merge request !7581

chi’s picture

chi commented

Since twig/twig 3.9: Twig node "Drupal\Core\Template\TwigNodeTrans" is not marked as ready for using "yield" instead of "echo"; please make it ready and then flag it with the #[YieldReady] attribute.

Twig deprecations may break contrib tests. Can we backport this fix to Drupal 10.2?