Skip to content

[3.0.2][BUG][Templating] Regression introduced in TemplateNameParser #17683

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
francoispluchino opened this issue Feb 4, 2016 · 13 comments
Closed

Comments

@francoispluchino
Copy link
Contributor

With the commit 4003532, The new regex pattern at the line 59 in Symfony\Bundle\FrameworkBundle\Templating\TemplateNameParser::parse confuses the letter of the Windows drive C:\ with a name of bundle.

This bug is only on Windows Platform with Symfony version:

  • 3.0.2,
  • 3.0.x-dev,
  • 2.8.x-dev, >2.8.2
@jakzal
Copy link
Contributor

jakzal commented Feb 4, 2016

@francoispluchino what kind of a template path are you talking about exactly?

'C:\\path\\to\\section\\name.php' would be converted to C:path/to/section/name.php even with the old regex.

@francoispluchino
Copy link
Contributor Author

Yes the path is correctly converted by

$name = str_replace(':/', ':', preg_replace('#/{2,}#', '/', str_replace('\\', '/', $name)));

But with the new regex

if (!preg_match('/^(?:([^:]*):)?(?:([^:]*):)?(.+)\.([^\.]+)\.([^\.]+)$/', $name, $matches)) {

the result of match is true and not false (normally).

This gives a TemplateReference with a bundle name C defined by $matches[1].

It is not the case with the old pattern.

@jakzal
Copy link
Contributor

jakzal commented Feb 4, 2016

What I was saying earlier is that both regular expressions work the same with the example path I gave (both don't match 'C:\\path\\to\\section\\name.php'). See https://3v4l.org/DlsP3

@francoispluchino
Copy link
Contributor Author

It's not the case with:

$name = 'C:\\path\\to\\section\\name.html.twig';

@jakzal
Copy link
Contributor

jakzal commented Feb 4, 2016

That's what I was after (an example). What's the outcome you expect?

@francoispluchino
Copy link
Contributor Author

preg_match result:
0 not 1

matches result:

[]

not:

[
 "C:path/to/section/name.html.twig",
 "C",
 "",
 "path/to/section/name",
 "html",
 "twig"
]

The problem is with the file names with many dot.

@xabbuh
Copy link
Member

xabbuh commented Feb 12, 2016

see also #17777 which contains some more information how to reproduce this issue

@jakzal
Copy link
Contributor

jakzal commented Feb 22, 2016

@francoispluchino any idea why the name is normalised to C:path/to/section/name.php? I'm not a windows user but this seems wrong as it misses the / after C:.

@francoispluchino
Copy link
Contributor Author

No I haven't idea, but the bug is caused by the name with more dot, not by the drive (ex. name.html.twig).
With the old version, the normalization already used the format with only c:, and it was not a problem..

@jakzal
Copy link
Contributor

jakzal commented Feb 22, 2016

@francoispluchino I know that, just trying to understand why it's normalised to this form :) Thanks anyway!

@francoispluchino
Copy link
Contributor Author

@fabpot Why the TemplateNameParser normalize the path without slash after the letter of Windows drive? Is it expected, or it's a bug?

C:path/to/section/name.html.twig and not C/:path/to/section/name.html.twig.

@jakzal I think it is better to focus on the end of the regex pattern, which blocks the use of file names with more dots.

@hcomnetworkers
Copy link

Template names starting with "@" are also broken (see ticket #17958). Are they also considered absolute paths, or should I reopen the other ticket?

@jakzal
Copy link
Contributor

jakzal commented Feb 29, 2016

@hcomnetworkers they're not considered absolute paths, but I'm fixing them as part of #17894 (that's why it's not merged yet).

fabpot added a commit that referenced this issue Mar 3, 2016
…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
@fabpot fabpot closed this as completed Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants