Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] Throw exception when PHP start tag is missing#18869
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
[Routing] Throw exception when PHP start tag is missing#18869
Uh oh!
There was an error while loading.Please reload this page.
Conversation
javiereguiluz commentedMay 25, 2016
A minor comment to reviewers: missing the opening PHP tag may seem like a stupid problem ... but please trust us when we say that we have received lots of reports about this error. We really need to fix this issue one way or another. Thank you! |
OskarStark commentedMay 25, 2016
PR looks appropriate to me 👍 , but yes it is a BC break |
| $namespace =false; | ||
| $tokens =token_get_all(file_get_contents($file)); | ||
| if (1 ===count($tokens) &&T_INLINE_HTML ===$tokens[0][0]) { |
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.
won't warn for code looking like
foo<?phpbarintended?
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.
I think so. We're catching the case of forgetting the open PHP tag here, not whether the PHP tag is the first token in the file.
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 needs to account for the fact that tokens are not always arrays
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.
the first one is always an array :)
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.
In this case, as it's the first item, only two options are available: inline HTML or PHP start tag. These both are arrays, so we don't have to worry about it.
Btw, I just discovered that we can simplify this by only doingcount($tokens) === 1, just<?php returns T_INLINE_HTML with<?php as value.
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 is because the PHP start tag is either<?php or<?phpEOL (with whatever supported EOL char)
linaori commentedMay 26, 2016
Besides of being a silly check forgotten, it's also quite embarrassing how you miss this error when helping someone. As more experienced phper, I kind of expect to see an open tag and if not present-make the basic assumption people simply left it out of the example. 👍 for this. |
nicolas-grekas commentedJun 3, 2016
rebase needed |
556cfa1 to1e765c8Comparewouterj commentedJun 7, 2016
Status: Needs Review |
nicolas-grekas commentedJun 7, 2016
👍 |
nicolas-grekas commentedJun 7, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Personally I'd be OK to merge this into 2.7 btw. The potential BC break is not more allowed on master than on 2.7. Thus if we're ok to consider its potentiality is near to zero, then 2.7 should be the target. |
fabpot commentedJun 7, 2016
@nicolas-grekas I don't agree. Introducing a potential BC break in master is ok and can be documented. Introducing a small BC break in a patch version is a no-go. |
dunglas commentedJun 12, 2016
👍 |
fabpot commentedJun 12, 2016
Thank you@wouterj. |
…g (WouterJ)This PR was merged into the 3.2-dev branch.Discussion----------[Routing] Throw exception when PHP start tag is missing| Q | A| ------------- | ---| Branch? | master (?)| Bug fix? | yes| New feature? | no| BC breaks? | yes| Deprecations? | no| Tests pass? | yes| Fixed tickets |symfony/symfony-docs#6116 (and many more)| License | MIT| Doc PR | -The Problem---In the documentation, we never use the PHP start tag. However, in the first tutorials, people simply copy/past the code examples, save the file and expect things to work. They seem to often forget to add the PHP start tag.Without start tag, the annotation file loader simply skips the file without providing any reason why. As a big framework is quite overwhelming, simple things like this are completely forgotten.The Fix---If a `*.php` file only consists of `T_INLINE_HTML`, it means there is no `<?php` start tag. In this case, instead of skipping the file, an exception is throwed with a possible fix.BC Break?---As the file loader is only executed for `*.php` files, I think the BC break is minimal, but it is possible that people have applications with `*.php` files *without* a start tag. If this file lives in a routing loaded directory (e.g. when doing `@AppBundle/Controller`), it would now result in an exception.I think this BC break is minimal and can be ignored. If you don't agree, we can add a little str match to check if `class ... {` exists. If that's the case, I think it's safe to say that it was meant to be a PHP file.Bug or Feature?---I don't know if this is considered a bug or a feature, so I submitted it as a feature.Commits-------1e765c8 Throw exception when PHP start tag is missing
…om annotations (jakzal)This PR was merged into the 2.7 branch.Discussion----------[Routing] Ignore hidden directories when loading routes from annotations| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#21497| License | MIT| Doc PR | -The problem surfaced after implementing#18869. Therefore it doesn't exist on 2.7, but I'd still merge it there to avoid conflicts when merging between branches. Without this fix, the oldest branch the added test will fail is 3.2.Commits-------ce9df02 [Routing] Ignore hidden directories when loading routes from annotations
The Problem
In the documentation, we never use the PHP start tag. However, in the first tutorials, people simply copy/past the code examples, save the file and expect things to work. They seem to often forget to add the PHP start tag.
Without start tag, the annotation file loader simply skips the file without providing any reason why. As a big framework is quite overwhelming, simple things like this are completely forgotten.
The Fix
If a
*.phpfile only consists ofT_INLINE_HTML, it means there is no<?phpstart tag. In this case, instead of skipping the file, an exception is throwed with a possible fix.BC Break?
As the file loader is only executed for
*.phpfiles, I think the BC break is minimal, but it is possible that people have applications with*.phpfileswithout a start tag. If this file lives in a routing loaded directory (e.g. when doing@AppBundle/Controller), it would now result in an exception.I think this BC break is minimal and can be ignored. If you don't agree, we can add a little str match to check if
class ... {exists. If that's the case, I think it's safe to say that it was meant to be a PHP file.Bug or Feature?
I don't know if this is considered a bug or a feature, so I submitted it as a feature.