Support custom HTTP headers #6
Conversation
|
Nice, thanks for filing this! 👍 I like this feature and would love to get this in! May I ask you provide a update this with documentation and some example and tests? Thanks! |
Unfortunately, the PR does not currently contain any documentation, examples or tests, so it's a bit unclear how this can be used from a consumer perspective.
valga
changed the title
|
@clue, docs, examples and tests are done. But I've stuck with Travis. I used pecl_http extension to parse HTTP messages in tests. It turned out that PHP5 requires 2.6 branch of pecl_http, while PHP7 requires 3.1 branch, and it seems that HHVM doesn't like pecl_http at all. Any idea how to deal with it? |
|
@clue Hi Christian. We've reached a bit of a bad situation... Valga extended this library to be able to communicate with Instagram's chat servers, for use in our very popular Instagram API repo. Unfortunately it turns out that Composer cannot recursively load "repository forks", so now all our users will be stuck unable to download our library, until the changes are merged into this official repo and we no longer need to fork it. Ouch. So today we learned the hard way that Composer cannot load forked repos from sublibraries. :-\ Anyway, Valga is one of the most perfectionistic programmers I've ever seen and I can see that he's done a great job here too. Could you please see if you can merge the changes soonish? Until then our users will be unable to use our in-dev version of our library. Ouch. |
|
@SteveJobzniak Thanks for the ping :-) I agree that maintaining custom forks is a real PITA and this is actually a high quality PR by @valga, so I'll look into getting this in within the upcoming week |
|
@clue Thank you very much, that's absolutely wonderful to hear. <3 :-) |
There was a problem hiding this comment.
@valga Thanks for this high quality PR!
I've decided to split this feature up into a dedicated PR so that basic proxy authentication support has recently been added via #14 (also ping @SteveJobzniak).
I'd like to get the custom headers feature in for the next release, what do you think about the below feedback? 👍
README.md
Outdated
| $proxy = new ProxyConnector('127.0.0.1:8080', $connector, array( | ||
| 'X-Custom-Header' => 'Value', | ||
| )); | ||
| ``` |
There was a problem hiding this comment.
What happens if you pass common headers such as Host? See also below wrt Proxy-Authorization.
Does it make sense to pass a "template request object" here instead of passing raw header values? This would potentially allow further customizations of the request object without cluttering the API in the future.
README.md
Outdated
| ``` | ||
|
|
||
| It should be noted, that custom `Proxy-Authorization` header take precedence | ||
| over credentials passed in proxy address. |
There was a problem hiding this comment.
I'm undecided on this, which one should have preference?
.travis.yml
Outdated
| @@ -11,6 +11,7 @@ php: | |||
| sudo: false | |||
|
|
|||
| install: | |||
| - printf "\n" | pecl install -f pecl_http-2.6.0 | |||
There was a problem hiding this comment.
This can probably be avoided, see also #14
composer.json
Outdated
| @@ -26,6 +26,7 @@ | |||
| }, | |||
| "require-dev": { | |||
| "react/socket-client": "^0.5", | |||
| "clue/block-react": "^1.1" | |||
| "clue/block-react": "^1.1", | |||
| "ext-http": "*" | |||
There was a problem hiding this comment.
See above, can probably be removed.
|
Whoa, there was so many changes, so I rewound the branch. Also, nice catch on urlencoded credentials. I rewrote custom headers feature by using a HTTP request template. It is just a draft, I didn't update the docs yet. Also, I didn't write a test set for custom headers, because I think we need to change the approach for testing outgoing HTTP requests. For now, we just compare them string-by-string. What do you think of running HTTP parser on them? |
valga
changed the title
|
@valga Thank you for both the quick update and your patience! Let's look into why this wasn't merged yet, I think it's about time :-) Functionally, I don't have any major objections with this PR. However, API-wise I'm not sure my suggestion to use a "template request" was a good idea to begin with. The provided example code probably looks a bit "strange" from a consumer perspective and I wonder if we really have a use case that justifies passing a full request object with all its properties (most of which are apparently unused). What do you think about this? Does it make more sense to revert the API to use a simple assoc array for custom HTTP request headers again (possibly keeping parts of the implementation details)? Any input would be much appreciated! 👍 |
|
@clue Rebased with simple array for headers, and some tests. |
See #3.