-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[HttpClient] Add resolve to copy as Curl #45729
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
[HttpClient] Add resolve to copy as Curl #45729
Conversation
db78877
to
d501e57
Compare
Hey! I think @Deuchnord has recently worked with this code. Maybe they can help review this? Cheers! Carsonbot |
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.
Nice addition. Thank you for the new test cases.
src/Symfony/Component/HttpClient/DataCollector/HttpClientDataCollector.php
Outdated
Show resolved
Hide resolved
d501e57
to
88b1b8f
Compare
@@ -199,19 +298,19 @@ public function testItDoesNotFollowRedirectionsWhenGeneratingCurlCommands() | |||
{ | |||
$sut = new HttpClientDataCollector(); | |||
$sut->registerClient('http_client', $this->httpClientThatHasTracedRequests([ | |||
[ | |||
'method' => 'GET', | |||
'url' => 'http://symfony.com/releases.json', |
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.
This case don't need to be updated.
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.
@GromNaN I changed it because the test was making a request to symfony.com, so I though it'd be better if it didn't rely on a 3rd party service. I can revert it if you want.
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 for the records, this test's purpose was to check that the redirections are not followed (in this case http → https), so the returned cURL command corresponds exactly to the request.
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.
@Deuchnord Tnx for the info, I totally missed that. I've updated the test to go to a url that redirects.
--compressed \\ | ||
--request GET \\ | ||
--url %1$shttps://symfony.com/releases.json%1$s \\ |
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.
You didn't keep a case using the default port.
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.
@GromNaN Not sure what to do about this one, I could revert to symfony.com, but as mentioned, I was hoping to avoid making requests to 3rd party services.
b6e9694
to
b0a57a3
Compare
b0a57a3
to
5f2d584
Compare
Thank you @HypeMC. |
…nd when files are uploaded (MatTheCat) This PR was merged into the 6.3 branch. Discussion ---------- [HttpClient][WebProfilerBundle] Do not generate cURL command when files are uploaded | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | Fix #51366 | License | MIT I also removed ``@requires` extension openssl` annotations since that does not seem to be the case since #45729. Failures in AppVeyor occur because double quotes are missing around `--data-raw` values. Possibly related to #52429. Commits ------- 4503f94 [HttpClient][WebProfilerBundle] Do not generate cURL command when files are uploaded
Adds the resolve option to the generated Curl command.