Skip to content
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

Remove guzzle parser #26

Closed
wants to merge 13 commits into from
Closed

Remove guzzle parser #26

wants to merge 13 commits into from

Conversation

hiend
Copy link

@hiend hiend commented May 1, 2015

Simple response parser

@WyriHaximus WyriHaximus self-assigned this May 1, 2015
list($header, $value) = array_map('trim', explode(':', $line, 2));

if (!isset($headers[$header])) {
$headers[$header] = $value;
Copy link
Member

Choose a reason for hiding this comment

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

IMO I think it would be better if $value was always an array. This would prevent the consumer from having to type check.

@cboden
Copy link
Member

cboden commented May 1, 2015

A good start! Off the top of my head this is also a valid multi value header:

A-Header: value1,value2,value3

@hiend
Copy link
Author

hiend commented May 1, 2015

hmm, values may contain comma for a different reasons:

Last-Modified: Wed**,** 28 Apr 2015 11:20:59 GMT

@WyriHaximus
Copy link
Member

Aren't header values seperated by ; and not ,?

@hiend
Copy link
Author

hiend commented May 1, 2015

github response :)

HTTP/1.1 200 OK
Server: GitHub.com
Date: Fri, 01 May 2015 18:47:47 GMT
Content-Type: text/html; charset=utf-8
Transfer-Encoding: chunked
Status: 200 OK
Content-Security-Policy: blablabla
Cache-Control: no-cache
X-GitHub-User: hiend
X-GitHub-Session-Id: blablabla
Vary: X-PJAX
X-UA-Compatible: IE=Edge,chrome=1
Set-Cookie: user_session=blablabla; path=/; expires=Fri, 15-May-2015 18:47:47 GMT; secure; HttpOnly
Set-Cookie: _gh_sess=blablabla; path=/; secure; HttpOnly
X-Request-Id: blabla
X-Runtime: 0.238943
X-GitHub-Request-Id: bla:bla:bla:bla
Strict-Transport-Security: max-age=31536000; includeSubdomains; preload
X-Content-Type-Options: nosniff
X-XSS-Protection: 1; mode=block
X-Frame-Options: deny
Vary: Accept-Encoding
X-Served-By: blabla
Content-Encoding: gzip

@WyriHaximus
Copy link
Member

👍

Also we would very highly appreciate unit tests.


list($protocol, $version) = explode('/', $http, 2);

if ($protocol !== 'HTTP' or $version !== '1.0') {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about the pinning down on 1.0. Using it to talk to 1.1 servers a lot and that works fine. Not sure what their responses are but blocking 1.1 responses while they used to work in previous 0.4.x versions could be a BC break. Will investigate for you :).

@WyriHaximus
Copy link
Member

I'm closing this issue in favor of #34. You've done a great job but after @cboden reactphp/http#29 I've realized that using an external package that is specialized in message parsing is the better way to go. Your work is much appreciated and this decision isn't because of the work you've put into it. Once again thank you 👍

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.

None yet

3 participants