Support for default request headers#461

51imyyy · 2022-06-24T19:02:27Z

closes issue: #449
new methods for the class Browser (withHeader and withoutHeader)

With this methods you are able to set/remove default headers

nhedger

Hey 👋, nice PR ! Found a few typos here and there, hope it helps :)

README.md

src/Browser.php

tests/BrowserTest.php

51imyyy · 2022-06-24T19:30:42Z

Thank you @nhedger for your help :)

clue

@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! 👍

README.md

src/Browser.php

clue · 2022-06-25T20:20:48Z

src/Browser.php

+privatefunctionfindCaseInsensitivHeader($headers, $header)
+{
+$found = null;
+/** @var string $key */


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.

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

SimonFrings

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?

README.md

src/Browser.php

frosty00 · 2022-08-03T23:11:51Z

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

'User-Agent' => 'ReactPHP/1',

and make it optional. I think that withoutHeaders will only apply to the headers that are added using withHeader. Can we make it so that withoutHeader can remove the User-Agent specified in mergeRequestHeaders too.

clue · 2022-08-05T18:21:08Z

@frosty00 Good catch! Agree that it should be possible to remove the default User-Agent: ReactPHP/1 request header with a withoutHeader('User-Agent') call.

@51imyy Good job with the PR so far! Is this something you can look into as part of this PR? 👍

src/Browser.php

dinooo13 · 2022-08-12T21:56:07Z

How about removing the default User Agent from here:

http/src/Client/RequestData.php

Line 32 in b5a66a4

'User-Agent' => 'ReactPHP/1',

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?

frosty00 · 2022-08-12T23:41:50Z

How about removing the default User Agent from here:
http/src/Client/RequestData.php
Line 32 in b5a66a4
'User-Agent' => 'ReactPHP/1',
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

51imyyy · 2022-08-13T17:54:19Z

added the suggestion and squashed the commits. Now you can use the withoutHeader() to remove the default header "User-Agent"

SimonFrings

@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

README.md

SimonFrings · 2022-08-16T08:56:03Z

src/Browser.php

+foreach ($this->defaultHeadersas$key => $value){
+if ($headers === array()){
+$headers = $this->defaultHeaders;
+break;
+ }
+foreach (\array_keys($headers) as$headerKey){
+if (\strcasecmp($headerKey, $key) !== 0){
+$headers[$key] = $value;
+ }
+ }
+ }


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.

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.

wrote another (longer) test and fixed the bug.

src/Browser.php

SimonFrings

Just two nits

README.md

SimonFrings

@51imyy Sorry for the late response, here are some additional nits I found when looking over your changes.

README.md

src/Browser.php

51imyyy · 2022-08-30T11:10:25Z

applied the suggestions.

I just added a missing opining tag (```php) to the README.md

SimonFrings · 2022-09-01T07:29:48Z

@51imyy I accidentally marked the first half of the examples (in README.md and docblocks) in my last review which now causes that half to be missing 😅

51imyyy · 2022-09-01T08:22:03Z

@SimonFrings I thought it was intended, so i applied it 😅 . I can change it back. So the real intention was only to change var_dump($response->getHeaders()); to var_dump($response); right?

SimonFrings · 2022-09-01T09:43:20Z

So the real intention was only to change var_dump($response->getHeaders()); to var_dump($response); right

@51imyy Yep, I messed my suggestion up.

51imyyy · 2022-09-01T09:49:19Z

@SimonFrings Changed it

SimonFrings

Only indentation is a bit off.

src/Browser.php

51imyyy · 2022-09-01T09:58:09Z

appiled the suggestions.

SimonFrings

The final touch, after this one I will stop to annoy you 😅

SimonFrings · 2022-09-01T11:23:09Z

README.md

+add a request header for all following requests.
+```php


Can you add an empty line between these two (you already did this in the withoutHeader() description)

added a new line. :D

SimonFrings

Awesome work, you got my approval 👍

51imyyy · 2022-09-01T14:25:16Z

Thank you :)

clue

@51imyy Thanks for the quick updates! I've added some minor remarks below, otherwise LGTM and would love to get this shipped!

README.md

src/Browser.php

tests/BrowserTest.php

src/Browser.php

…ader user-agent to the default headers.

clue

@51imyy Thanks for the update, changes LGTM, now let's get this shipped! Keep it up! 👍

tests/BrowserTest.php

nhedger reviewed Jun 24, 2022
View reviewed changes

clue changed the title ~~[FEATURE] | Support for default headers~~Support for default request headersJun 25, 2022

clue added the new feature label Jun 25, 2022

clue approved these changes Jun 25, 2022
View reviewed changes

SimonFrings reviewed Aug 1, 2022
View reviewed changes

README.md Outdated Show resolvedHide resolved
src/Browser.php Outdated Show resolvedHide resolved

WyriHaximus reviewed Aug 10, 2022
View reviewed changes

src/Browser.phpShow resolvedHide resolved

51imyyy force-pushed the 1.x branch 2 times, most recently from f7f9bf0 to 32e0b33Compare August 13, 2022 17:51

SimonFrings requested changes Aug 16, 2022
View reviewed changes

51imyyy force-pushed the 1.x branch from cf9ff39 to 99d458fCompare August 17, 2022 07:39

SimonFrings reviewed Aug 18, 2022
View reviewed changes

README.md Outdated Show resolvedHide resolved
README.md Outdated Show resolvedHide resolved

51imyyy force-pushed the 1.x branch from 99d458f to 43d79f0Compare August 18, 2022 08:35

SimonFrings reviewed Aug 30, 2022
View reviewed changes

README.md Outdated Show resolvedHide resolved
README.md Outdated Show resolvedHide resolved
src/Browser.php Outdated Show resolvedHide resolved
src/Browser.php Outdated Show resolvedHide resolved
src/Browser.php Outdated Show resolvedHide resolved
src/Browser.php Outdated Show resolvedHide resolved

51imyyy force-pushed the 1.x branch 2 times, most recently from 2f7d638 to 443fdf3Compare August 30, 2022 11:09

51imyyy force-pushed the 1.x branch from 443fdf3 to 3e29798Compare September 1, 2022 09:48

SimonFrings reviewed Sep 1, 2022
View reviewed changes

src/Browser.phpShow resolvedHide resolved
src/Browser.phpShow resolvedHide resolved

51imyyy force-pushed the 1.x branch from 0912adc to e2c722bCompare September 1, 2022 09:57

SimonFrings reviewed Sep 1, 2022
View reviewed changes

51imyyy force-pushed the 1.x branch from e2c722b to ff29545Compare September 1, 2022 12:26

SimonFrings approved these changes Sep 1, 2022
View reviewed changes

clue self-requested a review September 1, 2022 14:22

clue reviewed Sep 12, 2022
View reviewed changes

added support for default headers in Browser PHP and moved default he…
0c27d67
…ader user-agent to the default headers.

51imyyy force-pushed the 1.x branch from e60cff3 to 0c27d67Compare September 12, 2022 07:52

clue approved these changes Sep 12, 2022
View reviewed changes

tests/BrowserTest.phpShow resolvedHide resolved

clue requested a review from WyriHaximus September 12, 2022 08:35

clue added this to the v1.8.0 milestone Sep 12, 2022

WyriHaximus approved these changes Sep 14, 2022
View reviewed changes

WyriHaximus merged commit 14e9c6b into reactphp:1.xSep 14, 2022

clue mentioned this pull request Sep 15, 2022
HTTP client: Support passing default request headers #449
Closed

Uh oh!

Support for default request headers#461

Support for default request headers #461

Uh oh!

Conversation

51imyyy commented Jun 24, 2022

Uh oh!

nhedger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

51imyyy commented Jun 24, 2022

Uh oh!

clue left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

clueJun 25, 2022

Choose a reason for hiding this comment

Uh oh!

51imyyyJun 26, 2022

Choose a reason for hiding this comment

Uh oh!

SimonFrings left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

frosty00 commented Aug 3, 2022

Uh oh!

clue commented Aug 5, 2022

Uh oh!

Uh oh!

dinooo13 commented Aug 12, 2022

Uh oh!

frosty00 commented Aug 12, 2022

Uh oh!

51imyyy commented Aug 13, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonFrings left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SimonFringsAug 16, 2022

Choose a reason for hiding this comment

Uh oh!

51imyyyAug 17, 2022

Choose a reason for hiding this comment

Uh oh!

51imyyyAug 17, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SimonFrings left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SimonFrings left a comment

Choose a reason for hiding this comment

51imyyy commented Aug 13, 2022•
edited
Loading

51imyyy commented Sep 1, 2022•
edited
Loading