Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Fix a regression in handling absolute template paths#17894
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| if (!preg_match('/^(?:([^:]*):)?(?:([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/',$name,$matches)) { | ||
| if (!preg_match('/^(?:([^:]*):([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/',$name,$matches) ||$this->isAbsolute($name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Two groups replaced with one here. This will only match "Foo:Bar:" or "".
jakzal commentedFeb 22, 2016
@francoispluchino@wesleylancel would you mind checking if this fixes your issues please? |
francoispluchino commentedFeb 22, 2016
@jakzal It's OK for me! Thank you for this PR. |
jakzal commentedFeb 22, 2016
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. |
fabpot commentedFeb 23, 2016
Not really related to this PR, but loading templates via an absolute path looks terrible. |
francoispluchino commentedFeb 24, 2016
@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. |
jakzal commentedFeb 24, 2016
Back to the topic... I confirmed wesleylancel's problem still persists. I'll work on a fix this afternoon. |
6f5d2d3 tode3d770Comparejakzal commentedFeb 29, 2016
@hcomnetworkers can you try this patch please? status: needs review |
hcomnetworkers commentedMar 1, 2016
jakzal commentedMar 1, 2016
Several people confirmed this fixes the issue for them. status: reviewed |
| return$this->cache[$name] =$template; | ||
| } | ||
| privatefunctionisAbsolute($path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you rename toisAbsolutePath() as this is the name we are using elsewhere.
Also, I'd prefer to use a more restrictive approach like done elsewhere:
privatefunctionisAbsolutePath($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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 becomesC:path/to/section/name:foo.html.php. I really don't know what's this for.
de3d770 tofe1f5d2Comparefe1f5d2 tod8c493fComparefabpot commentedMar 3, 2016
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
stof commentedMar 3, 2016
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. Seehttps://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. |
fabpot commentedMar 3, 2016
jakzal commentedMar 4, 2016
@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. |
| thrownew \RuntimeException(sprintf('Template name "%s" contains invalid characters.',$name)); | ||
| } | ||
| if (!preg_match('/^(?:([^:]*):)?(?:([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/',$name,$matches)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That breaks FooBundle:Post/file.html.twig pattern
Regression introduced by#15272.