-
-
Notifications
You must be signed in to change notification settings - Fork 142
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
Support for default request headers #461
Conversation
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.
Hey 👋, nice PR ! Found a few typos here and there, hope it helps :)
Thank you @nhedger for your help :) |
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.
@51imyy Thank you very much for working on this, looks like a great feature and especially love the test suite and bonus points for added documentation!
Direction looks good to me, I've only added some remarks to bring this more in line with the rest of the project. Looking forward to get this sorted, keep it up! 👍
src/Browser.php
Outdated
private function findCaseInsensitivHeader($headers, $header) | ||
{ | ||
$found = null; | ||
/** @var string $key */ |
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.
I agree that this seems reasonable and I'd love this to be true, but the array ["1" => "first"]
uses (int) 1
as key automatically.
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.
the function got inlined. Im not that sure if that was your intention but i now changed it to "string|int $key". I could also just remove the annotation
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.
Hey @51imyy, great work on this so far 👍
I only found a few typos, the rest looks good to me. One thing I noticed is that you added quite a large number of commits for this, can you squash them together into one commit?
I am getting banned from cloudflare for having the default user agent in this library. Is it possible to remove that header from here - http/src/Client/RequestData.php Line 32 in b3ff9c8
withoutHeader can remove the User-Agent specified in mergeRequestHeaders too.
|
@frosty00 Good catch! Agree that it should be possible to remove the default @51imyy Good job with the PR so far! Is this something you can look into as part of this PR? 👍 |
How about removing the default User Agent from here: http/src/Client/RequestData.php Line 32 in b5a66a4
and re-adding it in the Browser constructor via your new withHeader() function? Then it should be easily removable with withoutHeader() . What do you think about this?
|
yeah this looks like the correct solution |
f7f9bf0
to
32e0b33
Compare
added the suggestion and squashed the commits. Now you can use the |
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.
@51imyy Great work 👍
I found a few more things that need to be changed. Most of them are nits, there's also one slightly larger issue describing a currently uncovered case.
src/Browser.php
Outdated
foreach ($this->defaultHeaders as $key => $value) { | ||
if ($headers === array()) { | ||
$headers = $this->defaultHeaders; | ||
break; | ||
} | ||
foreach (\array_keys($headers) as $headerKey) { | ||
if (\strcasecmp($headerKey, $key) !== 0) { | ||
$headers[$key] = $value; | ||
} | ||
} | ||
} |
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.
I am not sure if this shows the right behavior if a default header is set and and the same header (with a different value) is also passed as an explicit header into the requestMayBeStreaming()
method. The current implementation will always overwrite the explicit header and I don't think that's what supposed to happen here. I recommend writing tests which use multiple explicit headers and multiple default headers. This way we can assure that these scenarios behave as expected.
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.
strcasecmp()
returns 0 if the strings are the same (case-insensitiv). So only headers which are not in the explicit headers, will be added. But i foud a bug, while writing a test with multiple default and multiple explicit headers, which i have to fix.
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.
wrote another (longer) test and fixed the bug.
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.
Just two nits
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.
@51imyy Sorry for the late response, here are some additional nits I found when looking over your changes.
2f7d638
to
443fdf3
Compare
applied the suggestions. I just added a missing opining tag (```php) to the README.md |
@51imyy I accidentally marked the first half of the examples (in |
@SimonFrings I thought it was intended, so i applied it 😅 . I can change it back. So the real intention was only to change |
@51imyy Yep, I messed my suggestion up. |
@SimonFrings Changed it |
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.
Only indentation is a bit off.
appiled the suggestions. |
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.
The final touch, after this one I will stop to annoy you 😅
README.md
Outdated
add a request header for all following requests. | ||
```php |
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.
Can you add an empty line between these two (you already did this in the withoutHeader()
description)
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.
added a new line. :D
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.
Awesome work, you got my approval 👍
Thank you :) |
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.
@51imyy Thanks for the quick updates! I've added some minor remarks below, otherwise LGTM and would love to get this shipped!
…ader user-agent to the default headers.
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.
@51imyy Thanks for the update, changes LGTM, now let's get this shipped! Keep it up! 👍
closes issue: #449
new methods for the class Browser (withHeader and withoutHeader)
With this methods you are able to set/remove default headers