Skip to content

Conversation

@acrobat
Copy link

@acrobat acrobat commented Nov 21, 2025

This PR fixes the Twig 3 deprecations reported in #307

$hookContext = $this->parseMultitargetExpression();
} else {
// Remove when Twig 3.21 support is dropped
$hookContext = $this->parser->getExpressionParser()->parseMultitargetExpression();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is deprecated and no alternative is provided. In twig the usage of this method was replaced by a private method, so I'm applying the same logic here

twigphp/Twig@42c82e1#diff-c4fd0102f3bc6095b6b2c644eed251b98a65e46d2ed80329d97e9da3e97b984f

@acrobat acrobat changed the title Fix Twig 3 deprecations [TwigHooks] Fix Twig 3 deprecations Nov 21, 2025
@acrobat acrobat force-pushed the fix-twig3-deprecations branch from 17b7308 to d72e186 Compare December 2, 2025 14:48
@acrobat
Copy link
Author

acrobat commented Dec 2, 2025

I've fixed the code style errors of the previous build, can someone approve the build to run again?

],
$lineno,
);
} else {
Copy link
Member

@loic425 loic425 Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer an early return style.
But maybe in the opposite way to remove the code easily after bumping the package.
That's only a suggestion, cause it's ok like this too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm also using the early return in regular code but mostly when doing deprecations like this I structure it in an if/else to make more clear what to remove when going to the next major or when dependencies are bumped

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also prefer reversing the conditions and early returning, in other places as well, but ultimately think it's fine as is

@loic425
Copy link
Member

loic425 commented Jan 27, 2026

@acrobat Thx a lot for this PR!
@NoResponseMate Please could review this one too please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants