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

[Backport] Curl - allow setting HTTP version #21251

Closed
wants to merge 1 commit into from
Closed

[Backport] Curl - allow setting HTTP version #21251

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Feb 15, 2019

Original Pull Request

#19845

Description

Added allow for setting curl http version

Fixed Issues (if relevant)

  1. Magento/Framework/HTTP/Adapter/Curl.php doesn't allow setting HTTP version #19442: Magento/Framework/HTTP/Adapter/Curl.php doesn't allow setting HTTP version

Manual testing scenarios (*)

  1. Create new ZendClient $client
  2. Set HTTP version $client->setConfig(['httpversion' => CURL_HTTP_VERSION_1_1])
  3. Make a request $client->request()

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @SikailoISM. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.2-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@magento-engcom-team magento-engcom-team added Component: Framework/HTTP Release Line: 2.2 Partner: ISM eCompany Pull Request is created by partner ISM eCompany partners-contribution Pull Request is created by Magento Partner labels Feb 15, 2019
@ghost ghost mentioned this pull request Feb 15, 2019
4 tasks
@osrecio osrecio self-assigned this Feb 15, 2019
@osrecio osrecio self-requested a review February 15, 2019 07:40
osrecio
osrecio previously approved these changes Feb 15, 2019
Copy link
Member

@osrecio osrecio left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @SikailoISM .

We will process your PR and we will merge soon.

@magento-engcom-team
Copy link
Contributor

Hi @osrecio, thank you for the review.
ENGCOM-4274 has been created to process this Pull Request

@davidalger
Copy link
Member

This PR should not be merged as the parent PR that was previously merged into 2.3-develop was flawed and not thoroughly tested, and the change it introduced reverted in commit 55b8bf8. The reason is the addition of the line of code in this PR is assuming the $http_ver argument is expecting the value of the curl constants when it is actually expecting the value of the constants on the Zend libraries. The $http_ver argument has a default value of 1.1 and as we can see, this does not line up with what the curl constants that are defined as:

define ('CURL_HTTP_VERSION_1_0', 1);
define ('CURL_HTTP_VERSION_1_1', 2);
define ('CURL_HTTP_VERSION_2_0', 3);

I have submitted a PR to 2.3-develop with a proper fix for this underlying problem. See PR #21549. I'll be submitting a back port for it shortly.

@rogyar
Copy link
Contributor

rogyar commented Mar 3, 2019

Putting it on hold since the original change has been reverted (55b8bf8).

We have another PR with the modified implementation that seems to be working #21549

@ghost
Copy link
Author

ghost commented Mar 4, 2019

gg

@ghost ghost closed this Mar 4, 2019
@ghost
Copy link

ghost commented Mar 4, 2019

Hi @SikailoISM, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@ghost ghost deleted the 2.2-curl branch March 4, 2019 07:09
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Framework/HTTP Partner: ISM eCompany Pull Request is created by partner ISM eCompany partners-contribution Pull Request is created by Magento Partner Progress: on hold Release Line: 2.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants