Skip to content

Single header line parsing extracted to a separate method in ResponseBuilder #40

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

Merged
merged 3 commits into from
Mar 29, 2016
Merged

Single header line parsing extracted to a separate method in ResponseBuilder #40

merged 3 commits into from
Mar 29, 2016

Conversation

mekras
Copy link
Contributor

@mekras mekras commented Mar 23, 2016

Allows more flexible use of the class.

@sagikazarmark
Copy link
Member

LGTM. Wonder if we should add more tests for the addHeader method separately.

@mekras
Copy link
Contributor Author

mekras commented Mar 25, 2016

@sagikazarmark, may be. But a) new method is fully covered with existed tests; b)
I don't feel familiar enough with phpspec.

@sagikazarmark
Copy link
Member

Ok. Lemme check if I can do something on this front during the weekend.

$this->response = $this->response
->withStatus((int) $parts[1], $reasonPhrase)
->withProtocolVersion(substr($parts[0], 5));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think this should be handled via the addHeader method as it's not a header but more the status string, also now a request like this :

Header: headervalue
GET / HTTP/1.1

would be correctly parsed but is invalid and should throw an error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

@joelwurtz
Copy link
Member

👍 LGTM

@sagikazarmark sagikazarmark merged commit a814fba into php-http:master Mar 29, 2016
@mekras mekras deleted the response_builder_refactoring branch June 24, 2016 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants