[FrameworkBundle] Fix a regression in handling absolute template paths #17894
Conversation
| @@ -56,7 +56,7 @@ public function parse($name) | |||
| throw new \RuntimeException(sprintf('Template name "%s" contains invalid characters.', $name)); | |||
| } | |||
|
|
|||
| if (!preg_match('/^(?:([^:]*):)?(?:([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/', $name, $matches)) { | |||
| if (!preg_match('/^(?:([^:]*):([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/', $name, $matches) || $this->isAbsolute($name)) { | |||
There was a problem hiding this comment.
Two groups replaced with one here. This will only match "Foo:Bar:" or "".
|
@francoispluchino @wesleylancel would you mind checking if this fixes your issues please? |
|
@jakzal It's OK for me! Thank you for this PR. |
|
I just confirmed it also fixes the issue reported by @Congelli501. Waiting for @wesleylancel to either confirm his case's fixed, or to provide a failing scenario. |
|
Not really related to this PR, but loading templates via an absolute path looks terrible. |
|
@fabpot I agree with you if you load a template from a Bundle or App Resources, but when the template file is generated dynamically, and it is not in a Bundle we have no other choice. Personally, I have this problem with the mail templates that are accessible via a custom stream, with the email generator that provided me the absolute path of the template. So, dynamically add the template file before loading the template is not very subtle, Because the Of course, I am interested by best practices, if you have any. |
|
Back to the topic... I confirmed wesleylancel's problem still persists. I'll work on a fix this afternoon. |
jakzal
force-pushed
the
template-name-parser-regression
branch
from
6f5d2d3
to
de3d770
Compare
|
@hcomnetworkers can you try this patch please? status: needs review |
|
Several people confirmed this fixes the issue for them. status: reviewed |
| @@ -72,4 +72,9 @@ public function parse($name) | |||
|
|
|||
| return $this->cache[$name] = $template; | |||
| } | |||
|
|
|||
| private function isAbsolute($path) | |||
There was a problem hiding this comment.
Can you rename to isAbsolutePath() as this is the name we are using elsewhere.
Also, I'd prefer to use a more restrictive approach like done elsewhere:
private function isAbsolutePath($file)
{
return $file[0] === '/' || $file[0] === '\\'
|| (strlen($file) > 3 && ctype_alpha($file[0])
&& $file[1] === ':'
&& ($file[2] === '\\' || $file[2] === '/')
);
}There was a problem hiding this comment.
This won't work with windows paths which are "normalised" for some reasons here:
$name = str_replace(':/', ':', preg_replace('#/{2,}#', '/', str_replace('\\', '/', $name)));C:\\path\\to\\section\\name:foo.html.php becomes C:path/to/section/name:foo.html.php. I really don't know what's this for.
jakzal
force-pushed
the
template-name-parser-regression
branch
from
de3d770
to
fe1f5d2
Compare
jakzal
force-pushed
the
template-name-parser-regression
branch
from
fe1f5d2
to
d8c493f
Compare
|
Thank you @jakzal. |
…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
The right way to do this would be to come up with your own convention about template names and a template loader able to resolve it. See https://github.com/Behat/Borg/blob/2d8422d006b32cff929a0e475acb473258911baf/src/Integration/Symfony/Documentation/Twig/DocumentationLoader.php for an example doing it @fabpot @jakzal what do you think about deprecating the possibility to use absolute paths ? AFAIK, this was never intended to work. It was just a side-effect of the initial implementation. |
|
@stof indeed looks like many cases were supported unintentionally. Since there was no test cases for them I wasn't sure if it was intended. I'll send a PR to deprecated absolute paths. |
| @@ -56,7 +56,7 @@ public function parse($name) | |||
| throw new \RuntimeException(sprintf('Template name "%s" contains invalid characters.', $name)); | |||
| } | |||
|
|
|||
| if (!preg_match('/^(?:([^:]*):)?(?:([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/', $name, $matches)) { | |||
There was a problem hiding this comment.
That breaks FooBundle:Post/file.html.twig pattern
Regression introduced by #15272.