[FrameworkBundle] Fix template location for PHP templates #15272
Conversation
jakzal
force-pushed
the
bugfix/php-template-location
branch
from
d024fde
to
cd42e2d
Compare
|
@jakzal I see you've enhanced the test - but I'm confused (in part, because I honestly haven't spent much time looking into the template name parsing stuff): this doesn't contain a fix for #14804 or add a failing test case. Was this just a step 1 to enhance to test? Or am I missing something (or perhaps I haven't answered your questions well yet on #14804). Thanks for taking this on - I see PR's from you fixing issues. It's awesome. |
|
@weaverryan indeed. That's why it's marked WIP. I updated PR's description. Few test cases are either wrong ("name.twig" and "name"), or we should treat them differently then templates that follow the "name.html.twig" pattern. |
jakzal
changed the title
|
Are "name.twig" and "name" even valid template names? I went with the second option for now to avoid changing existing cases. |
|
@jakzal I don't know if |
| array('/path/to/section/name.php', '/path/to/section/name.php', '/path/to/section/name.php', new BaseTemplateReference('/path/to/section/name.php', 'php')), | ||
| array('name.twig', 'name.twig', 'name.twig', new BaseTemplateReference('name.twig', 'twig')), | ||
| array('name', 'name', 'name', new BaseTemplateReference('name')), | ||
| array('default/index.html.php', '::default/index.html.php', 'views/default/index.html.php', new TemplateReference(null, null, 'default/index', 'html', 'php')), |
There was a problem hiding this comment.
So this last entry is the only one that was really added or changed at all, right?
What I don't understand yet is why default/index.html.twig seems to have always pointed to ::default/index.html.twig but default/index.html.php did not work correctly (and it only starts working "incorrectly" once both the twig and php engines are enabled).
|
status: needs review |
|
ping @symfony/deciders |
|
I have some hard time wrapping my head around these changes, but the result looks like what we expect. |
|
Thank you @jakzal. |
…(jakzal) This PR was merged into the 2.3 branch. Discussion ---------- [FrameworkBundle] Fix template location for PHP templates | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #14804 | License | MIT | Doc PR | - - [x] improve the test to cover logical path & filesystem path - [x] Add a new test case and fix the path to the template As the first commit only enchanced the test, and the second commit fixed the bug, it's best to review them seperately. Commits ------- 132a4e4 [FrameworkBundle] Fix template location for PHP templates cd42e2d [FrameworkBundle] Add path verification to the template parsing test cases
…mplate paths (jakzal) This PR was merged into the 2.3 branch. Discussion ---------- [FrameworkBundle] Fix a regression in handling absolute template paths | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #17777 #17683 | License | MIT | Doc PR | - Regression introduced by #15272. Commits ------- d8c493f [FrameworkBundle] Fix a regression in handling absolute and namespaced template paths
As the first commit only enchanced the test, and the second commit fixed the bug, it's best to review them seperately.