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

Support for default request headers #461

Merged
merged 1 commit into from Sep 14, 2022
Merged

Conversation

51imyyy
Copy link
Contributor

@51imyyy 51imyyy commented Jun 24, 2022

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

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

Copy link
Contributor

@nhedger nhedger left a 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 :)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
tests/BrowserTest.php Outdated Show resolved Hide resolved
tests/BrowserTest.php Outdated Show resolved Hide resolved
@51imyyy
Copy link
Contributor Author

51imyyy commented Jun 24, 2022

Thank you @nhedger for your help :)

@clue clue changed the title [FEATURE] | Support for default headers Support for default request headers Jun 25, 2022
Copy link
Member

@clue clue left a 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! 👍

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated
private function findCaseInsensitivHeader($headers, $header)
{
$found = null;
/** @var string $key */
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

@SimonFrings SimonFrings left a 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?

README.md Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
@frosty00
Copy link

frosty00 commented Aug 3, 2022

I am getting banned from cloudflare for having the default user agent in this library. Is it possible to remove that header from here -

'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
Copy link
Member

clue commented Aug 5, 2022

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

@dinooo13
Copy link
Contributor

How about removing the default User Agent from here:

'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
Copy link

How about removing the default User Agent from here:

'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
Copy link
Contributor Author

51imyyy commented Aug 13, 2022

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

Copy link
Member

@SimonFrings SimonFrings left a 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 Show resolved Hide resolved
src/Browser.php Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Browser.php Outdated
Comment on lines 832 to 856
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;
}
}
}
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

src/Browser.php Outdated Show resolved Hide resolved
Copy link
Member

@SimonFrings SimonFrings left a comment

Choose a reason for hiding this comment

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

Just two nits

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@SimonFrings SimonFrings left a 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.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
@51imyyy
Copy link
Contributor Author

51imyyy commented Aug 30, 2022

applied the suggestions.

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

@SimonFrings
Copy link
Member

@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
Copy link
Contributor Author

51imyyy commented Sep 1, 2022

@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
Copy link
Member

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
Copy link
Contributor Author

51imyyy commented Sep 1, 2022

@SimonFrings Changed it

Copy link
Member

@SimonFrings SimonFrings left a 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.

src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
@51imyyy
Copy link
Contributor Author

51imyyy commented Sep 1, 2022

appiled the suggestions.

Copy link
Member

@SimonFrings SimonFrings left a 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
Comment on lines 2389 to 2391
add a request header for all following requests.
```php
Copy link
Member

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)

Copy link
Contributor Author

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

Copy link
Member

@SimonFrings SimonFrings left a 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 👍

@clue clue self-requested a review September 1, 2022 14:22
@51imyyy
Copy link
Contributor Author

51imyyy commented Sep 1, 2022

Thank you :)

Copy link
Member

@clue clue left a 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! :shipit:

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
src/Browser.php Outdated Show resolved Hide resolved
tests/BrowserTest.php Outdated Show resolved Hide resolved
tests/BrowserTest.php Show resolved Hide resolved
tests/BrowserTest.php Outdated Show resolved Hide resolved
tests/BrowserTest.php Outdated Show resolved Hide resolved
src/Browser.php Show resolved Hide resolved
Copy link
Member

@clue clue left a 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! :shipit: Keep it up! 👍

tests/BrowserTest.php Show resolved Hide resolved
@clue clue added this to the v1.8.0 milestone Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants