Hard to find improvements #34
Conversation
Allow running phpunit in tests folder alone. Generate coverage report.
Headers should end in CRLF make them so.
So that diff may stap complaining about missing new lines.
Test changes made to Accept and User Agent headers.
Test changes made to hasErrors , raw_headers, _parseHeaders. Additional tests against __toString and _parseCode in an attempt to get the coverage up.
Although not implicitly required added for completion.
Only status codes 4xx and 5xx should be considered errors. 1xx are informational and therefor not an error.
As per 2616 headers should end in CRLF parser should handle it appropriately. Additional headers are optional and prser should consider not getting headers to parse.
Compose an User-Agent header by collecting available information. This behaviour can be ommitted by adding a custom User-Agent request header.
If you can coneg against this Accept: header you can mark your implementation passed. Contains all the legal LWS. Preferable mime types are listed in reverse order, expected mime-type is added to the end. The order the server should sort these in are as follows 1 expected/type 0.9 text/html;level=3 0.8 text/plain 0.5 *.* This will also prevent you from getting 406 Not Acceptable errors.
Mainly done to be able to test against the composed header without a bajor refactor. This is now similar to the response and although there are other ways to get this information from cURL this now makes HttpFul complete being able to provide both request and response headers. Note the HTTP version is hard coded this might require additional work but should be efficient for the majority of use cases.
|
Looking at these changes in retrospect just feels so obvious, though I definitely learned new things about "proper" usage of the (Aside, @nickl-, I think the purpose of the user friendliness was to make the usage APIs as fluid as possible, hence: $httpful->withXMyAwesomeHeader('this works fine')
->withXMyOtherAwesomeHeader('this wouldnt work without the fluid interface');is really nice for metaprogramming (as I'm using it) and assigning a lot of headers.) @nategood, if you're taking votes, I +1 all of these. |
|
Whoa. A lot to go through here, but I like what I see so far. I'll give it a look over shortly. Thanks! |
|
|
||
| $this->assertEquals(Mime::JSON, $r->expected_type); | ||
| $r->_curlPrep(); | ||
| $this->assertContains('application/json', $r->raw_headers); |
There was a problem hiding this comment.
Missing whitespace here and on 188.
There was a problem hiding this comment.
tabs and spaces spaces and tabs =( my bad
This should fix it: @79b323c
|
In regard to the header design decision, one of the goals of this project was not only to provide a user friendly, sane alternative to cURL, but also to provide a clean, readable API. It was my intent that each HTTP request read almost as a sentence. This applies to both setting/getting headers and most of the other options provided by the API. For instance consider the two snippets: vs The former reads much more naturally (and in this case is a bit more concise). The latter syntax is based on another new, popular PHP HTTP library. I agree that there may be circumstances where a more traditional associative array might be desirable and there even may be developers that always favor this syntax. For that reason, I provided many alias methods and alternatives to some of the more controversial design decisions (like the magic methods for headers). In the case of the headers, you can always do the following with Httpful in its current state: I'm a firm believer that APIs should work for you, not you for it. If the developer strongly prefers one syntax or has a use case for a particular syntax, by all means they should feel free to use it. Hope that helps! |
|
As far as 2616 is concerned there are no invalid status codes hence no reason to validate against them, you won't believe how many frameworks and implementations go out of their way to check these. You SHOULD get at least a status line back from the host, it's the only header not marked optional but 2616 also calls for Tolerant Applications probably my most favourite aspect of the HTTP specification and cheering for the same team @nategood the machine should work for us not make us work. No one said all things are fair and equal though, while you must silently grin, bare and bend over backwards to cope with whatever garbage you receive and best you not make much fuss over it either or expose DOS vulnerabilities to boot. Unfortunately the specification is quite clear that you MUST understand the class of the status code which leaves you very little room to argue a defense for your strategy of using 1xx (information) class messages as errors. This excerpt explains... From the last paragraph of Section 6.1.1 Status Code and Reason Phrase
In the same manner as Roy Fielding's suggestion to turn un-identifiable codes down to the x00 status, something others further down the line may actually understand, nothing is stopping you from inventing your own codes for internal use to identify specific reasons, which may give hints to properly dealing with them, as long as you remain in the appropriate class, is all. Wikipedia catalogues the most up to date List of HTTP status codes I have found, albeit official or not. The following two examples, which MS has employed for years, serve as sufficient examples.
|
|
@zackdouglas =) glad you enjoyed that. It's even more fun on the other end negotiating content for these headers have a look see at this: @e740546 |
| $headers[] = !empty($this->expected_type) ? | ||
| "Accept: {$this->expected_type}, text/plain" : | ||
| "Accept: */*"; | ||
| $headers = array(); |
There was a problem hiding this comment.
Whoops. It looks like there are also some whitespace issues here. Probably a tab/space thing again. 4 space, soft tabs please :-)
There was a problem hiding this comment.
You are a slave driver =)
|
I finally ended up putting pushing a |
|
Yeah, you are correct that it shouldn't have been lumped in as an "http error". Did you get my message about re-opening this as a pull request into the |
|
Oh no! This is turning out to be much more work than I thought. I will have to get back to this at a later stage then. =/ |
|
Something in particular? I thought the request looked good after some of the whitespace tweaks. Reopen it for dev and it can go in. I wouldn't worry about the invalid HTTP status code handling just yet. |
|
Just the whole pul,l merge, tweak whitespace, push, resubmit pull request, running out of breath. =) I will get to it sometime tonight... just not something quick enough to do now. |
|
I have been asked to join Respect which now takes priority. Also rewriting Shuber's Curl which is taking up some time as well. Will still make time for you too, I promise. |
|
Haha no rush. But you shouldn't have to do all pulling/merging. Just reopen your same request (from your same branch nickl-:development) and change the target from nategood:master to nategood:dev. A few clicks ;-). I'll take care of the whitespace, etc.. |
Wow you've been busy! =)
I took this down a few weeks ago but only got around to checking it out now. Read through Response class and saw body, raw_body, headers and alarms went off NOOOO where's raw_header??? ...and then I saw $this->raw_headers were being populated in the constructor and it was only the property reference that was missing.
I quickly scanned my source and saw ahhh I can help with composer package etc, sure there are things I can do but when I came back to fork I realized that I was going to have to look deeper if I wanted to help. It was not easy but I think I found a few items that will be useful.
The commit messages will have additional comments but these should be among them.
User-Agent: HttpFul/1.0you might want to tweak the name and version number.$_SERVER['SERVER_SOFTWARE']and remove PHP from there should it exist.TERM_PROGRAMandTERM_PROGRAM_VERSIONfrom $_SERVER.I like what you've done to this implementation but maybe a tad too user friendly for my liking, is there such a thing as too user friendly?.. not that this should demotivate you there definitely is a place for this. I might steel a few concepts if you don't mind =)
The huge focus on the request headers was different and it works quite intuitively but I don't see how this is an improvement on say:
Maybe this is where my gripe with the user friendliness comes in, I want to know that's my header and I want to set it as I expect it without the camelCasing and "with" parsing and all the other bells getting in the way. Maybe if I sleep over it I will see things differently... we'll see.
Keep up the good work!