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.

Sign up for GitHub

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

Jump to bottom

[HttpClient] Add an opportunity to use curl BLOB constants certs #48775

Closed
wants to merge 5 commits into from
Closed

[HttpClient] Add an opportunity to use curl BLOB constants certs #48775

wants to merge 5 commits into from

Conversation

voodooism
Copy link
Contributor

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR symfony/symfony-docs#...

It would be good to have an opportunity to pass raw data instead of path in HTTP-client configuration, like so:

$httpClient->scopedClient('client_name')
  ->cafile('raw-data-here, not path')
  ->localCert('raw-data-here, not path')
  ->localPk('raw-data-here, not path');

@carsonbot carsonbot added this to the 6.3 milestone Dec 23, 2022
@stof
Copy link
Member

stof commented Dec 23, 2022

I don't think we should have the same option supporting strings being either filesystem paths or file contents. Guessing the expected signature based on whether the file actually exists is quite fragile (and might even be dangerous)

@voodooism
Copy link
Contributor Author

voodooism commented Dec 23, 2022 •
edited
Loading

I don't think we should have the same option supporting strings being either filesystem paths or file contents. Guessing the expected signature based on whether the file actually exists is quite fragile (and might even be dangerous)

Ok, I agree. To be honest, initially, I wanted to introduce new options, but I spent some time thinking about naming and finished with no good idea :)

What do you think about the next naming: cafileData/ cafileRaw, localCertData/localCertRaw, localPkData/localPkRaw?

Also, I didn't get why the tests failed with the "Undefined constant "CURLOPT_SSLCERT_BLOB". As I see the constant is supported since PHP 8.1.0

@voodooism
[HttpClient] Add an opportunity to use curl BLOB constants for `CAINF…
300e70d 
…O`, `SSLCERT`, `SSLKEY`
@voodooism
[FrameworkBundle] Extend configuration to have an opportunity to set …
9c93ec8 
…`cafile`, `local_cert` and `local_pk` options as raw content instead of path to file
@nicolas-grekas
Copy link
Member

Thanks for the PR.
I'd rather not add options that are specific to some implementation.
Instead, I'd advise using extra.curl options.

@voodooism
Copy link
Contributor Author

Thanks for the PR. I'd rather not add options that are specific to some implementation. Instead, I'd advise using extra.curl options.

But in that case, we need to do it in every request, if I understand correctly.
As libcurl supports it, I think it would be useful to have those options to configure clients in application configs.

@nicolas-grekas
Copy link
Member

It should be possible to configure default "extra" entries. If that's not possible today, this should be improved.

@voodooism
Copy link
Contributor Author

Okay, will do it in a separate PR

@nicolas-grekas
Copy link
Member

Thanks. Note that you can also configure the default options at the injection point, with eg
$this->client = $client->withOptions([...])

@voodooism
Copy link
Contributor Author

Thanks. Note that you can also configure the default options at the injection point, with eg $this->client = $client->withOptions([...])

Yes, it's applicable, but again in that case we need to do it in every class we use those options :(

@voodooism voodooism mentioned this pull request Dec 27, 2022
Merged
@nicolas-grekas
Copy link
Member

Closing in favor of #48797

fabpot added a commit that referenced this pull request Jan 5, 2023
@fabpot
feature #48797 [FrameworkBundle] Add extra attribute for HttpClient…
ed96673 
… Configuration (voodooism)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[FrameworkBundle] Add `extra` attribute for HttpClient Configuration

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| License       | MIT
| Doc PR        | coming soon...<!-- required for new features -->
<!--
Replace this notice by a short README for your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the latest branch.
 - For new features, provide some code snippets to help understand usage.
 - Changelog entry sh
 - ould follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

I want to configure HttpClient once in configure file and forget about the configuration when doing a request or injection. In my particular case, I want to pass certificates to the client as raw values using special curlopts. Here #48775 proposed the best way to do it - using `extra` attribute in configuration.

It's going to look like so
```php
return static function (FrameworkConfig $frameworkConfig): void {
    $httpClient = $frameworkConfig->httpClient();
    $httpClient->defaultOptions([
        'extra' => ['curl' => ['foo' => 'bar']]
    ]);

    $httpClient->scopedClient('some_client')
        ->baseUri('https://some.uri')
        ->header('Accept', 'application/json')
        ->extra(['curl' => ['foo' => 'bar']]);
}
```

Commits
-------

6c89894 [FrameworkBundle] Add `extra` attribute for HttpClient Configuration
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jan 7, 2023
@fabpot
feature #48797 [FrameworkBundle] Add extra attribute for HttpClient…
b06b1c9 
… Configuration (voodooism)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[FrameworkBundle] Add `extra` attribute for HttpClient Configuration

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| License       | MIT
| Doc PR        | coming soon...<!-- required for new features -->
<!--
Replace this notice by a short README for your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the latest branch.
 - For new features, provide some code snippets to help understand usage.
 - Changelog entry sh
 - ould follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

I want to configure HttpClient once in configure file and forget about the configuration when doing a request or injection. In my particular case, I want to pass certificates to the client as raw values using special curlopts. Here symfony/symfony#48775 proposed the best way to do it - using `extra` attribute in configuration.

It's going to look like so
```php
return static function (FrameworkConfig $frameworkConfig): void {
    $httpClient = $frameworkConfig->httpClient();
    $httpClient->defaultOptions([
        'extra' => ['curl' => ['foo' => 'bar']]
    ]);

    $httpClient->scopedClient('some_client')
        ->baseUri('https://some.uri')
        ->header('Accept', 'application/json')
        ->extra(['curl' => ['foo' => 'bar']]);
}
```

Commits
-------

6c898944d6 [FrameworkBundle] Add `extra` attribute for HttpClient Configuration
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jul 28, 2023
@fabpot
feature #48797 [FrameworkBundle] Add extra attribute for HttpClient…
61be045 
… Configuration (voodooism)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[FrameworkBundle] Add `extra` attribute for HttpClient Configuration

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| License       | MIT
| Doc PR        | coming soon...<!-- required for new features -->
<!--
Replace this notice by a short README for your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the latest branch.
 - For new features, provide some code snippets to help understand usage.
 - Changelog entry sh
 - ould follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

I want to configure HttpClient once in configure file and forget about the configuration when doing a request or injection. In my particular case, I want to pass certificates to the client as raw values using special curlopts. Here symfony/symfony#48775 proposed the best way to do it - using `extra` attribute in configuration.

It's going to look like so
```php
return static function (FrameworkConfig $frameworkConfig): void {
    $httpClient = $frameworkConfig->httpClient();
    $httpClient->defaultOptions([
        'extra' => ['curl' => ['foo' => 'bar']]
    ]);

    $httpClient->scopedClient('some_client')
        ->baseUri('https://some.uri')
        ->header('Accept', 'application/json')
        ->extra(['curl' => ['foo' => 'bar']]);
}
```

Commits
-------

6c898944d6 [FrameworkBundle] Add `extra` attribute for HttpClient Configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Feature HttpClient Status: Needs Review
Projects
None yet
Milestone
6.3
Development

Successfully merging this pull request may close these issues.
4 participants
@voodooism @stof @nicolas-grekas @carsonbot