[HttpClient] Enable using EventSourceHttpClient::connect() for both GET and POST #51558
Conversation
|
Hey! I see that this is your first PR. That is great! Welcome! Symfony has a contribution guide which I suggest you to rea 10000 d. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
|
This looks like a new feature to me, especially since we need to change a method signature. Also, please note that we usually don't merge changes that are not covered by tests. |
|
What you say makes sense. Technically it's indeed not a bug. At the same time, was not sure it really deserved being called a feature. |
|
@wivaku we still need tests to cover the new feature you are adding (which is here to prevent regressing on it which would break it). and anything that is not a bug fix is treated as a new feature going into the next minor version. |
|
What both of you say makes sense. Technically it's indeed not a bug. At the same time, was not sure it really deserved being called a feature. POST was already allowed (using As the change maintains backwards compatibility (before: always GET, after: optional argument that defaults to GET) and My knowledge of the internals of the code is not sufficient to add one though. |
|
@wivaku And the rule is that anything that is not a bug fix goes into the next minor version. It is not about deserving being called a feature or no.
The new test is not there to ensure this PR maintains backward compatibility (that's what the existing test ensure). It is there to ensure that *future PRs do not break the change you introduce in this one.
Look at the existing tests of the EventSourceHttpClient. The test for POST will be quite similar except that the mock will expect being called with a POST method. |
|
Ok, attempted to add test. Similar to the GET test, but only checking the response, not the responseStream as this is already covered by the GET test. All this, including Mockery tests are quite new to me, so apologies if I'm doing something wrong. Because of my lack of experience the way I read the Mockery test is confusing to me: in the test I specify that the response will be for the method and then I test if the response I get is indeed that response. |
src/Symfony/Component/HttpClient/Tests/EventSourceHttpClientTest.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/HttpClient/Tests/EventSourceHttpClientTest.php
Outdated
Show resolved
Hide resolved
|
Thank you @wivaku. |
Fix so connect() can be used for SSE connections that require POST, e.g. GraphQL SSE subscriptions. This so we don't have to manually create the request() and include all of the options that are used for connect().