-
Notifications
You must be signed in to change notification settings - Fork 29
[TwigHooks] Fix Twig 3 deprecations #311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| $hookContext = $this->parseMultitargetExpression(); | ||
| } else { | ||
| // Remove when Twig 3.21 support is dropped | ||
| $hookContext = $this->parser->getExpressionParser()->parseMultitargetExpression(); |
There was a problem hiding this comment.
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
17b7308 to
d72e186
Compare
|
I've fixed the code style errors of the previous build, can someone approve the build to run again? |
| ], | ||
| $lineno, | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
|
@acrobat Thx a lot for this PR! |
This PR fixes the Twig 3 deprecations reported in #307