-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
Add request header accessors (à la PSR-7) #103
Conversation
src/Request.php
Outdated
* there's only a single value or an array of strings if this header has | ||
* multiple values. | ||
* | ||
* Note that this differs from the PSR-7 implementation of this method. |
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.
It's probably worth mentioning in which way it differs.
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.
Agreed, could you add that @clue?
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.
Absolutely 👍
src/Request.php
Outdated
* there's only a single value or an array of strings if this header has | ||
* multiple values. | ||
* | ||
* Note that this differs from the PSR-7 implementation of this method. |
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.
Agreed, could you add that @clue?
src/Request.php
Outdated
foreach ($this->headers as $key => $value) { | ||
if (strtolower($key) === $name) { | ||
foreach((array)$value as $one) { | ||
$found []= $one; |
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.
Shouldn't this be $found[] = $one;
?
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.
There are valid reasons for both styles IMO, but I have no real preference here, so yeah 👍
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.
Not used to this syntax, but I prefer to be consistent 😄
Updated |
@clue What's the reason for differing from PSR-7 here? I think the PSR-7 behavior makes more sense. |
@kelunik mainly that this will go into 0.4.3 and PSR-7 will come in 0.5 so this is more a step in the right direction 😄 |
@WyriHaximus I missed that |
@kelunik np 😄 |
Rebased on current master to resolve merge conflicts |
Note that this PR does not implement the PSR-7 interfaces, but follows its method signatures for the header methods to ease a future switch to PSR-7 via #28.
This is also the base for an upcoming PR that fixes #51.