Skip to content

[HttpClient][WebProfilerBundle] Add button to copy a request as a cURL command #43931

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

Merged

Conversation

Deuchnord
Copy link

@Deuchnord Deuchnord commented Nov 4, 2021

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

Adding a button to each request in the HttpClient section to copy it as a curl command that can be then pasted either in the terminal (on Linux and Mac) or in an application that can parse it (like Insomnia).

Work in progress:

  • Make a first PoC of the feature
  • Generate the curl command
  • Added some UX considerations to the button
  • Update the tests

@carsonbot
Copy link

Hey!

To help keep things organized, we don't allow "Draft" pull requests. Could you please click the "ready for review" button or close this PR and open a new one when you are done?

Note that a pull request does not have to be "perfect" or "ready for merge" when you first open it. We just want it to be ready for a first review.

Cheers!

Carsonbot

@Deuchnord Deuchnord marked this pull request as ready for review November 4, 2021 16:05
@carsonbot carsonbot added this to the 6.0 milestone Nov 4, 2021
@carsonbot carsonbot changed the title [Profiler] HttpClient: add button to copy a request to a cURL command [WebProfilerBundle] HttpClient: add button to copy a request to a cURL command Nov 4, 2021
@stof
Copy link
Member

stof commented Nov 4, 2021

  • Make the _cURL' button show a small window that displays the curl command to copy (useful on browsers that do not support navigator.clipboard) along with a button to copy.

The WebProfiler pages are targetting modern browsers only. And all modern browsers support it navigator.clipboard. However, they support it only in secure contexts.
So the cases that would benefit from the displayed text are not older browsers, but non secure contexts.

@derrabus derrabus modified the milestones: 6.0, 6.1 Nov 4, 2021
@Deuchnord Deuchnord force-pushed the profiler-http-client-curl-clipboard-button branch from 8e80365 to 4ba28ab Compare November 5, 2021 16:10
@Deuchnord
Copy link
Author

@stof @nicolas-grekas I'm not really sure of how I should handle the body in the generated curl command when it is a closure or a resource? (see https://symfony.com/doc/current/http_client.html#uploading-data)

@stof
Copy link
Member

stof commented Nov 5, 2021

@Deuchnord for such cases, I would suggest skipping the "copy as curl" button

@Deuchnord Deuchnord force-pushed the profiler-http-client-curl-clipboard-button branch 9 times, most recently from c2e3fff to d09175a Compare November 9, 2021 10:12
@Deuchnord Deuchnord force-pushed the profiler-http-client-curl-clipboard-button branch 2 times, most recently from b2fafdd to 0a3b52e Compare December 5, 2021 15:41
@Deuchnord Deuchnord changed the title [HttpClient][WebProfilerBundle] HttpClient: add button to copy a request to a cURL command [HttpClient][WebProfilerBundle] Add button to copy a request to a cURL command Dec 14, 2021
@Deuchnord Deuchnord force-pushed the profiler-http-client-curl-clipboard-button branch from 0a3b52e to 34ef21e Compare February 21, 2022 07:59
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I'm not sure the current logic works when there are redirects involved.
Can you add a test case that hits http://symfony.com/releases.json to trigger one?
The curl command-line should only contain the first call of course.

@Deuchnord
Copy link
Author

I'm not sure the current logic works when there are redirects involved. Can you add a test case that hits http://symfony.com/releases.json to trigger one? The curl command-line should only contain the first call of course.

Looks like $trace['info']['url'] only contains the last URL of the redirection chain. Does it make sense it for you if the generated cURL command gives it directly?

@Deuchnord Deuchnord force-pushed the profiler-http-client-curl-clipboard-button branch from 812cb6c to 0b76a18 Compare March 4, 2022 15:24
@Deuchnord Deuchnord changed the title [HttpClient][WebProfilerBundle] Add button to copy a request to a cURL command [HttpClient][WebProfilerBundle] Add button to copy a request as a cURL command Mar 4, 2022
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 4, 2022

Looks like $trace['info']['url'] only contains the last URL of the redirection chain

indeed, but why don't we use $trace['url'] instead?

@Deuchnord
Copy link
Author

Because I didn't spot it until now, thanks for pointing it 😅
It's fixed :)

@nicolas-grekas
Copy link
Member

There's a failure on appveyor :)

@Deuchnord
Copy link
Author

I'm not sure to understand the error given by the tests on AppVeyor:

1) Symfony\Component\HttpClient\Tests\DataCollector\HttpClientDataCollectorTest::testItGeneratesCurlCommandsAsExpected
Symfony\Component\HttpClient\Exception\TransportException: fopen(): Unable to find the wrapper "https" - did you forget to enable it when you configured PHP?

If I read the config file correctly, the php_openssl.dll is already activated, isn't it?

@nicolas-grekas
Copy link
Member

openssl is first disabled then re-enabled, to ensure that the test suite doesn't rely on implicit deps, including ext-openssl.

@Deuchnord Deuchnord force-pushed the profiler-http-client-curl-clipboard-button branch from 71e4d65 to d7adc1f Compare March 8, 2022 15:17
@fabpot fabpot force-pushed the profiler-http-client-curl-clipboard-button branch from d7adc1f to 67c9146 Compare March 9, 2022 07:18
@fabpot
Copy link
Member

fabpot commented Mar 9, 2022

Thank you @Deuchnord.

@fabpot fabpot merged commit 194e274 into symfony:6.1 Mar 9, 2022
@Deuchnord Deuchnord deleted the profiler-http-client-curl-clipboard-button branch March 9, 2022 08:20
@fabpot fabpot mentioned this pull request Apr 15, 2022
@sashaaro
Copy link

can we collect class request objects and use https://github.com/php-http/message/blob/master/src/Formatter/CurlCommandFormatter.php for it? it can make service usable and universal

@stof
Copy link
Member

stof commented May 13, 2022

There is no class request object in symfony/http-client

nicolas-grekas added a commit that referenced this pull request Jun 9, 2022
… body for c… (Phillip Look)

This PR was merged into the 6.1 branch.

Discussion
----------

[HttpClient][WebProfilerBundle] Catch errors when encoding body for c…

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

In Symfony 6.1 a [button to copy a request as a cURL command](#43931) was introduced for the profiler.

But if I post a binary file containing null characters using the curl-http-client the `HttpClientDataCollector` throws an error.
```
Warning: Uncaught ValueError: escapeshellarg(): Argument #1 ($arg) must not contain any null bytes
```

My solution is to catch the `ValueError` in this situation and to return `null` as the resulting curl command. Returning `null` seems to be the standard handling for unexpectad values in this data collector.

Commits
-------

36e6fa0 [HttpClient][WebProfilerBundle] Catch errors when encoding body for curl command line
symfony-splitter pushed a commit to symfony/http-client that referenced this pull request Jun 9, 2022
… body for c… (Phillip Look)

This PR was merged into the 6.1 branch.

Discussion
----------

[HttpClient][WebProfilerBundle] Catch errors when encoding body for c…

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

In Symfony 6.1 a [button to copy a request as a cURL command](symfony/symfony#43931) was introduced for the profiler.

But if I post a binary file containing null characters using the curl-http-client the `HttpClientDataCollector` throws an error.
```
Warning: Uncaught ValueError: escapeshellarg(): Argument #1 ($arg) must not contain any null bytes
```

My solution is to catch the `ValueError` in this situation and to return `null` as the resulting curl command. Returning `null` seems to be the standard handling for unexpectad values in this data collector.

Commits
-------

36e6fa0935 [HttpClient][WebProfilerBundle] Catch errors when encoding body for curl command line
sadafrangian3 pushed a commit to sadafrangian3/Dependency-Injection-http-client that referenced this pull request Nov 2, 2022
… body for c… (Phillip Look)

This PR was merged into the 6.1 branch.

Discussion
----------

[HttpClient][WebProfilerBundle] Catch errors when encoding body for c…

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

In Symfony 6.1 a [button to copy a request as a cURL command](symfony/symfony#43931) was introduced for the profiler.

But if I post a binary file containing null characters using the curl-http-client the `HttpClientDataCollector` throws an error.
```
Warning: Uncaught ValueError: escapeshellarg(): Argument #1 ($arg) must not contain any null bytes
```

My solution is to catch the `ValueError` in this situation and to return `null` as the resulting curl command. Returning `null` seems to be the standard handling for unexpectad values in this data collector.

Commits
-------

36e6fa0935 [HttpClient][WebProfilerBundle] Catch errors when encoding body for curl command line
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.

8 participants