Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] Add HeaderUtils class#24699
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
eee2a20 to9b08a46Comparenicolas-grekas commentedOct 28, 2017
This is the kind of change that should be really fine, but is risky until proven right. I'm not sure this can be merged in 3.4 as this is a new feature, even if it fixes edge cases. |
7b3d34c to3674a7dComparec960657 commentedOct 28, 2017
At least some of the edge cases are easy to fix, e.g. those dealing with missing case-insensitivity. I don't think these problems are particularly important to fix in 3.x. They hardly occur in practice. So I wouldn't mind postponing this to 4.x. |
c960657 commentedNov 18, 2017
Rebased to master. |
| * | ||
| * @author Christian Schmidt <github@chsc.dk> | ||
| */ | ||
| class HeaderUtils |
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.
Shall this class be marked@internal?
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 don't think so. It is generally useful when parsing HTTP headers, including those not supported directly by HttpFoundation. It may even be used in other contexts than parsing incoming HTTP request headers.
c960657 commentedMar 6, 2018
Rebased to master. |
| * added`CannotWriteFileException`,`ExtensionFileException`,`FormSizeFileException`, | ||
| `IniSizeFileException`,`NoFileException`,`NoTmpDirFileException`,`PartialFileException` to | ||
| handle failed`UploadedFile`. | ||
| * added`HeaderUtility`. |
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.
HeaderUtils :)
| $assoc =array(); | ||
| foreach ($partsas$part) { | ||
| $name =strtolower($part[0]); | ||
| $value =isset($part[1]) ?$part[1] :true; |
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.
$part[1] ?? true
| * @return array Nested array with as many levels as there are characters in | ||
| * $separators | ||
| */ | ||
| publicstaticfunctionsplit($header,$separators) |
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 general type declarations / return types are missing
| publicfunctiontestQuote() | ||
| { | ||
| $this->assertEquals('foo', HeaderUtils::quote('foo')); |
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.
alsoassertSame as of here..
2bb54da to1ef098aComparec960657 commentedApr 20, 2018
@ro0NL I have addresses your comments. Thanks! |
| * | ||
| * @param array $parts Array of arrays | ||
| * | ||
| * @return array Associative 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.
I think many docblocks here could be removed. We prefer not adding them when they are of low added value.
While doing so, could you please squash your commits also?
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 assume you mean the individual@param and@return lines, not the entire docblock, right?
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's correct.
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.
Sure I can squash, but doesn't Github offer to do this automatically? I'm just trying to understand why it is relevant :-)
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.
we don't use github to merge
and the squashing feature of the tool we're using is broken right now :)
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.
Ok, cool :-) I have removed the unnecessary docblock lines and squashed my commits (it took a few attempts to get it right).
nicolas-grekas left a comment
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.
(with minor comment)
| returnself::groupParts($matches,$separators); | ||
| } | ||
| privatestaticfunctiongroupParts(array$matches,string$separators):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.
private methods should be afterpublic ones
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.
Fixed.
c92e14b to96f7606Comparefabpot commentedApr 22, 2018
Thank you@c960657. |
This PR was merged into the 4.1-dev branch.Discussion----------[HttpFoundation] Add HeaderUtils class| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |In several places in HttpFoundation we parse HTTP header values using a variety of regular expressions. Some of them fail in various corner cases.Parsing HTTP headers is not entirely trivial. We must be able to parse quoted strings with backslash escaping properly and ignore white-space in certain places.In practice, our limitations in this respect may not be a big problem. We only care about a few different HTTP request headers, and they are usually restricted to a simple values without quoted strings etc. However, this is no excuse for not doing it right :-)This PR introduces a new utility class for parsing headers. This allows Symfony itself and third-party code to parse HTTP headers in a robust way without using complex regular expressions that are difficult to write and error prone.Commits-------b435e80 [HttpFoundation] Add HeaderUtility class
| */ | ||
| publicstaticfunctionsplit(string$header,string$separators):array | ||
| { | ||
| $quotedSeparators =preg_quote($separators); |
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 should include the regex delimiter (/ as second argument of the call, in case it is one of the separators
| * Combines an array of arrays into one associative array. | ||
| * | ||
| * Each of the nested arrays should have one or two elements. The first | ||
| * value will be used as the keys in the associative array, and the second |
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's not what is done. The key is the associative array is thelowercased first value, not the first value.
| * Example: | ||
| * | ||
| * HeaderUtils::split("da, en-gb;q=0.8", ",;") | ||
| * // => array(array("da"), array("en-gb"), array("q", "0.8")) |
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.
Are you sure about this example ? Why is the header split on= when giving,; as separator ?
c960657 commentedApr 23, 2018 • 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.
This PR was squashed before being merged into the 4.1-dev branch (closes#27019).Discussion----------[HttpFoundation] Fixes to new HeaderUtils class| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |A follow-up to#24699 with a few code and documentation fixes for post-merge review comments by@stof.Commits-------d7c3c79 [HttpFoundation] Fixes to new HeaderUtils class
Uh oh!
There was an error while loading.Please reload this page.
In several places in HttpFoundation we parse HTTP header values using a variety of regular expressions. Some of them fail in various corner cases.
Parsing HTTP headers is not entirely trivial. We must be able to parse quoted strings with backslash escaping properly and ignore white-space in certain places.
In practice, our limitations in this respect may not be a big problem. We only care about a few different HTTP request headers, and they are usually restricted to a simple values without quoted strings etc. However, this is no excuse for not doing it right :-)
This PR introduces a new utility class for parsing headers. This allows Symfony itself and third-party code to parse HTTP headers in a robust way without using complex regular expressions that are difficult to write and error prone.