Support for custom HTTP headers

[qharnay]

Hello, and thanks for your work on EventSourcing from backend with Promises !

I'd like to use this lib. to set up a backend subscriber, but there was a lack of support with custom headers, preventing me to subscribe to my SSE server (I have to provide Authorization Bearers header into HTTP requests).

This PR enables custom headers to be sent into HTTP headers of the EventSource's request.

Tests have been updated accordingly & I'm currently using it in my dev. stack.

Cheers,
Quentin H.

[clue]

@qharnay Thanks for filing this PR, this is an interesting one! 👍

I'm curious, what is your use case for providing custom HTTP request headers? It's my understanding this isn't possible in the browser-based API, so I wonder how much value it could provide in this implementation.

From a technical perspective, this mostly LGTM, perhaps we could get rid of the $defaultHeaders attribute and merge it with the static defaults in $headers instead. We should also add some additional tests for overwriting existing headers (Accept, Authorization etc.)

What do you think? 👍

[qharnay]

@clue , thanks for reviewing this one.

what is your use case for providing custom HTTP request headers:

I'm using mercure ( https://mercure.rocks ) for a SSE service, on a multi domain app, with private updates, requiring auth. As a multi-domain app, I could not use cookie-based auth and have to use custom HTTP header '"authorization"

It's my understanding this isn't possible in the browser-based API

This is totally possible with event-source polyfill : https://www.npmjs.com/package/event-source-polyfill
Which is really widespread on SSE community to ensure browser compliance.

perhaps we could get rid of the $defaultHeaders attribute and merge it with the static defaults in $headers instead

This property was set to ease tests. but this is not a strong requirement

We should also add some additional tests for overwriting existing headers (Accept, Authorization etc.)

This could be a missing case, you're totally right.

I'll push a new commit ensuring default headers override works.

Thank you @clue !

[qharnay]

I pushed a tiny test change, ensuring default headers are not overriden . In fact the default headers are standard for the SSE and should not be overridden.

Is it enough for you to ensure custom HTTP header ability ?

cheers !

[qharnay]

@clue , the CI seems quite broken ... ( PHP 5.5 error has nothing to do with this PR )

[clue]

@clue , the CI seems quite broken ... ( PHP 5.5 error has nothing to do with this PR )

Restart and fixed the temporary hiccup 👍

I pushed a tiny test change, ensuring default headers are not overriden . In fact the default headers are standard for the SSE and should not be overridden.

Is it enough for you to ensure custom HTTP header ability ?

I agree with your reasoning for needing custom HTTP header support. Let's work out some nasty details: What is supposed to happen if I try to overwrite the Accept header, what happens if I use alternative casing like aCCept? IMO what's most important is that this should be consistent, i.e. either accepting or rejecting all forms.

Perhaps we can take a look at how other implementations handle this. What do you think? 👍

[qharnay]

Let's work out some nasty details: What is supposed to happen if I try to overwrite the Accept header, what happens if I use alternative casing like aCCept?

According to HTTP headers RFC (both for 1.1 and 2): HTTP headers are case insensitive.
So, ok, overwriting could lead to several headers with different cases, handled the same way on web server's side

IMO what's most important is that this should be consistent, i.e. either accepting or rejecting all forms.

"Deal": I'll add some mechanism to ensure default headers consistency. "See U with the thext commit"

Perhaps we can take a look at how other implementations handle this. What do you think?

From what I've seen, implementations are really tolerant on headers provided by "users" ( they are mostly key-value or associative array handlers ).
This makes senses: the developper wants to deals with custom http headers, he'd better know what he is doing and how to debug.

Tha's no big deal, I'm pleased with the idea of ensuring default headers can not be touched for this implementation.

[qharnay]

I finally came up with the (I think) final version for adding custom HTTP headers / preserving default ones / testing the whole process. Tell me what you think about it @clue.

[If you're okay, I'll come up with another PR for initializing the Last-Event-Id afterthat ;-) ]

[bpolaszek]

Dunno how I can add commits to a PR which isn't mine (I probably can't), but here's the rebased version: https://github.com/bpolaszek/reactphp-eventsource/tree/customHeaders

Should I make another PR? Or @qharnay Can you check this out and push again so that this can be moved forward?
Thank you ☺️

[SimonFrings]

@qharnay and @bpolaszek thanks for looking into this, your changes are definitely an improvement to this project 👍

I discussed this topic with @clue.
I'm currently working on the constructor of the EventSource class to move the loop parameter to the end of the parameter list so you don't have to write null every time you want to use a custom Browser.

There are a few more projects supporting custom HTTP headers and they're also using the React/Http/Browser class. Wouldn't it make more sense to implement this into the ReactPHP HTTP component instead?

[bpolaszek]

@qharnay and @bpolaszek thanks for looking into this, your changes are definitely an improvement to this project 👍

I discussed this topic with @clue. I'm currently working on the constructor of the EventSource class to move the loop parameter to the end of the parameter list so you don't have to write null every time you want to use a custom Browser.

There are a few more projects supporting custom HTTP headers and they're also using the React/Http/Browser class. Wouldn't it make more sense to implement this into the ReactPHP HTTP component instead?

You mean, by instantiating a React/Http/Browser by ourselves, with it already having custom headers? I'm not familiar with that one, but If it's already possible, it would make more sense indeed.

[clue]

Here's what this could potentially look like:

$browser = new React\Http\Browser();
$browser = $browser->withProtocolVersion('1.1'); // already supported
$browser = $browser->withHeader('User-Agent', 'ACME'); // could be useful?

$es = new Clue\React\EventSource\EventSource('https://example.com/stream', $browser);

This isn't currently supported, but seems reasonable and would allow far more packages to take advantage of custom HTTP request headers. What do you think about this?

[ellisonpatterson]

Here's what this could potentially look like:

$browser = new React\Http\Browser();
$browser = $browser->withProtocolVersion('1.1'); // already supported
$browser = $browser->withHeader('User-Agent', 'ACME'); // could be useful?

$es = new Clue\React\EventSource\EventSource('https://example.com/stream', $browser);

This isn't currently supported, but seems reasonable and would allow far more packages to take advantage of custom HTTP request headers. What do you think about this?

Would that potentially allow for setting headers that are persistent across all requests made with that browser instance?

[SimonFrings]

@ellisonpatterson Yes!

[clue]

I've just filed reactphp/http#449 as a way to keep track of this ticket upstream and believe this is the best way to address this 👍

In the meantime, I'll assume this one is resolved and will close this for now, please feel free to report back otherwise 👍