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

[HttpFoundation] Add HeaderUtils class #24699

Merged
merged 1 commit into from Apr 22, 2018

Conversation

@c960657
Contributor

c960657 commented Oct 26, 2017

Q A
Branch? master
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.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Oct 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.
About these edge-cases, are they solvable, testable, important to fix now?

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Oct 28, 2017

@c960657

This comment has been minimized.

Contributor

c960657 commented Oct 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.

@xabbuh xabbuh changed the base branch from 3.4 to master Nov 15, 2017

@c960657

This comment has been minimized.

Contributor

c960657 commented Nov 18, 2017

Rebased to master.

*
* @author Christian Schmidt <github@chsc.dk>
*/
class HeaderUtils

This comment has been minimized.

@hhamon

hhamon Nov 23, 2017

Contributor

Shall this class be marked @internal?

This comment has been minimized.

@c960657

c960657 Nov 24, 2017

Contributor

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

This comment has been minimized.

Contributor

c960657 commented Mar 6, 2018

Rebased to master.

@fabpot

fabpot approved these changes Apr 6, 2018

@@ -15,6 +15,7 @@ CHANGELOG
* added `CannotWriteFileException`, `ExtensionFileException`, `FormSizeFileException`,
`IniSizeFileException`, `NoFileException`, `NoTmpDirFileException`, `PartialFileException` to
handle failed `UploadedFile`.
* added `HeaderUtility`.

This comment has been minimized.

@ro0NL

ro0NL Apr 19, 2018

Contributor

HeaderUtils :)

$assoc = array();
foreach ($parts as $part) {
$name = strtolower($part[0]);
$value = isset($part[1]) ? $part[1] : true;

This comment has been minimized.

@ro0NL

ro0NL Apr 19, 2018

Contributor

$part[1] ?? true

* @return array Nested array with as many levels as there are characters in
* $separators
*/
public static function split($header, $separators)

This comment has been minimized.

@ro0NL

ro0NL Apr 19, 2018

Contributor

in general type declarations / return types are missing

public function testQuote()
{
$this->assertEquals('foo', HeaderUtils::quote('foo'));

This comment has been minimized.

@ro0NL

ro0NL Apr 19, 2018

Contributor

also assertSame as of here..

@c960657 c960657 changed the title from [HttpFoundation] Add HeaderUtility class to [HttpFoundation] Add HeaderUtils class Apr 20, 2018

@c960657

This comment has been minimized.

Contributor

c960657 commented Apr 20, 2018

@ro0NL I have addresses your comments. Thanks!

*
* @param array $parts Array of arrays
*
* @return array Associative array

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 20, 2018

Member

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?

This comment has been minimized.

@c960657

c960657 Apr 20, 2018

Contributor

I assume you mean the individual @param and @return lines, not the entire docblock, right?

This comment has been minimized.

@fabpot

fabpot Apr 20, 2018

Member

That's correct.

This comment has been minimized.

@c960657

c960657 Apr 20, 2018

Contributor

Sure I can squash, but doesn't Github offer to do this automatically? I'm just trying to understand why it is relevant :-)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 20, 2018

Member

we don't use github to merge
and the squashing feature of the tool we're using is broken right now :)

This comment has been minimized.

@c960657

c960657 Apr 20, 2018

Contributor

Ok, cool :-) I have removed the unnecessary docblock lines and squashed my commits (it took a few attempts to get it right).

@nicolas-grekas

(with minor comment)

return self::groupParts($matches, $separators);
}
private static function groupParts(array $matches, string $separators): array

This comment has been minimized.

@xabbuh

xabbuh Apr 20, 2018

Member

private methods should be after public ones

This comment has been minimized.

@c960657

c960657 Apr 20, 2018

Contributor

Fixed.

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 22, 2018

Thank you @c960657.

@fabpot fabpot merged commit b435e80 into symfony:master Apr 22, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 22, 2018

feature #24699 [HttpFoundation] Add HeaderUtils class (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
*/
public static function split(string $header, string $separators): array
{
$quotedSeparators = preg_quote($separators);

This comment has been minimized.

@stof

stof Apr 23, 2018

Member

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

This comment has been minimized.

@stof

stof Apr 23, 2018

Member

that's not what is done. The key is the associative array is the lowercased 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"))

This comment has been minimized.

@stof

stof Apr 23, 2018

Member

Are you sure about this example ? Why is the header split on = when giving ,; as separator ?

@c960657 c960657 deleted the c960657:header-utils branch Apr 23, 2018

@c960657

This comment has been minimized.

Contributor

c960657 commented Apr 23, 2018

@stof You are right x 3 :-) I have created a follow-up PR: #27019

fabpot added a commit that referenced this pull request Apr 27, 2018

minor #27019 [HttpFoundation] Fixes to new HeaderUtils class (c960657)
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

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment