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

Don't overwrite CURLOPT_HTTP_VERSION option #586

Merged
merged 1 commit into from
Jan 25, 2019

Conversation

ob-stripe
Copy link
Contributor

r? @remi-stripe
cc @stripe/api-libraries

Don't force CURLOPT_HTTP_VERSION to CURL_HTTP_VERSION_2TLS if a value is already set. This allows users to set their own value for CURLOPT_HTTP_VERSION when using custom clients, e.g.:

$client = new \Stripe\HttpClient\CurlClient([CURLOPT_HTTP_VERSION => CURL_HTTP_VERSION_1_1]);
\Stripe\ApiRequestor::setHttpClient($client);

Copy link
Contributor

@remi-stripe remi-stripe left a comment

Choose a reason for hiding this comment

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

That looks good to me but I have some feels:

  • we should update the readme
  • we should likely add the version to the request headers (where we track curl and such) because we should be able to know
  • not entirely sure I follow whether this could be a breaking change
  • we likely need some kind of test? Not fully convinced

Can I ask that you re-assign to brandur or someone else than me? I'm always wary of approving core libs changes

@stripe-ci stripe-ci assigned ob-stripe and unassigned remi-stripe Jan 24, 2019
@ob-stripe
Copy link
Contributor Author

we should update the readme

I think this is already being covered by the "Custom cURL options" section and we don't need to cover every single option. Overriding the HTTP version should rarely be necessary.

we should likely add the version to the request headers (where we track curl and such) because we should be able to know

I don't think this is necessary, we already know which HTTP version is being used. If we want to make it clearer in our own internal logs we can do that without relying on a custom header.

not entirely sure I follow whether this could be a breaking change

It's not. Even if someone is already passing a custom CURLOPT_HTTP_VERSION today and it's being ignored, this wouldn't break anything. (Unless the user is passing an invalid value, but I wouldn't call that a breaking change.)

we likely need some kind of test? Not fully convinced

I meant to write one, but it's actually difficult to test this without some refactoring. I'll give it another try though.

@ob-stripe
Copy link
Contributor Author

Okay, so the only way we can test this right now is by switching the visibility of executeRequestWithRetries from private to protected so that we can mock the method in tests in order to check the contents of $opts. I'd rather not do that though.

@remi-stripe
Copy link
Contributor

r? @brandur-stripe

@brandur-stripe
Copy link
Contributor

LGTM.

@ob-stripe
Copy link
Contributor Author

Thank you both!

@ob-stripe ob-stripe merged commit 260d0e9 into master Jan 25, 2019
@ob-stripe ob-stripe deleted the ob-dont-overwrite-opt-http-version branch January 25, 2019 14:22
@ob-stripe
Copy link
Contributor Author

Released as 6.29.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants