allow Twig 4 #58145
allow Twig 4 #58145
Conversation
xabbuh
commented
Sep 2, 2024
| Q | A |
|---|---|
| Branch? | 7.2 |
| Bug fix? | no |
| New feature? | yes |
| Deprecations? | no |
| Issues | |
| License | MIT |
|
looks like not all of our code is ready for Twig 4, I'm looking into it |
|
accessing the key or value of an invalid iterator is a bug in Twig itself: twigphp/Twig#4260 |
|
Looks like we only have 2 issues in Symfony itself (not counting the Twig bug):
|
20f68c9 to
b51699f
Compare
| @@ -50,6 +51,10 @@ protected function getVariableGetterWithoutStrictCheck($name) | |||
|
|
|||
| protected function getVariableGetterWithStrictCheck($name) | |||
| { | |||
| if (class_exists(LoopIterator::class)) { | |||
| return \sprintf('(array_key_exists("%1$s", $context) ? $context["%1$s"] : throw new RuntimeError(\'Variable "%1$s" does not exist.\', 0, $this->source))', $name); | |||
There was a problem hiding this comment.
see twigphp/Twig#3933 and twigphp/Twig#3936
There was a problem hiding this comment.
I suggest adding a comment to make it clear that the code path below is for Twig 3.x and older (allowing to clean this method when dropping support for Twig 3)
| @@ -218,7 +218,7 @@ private function displayGeneralText(SymfonyStyle $io, ?string $filter = null): v | |||
| $types = ['functions', 'filters', 'tests', 'globals']; | |||
| foreach ($types as $index => $type) { | |||
| $items = []; | |||
| foreach ($this->twig->{'get'.ucfirst($type)}() as $name => $entity) { | |||
| foreach ((new \ReflectionMethod(Environment::class, 'get' . ucfirst($type)))->invoke($this->twig) as $name => $entity) { | |||
There was a problem hiding this comment.
Would it be possible to rely on the Twig public API instead ? Private methods of Twig are not covered by the BC promise.
There was a problem hiding this comment.
we possibly could, but that would require to completely rewrite the command, for functions, filters and tests we also depend on internal methods of the Environment class
There was a problem hiding this comment.
Maybe I can revert the change from public to private for getGlobals? We will still use internal methods, but at least that works without using reflection.
There was a problem hiding this comment.
that would be even better I guess
There was a problem hiding this comment.
I have reverted the change here
There was a problem hiding this comment.
FTR, getGlobals() is now also no longer internal since twigphp/Twig@194cced
| @@ -50,6 +51,10 @@ protected function getVariableGetterWithoutStrictCheck($name) | |||
|
|
|||
| protected function getVariableGetterWithStrictCheck($name) | |||
| { | |||
| if (class_exists(LoopIterator::class)) { | |||
| return \sprintf('(array_key_exists("%1$s", $context) ? $context["%1$s"] : throw new RuntimeError(\'Variable "%1$s" does not exist.\', 0, $this->source))', $name); | |||
There was a problem hiding this comment.
I suggest adding a comment to make it clear that the code path below is for Twig 3.x and older (allowing to clean this method when dropping support for Twig 3)
…ached (xabbuh) This PR was merged into the 4.x branch. Discussion ---------- don't read current key and value when end of iterator is reached see failures in symfony/symfony#58145 Commits ------- 12b4cdc don't read current key and value when end of iterator is reached
7f965f4 to
38258f5
Compare
|
Thank you @xabbuh. |
This PR was merged into the 7.2 branch. Discussion ---------- revert allowing Twig 4 | Q | A | ------------- | --- | Branch? | 7.2 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | reverts #58145 | License | MIT As Twig 4 will not be released before Symfony 7.2 we should not claim compatibility before a stable Twig 4 release. Commits ------- c9107ae revert allowing Twig 4