Skip to content

[TwigBundle] FilesystemLoader::findTemplate() breaks FilesystemLoader::exists() with Twig3 #34877

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.

Sign up for GitHub

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

Jump to bottom
Closed
dpesch opened this issue Dec 8, 2019 · 0 comments
Closed

[TwigBundle] FilesystemLoader::findTemplate() breaks FilesystemLoader::exists() with Twig3 #34877

dpesch opened this issue Dec 8, 2019 · 0 comments
Labels
Bug Status: Needs Review TwigBundle

Comments

@dpesch
Copy link
Contributor

dpesch commented Dec 8, 2019

Symfony version(s) affected: 4.4.1 (with Twig >= 3.0)

Description
Symfony TwigBridge (4.4.1) FilesystemLoader extends \Twig\Loader\FilesystemLoader and overwrites findTemplate() (\Twig\Loader\FilesystemLoader::findTemplate).

With Twig3 the return values of the findTemplate() method has been changed and should return string|null (please see https://github.com/twigphp/Twig/blob/3.x/src/Loader/FilesystemLoader.php#L167).

But \Symfony\Bundle\TwigBundle\Loader\FilesystemLoader::findTemplate() returns false if the template does not exists and $throw is false (see

symfony/src/Symfony/Bundle/TwigBundle/Loader/FilesystemLoader.php

Line 99 in 6e44447

return false;
).

Twig3 \Twig\Loader\FilesystemLoader::exists() expects null for not found templates (please see https://github.com/twigphp/Twig/blob/a5f91f3d8fb2965be8b715323dbdc81ce288a14a/src/Loader/FilesystemLoader.php#L153) and returns true for not existing templates.

How to reproduce
Use templateExists() with Symfony TwigBridge 4.4.1 and Twig 3.0 or adapt the test \Symfony\Bundle\TwigBundle\Tests\Loader\FilesystemLoaderTest::testTwigSoftErrorIfTemplateDoesNotExist to the Twig3 declaration.

The test use $this->assertFalse(…) but should be changed to assertNull().

Possible Solution

I've prepared a pull request which fixes the test and the return value.

Twig 2 should not be affected. The exists() method expects null or false for not found templates: https://github.com/twigphp/Twig/blob/fdb691a424682a524555f73b2c665c06971c4ed5/src/Loader/FilesystemLoader.php#L169
@dpesch dpesch mentioned this issue Dec 8, 2019
Merged
@dpesch dpesch changed the title [TwigBundle] FilesystemLoader::findTemplate() breaks Twig 3 FilesystemLoader:exists() [TwigBundle] FilesystemLoader::findTemplate() breaks FilesystemLoader::exists() with Twig3 Dec 8, 2019
dpesch added a commit to 11com7/symfony that referenced this issue Dec 10, 2019
@dpesch
bug symfony#34877 [TwigBundle] fix findTemplate() to return null
ff1d77e 
Twig3 FilesystemLoader::findTemplate() should return `string|null`
instead of Twig2 `string|null|false`: see
<https://github.com/twigphp/Twig/blob/3.x/src/Loader/FilesystemLoader.php#L167>

Returning `null` fixes `exists()` of Twig 3 FilesystemLoader without
breaking Twig 2 (which expected `null` or `false` for not found
templates).

Change the test to assert `null` instead of `false`.
fabpot added a commit that referenced this issue Dec 11, 2019
@fabpot
bug #34878 [TwigBundle] fix broken FilesystemLoader::exists() with Tw…
25494fa 
…ig 3 (dpesch)

This PR was merged into the 4.4 branch.

Discussion
----------

[TwigBundle] fix broken FilesystemLoader::exists() with Twig 3

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34877
| License       | MIT
| Doc PR        |

This fix adapts the declaration of Twig 3 `\Twig\Loader\FilesystemLoader::findTemplate()` which expects to return `string|null` and returns `null` instead of `false`.

Returning `false` breaks `\Twig\Loader\FilesystemLoader::exists()` which returns `true` if `findTemplate()` does not return `null`.

Twig 2 should not be affected by this patch. The `exists()` method expects `null` or `false` for not found templates: <https://github.com/twigphp/Twig/blob/fdb691a424682a524555f73b2c665c06971c4ed5/src/Loader/FilesystemLoader.php#L169>

Commits
-------

ff1d77e bug #34877 [TwigBundle] fix findTemplate() to return `null`
nicolas-grekas added a commit that referenced this issue Dec 12, 2019
@nicolas-grekas
Merge branch '4.4' into 5.0
cf073e5 
* 4.4:
  Fix merge
  [DoctrineBridge] try to fix deprecations from doctrine/persistence
  [DI] Add support for immutable setters in CallTrait
  [Cache] Propagate expiry when syncing items in ChainAdapter
  Removed request header "Content-Type" from the preferred format guessing mechanism
  [Routing] fix memoryleak when loading compiled routes
  [Translation] fix memoryleak in PhpFileLoader
  fix triggering deprecation in file locator
  bug #34877 [TwigBundle] fix findTemplate() to return `null`
nicolas-grekas added a commit that referenced this issue Dec 12, 2019
@nicolas-grekas
Merge branch '5.0'
b94f9e5 
* 5.0:
  Fix merge
  [DoctrineBridge] try to fix deprecations from doctrine/persistence
  [DI] Add support for immutable setters in CallTrait
  [Cache] Propagate expiry when syncing items in ChainAdapter
  Removed request header "Content-Type" from the preferred format guessing mechanism
  [Routing] fix memoryleak when loading compiled routes
  [Translation] fix memoryleak in PhpFileLoader
  fix triggering deprecation in file locator
  bug #34877 [TwigBundle] fix findTemplate() to return `null`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review TwigBundle
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@javiereguiluz @nicolas-grekas @dpesch @carsonbot