Made Symfony 2.7 compatible with Twig 2.0 #15590
Conversation
fabpot
commented
Aug 23, 2015
fabpot
commented
| Q | A |
|---|---|
| Bug fix? | no |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | n/a |
| License | MIT |
| Doc PR | n/a |
| @@ -63,7 +63,7 @@ public function exists($name) | |||
| * | |||
| * @throws \Twig_Error_Loader if the template could not be found | |||
| */ | |||
| protected function findTemplate($template) | |||
| protected function findTemplate($template, $throw = true) | |||
| { | |||
| $logicalName = (string) $template; | |||
There was a problem hiding this comment.
We have a big problem here. In Twig 2.0, we don't convert template name to string anymore. But Symfony is made around this assumption. Failing tests are failing because they pass a TemplateReference instance directly to the Twig_Loader_Array class, which cannot deal with that anymore.
Moreover, if we were to refactor the Filesystem loader class to use composition, or just implement the interface and use the chain, that won't work as well for the same reason.
The only option I can see is to revert the non-string conversion in Twig 2.0 to keep BC.
There was a problem hiding this comment.
Would it not be enough to cast the template reference to string before calling the twig loader methods in this place: https://github.com/symfony/symfony/blob/2.3/src/Symfony/Bundle/TwigBundle/Controller/ExceptionController.php#L122
Ref. #12197
There was a problem hiding this comment.
Not really. If I do that, it breaks elsewhere. The problem is that Symfony uses objects and Twig do not support them anymore in 2.0.
There was a problem hiding this comment.
Where does it break? I only see two test failures which should be fixed by that.
There was a problem hiding this comment.
Indeed, it would fix the tests, but that breaks the general use case. The problem here is that the loader can be a Symfony one (expecting an object) and/or a Twig one (expecting a string).
There was a problem hiding this comment.
- if the loader is the symfony one, it will still work AFAIK if we pass a string because that is what the parser is for: https://github.com/symfony/symfony/blob/2.8/src/Symfony/Bundle/TwigBundle/Loader/FilesystemLoader.php#L83
- if it's a twig loader, it will not understand the symfony notation but still work as expected since it's a string
Or am I wrong?
There was a problem hiding this comment.
Indeed, that should work, but it's a bit slower as we need to parse the string to convert it again to an object.
This PR was merged into the 2.7 branch. Discussion ---------- Made Symfony 2.7 compatible with Twig 2.0 | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Commits ------- 7b64354 fixed Twig deprecations 99a1fcc replaced deprecated Twig sameas test by same as
