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

[HttpClient] Add resolve to copy as Curl #45729

Merged
merged 1 commit into from Mar 17, 2022

Conversation

HypeMC
Copy link
Contributor

@HypeMC HypeMC commented Mar 13, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

Adds the resolve option to the generated Curl command.

@carsonbot
Copy link

Hey!

I think @Deuchnord has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Copy link
Member

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

@@ -199,19 +298,19 @@ public function testItDoesNotFollowRedirectionsWhenGeneratingCurlCommands()
{
$sut = new HttpClientDataCollector();
$sut->registerClient('http_client', $this->httpClientThatHasTracedRequests([
[
'method' => 'GET',
'url' => 'http://symfony.com/releases.json',
Copy link
Member

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.

Copy link
Contributor Author

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.

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.

Copy link
Contributor Author

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

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.

Copy link
Contributor Author

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.

@HypeMC HypeMC force-pushed the add-resolve-to-copy-as-curl branch 2 times, most recently from b6e9694 to b0a57a3 Compare March 16, 2022 10:25
@nicolas-grekas
Copy link
Member

Thank you @HypeMC.

@nicolas-grekas nicolas-grekas merged commit 9a29631 into symfony:6.1 Mar 17, 2022
@HypeMC HypeMC deleted the add-resolve-to-copy-as-curl branch March 17, 2022 15:56
nicolas-grekas added a commit that referenced this pull request Nov 6, 2023
…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
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

5 participants