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

Http adapter curl missing delete method #25742

Merged
merged 4 commits into from
Feb 9, 2020
Merged

Http adapter curl missing delete method #25742

merged 4 commits into from
Feb 9, 2020

Conversation

jimuld
Copy link
Contributor

@jimuld jimuld commented Nov 26, 2019

Description (*)

Http Adapter Curl missing support for REST API call Method Delete, added delete option in that so the developer can use Curl adapter for delete method api call.

Fixed Issues (if relevant)

  1. magento/magento2#<issue_number>: Issue title

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

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 are green)

@m2-assistant
Copy link

m2-assistant bot commented Nov 26, 2019

Hi @jimuld. 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 give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@pmclain pmclain self-assigned this Nov 26, 2019
pmclain
pmclain suggested changes Nov 26, 2019
View reviewed changes
Copy link
Contributor

@pmclain pmclain left a comment

Choose a reason for hiding this comment

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

@jimuld Thanks for the contribution. Please remember to take a look at the Adobe CLA.

lib/internal/Magento/Framework/HTTP/Adapter/Curl.php Outdated Show resolved Hide resolved
lib/internal/Magento/Framework/HTTP/Adapter/Curl.php Outdated Show resolved Hide resolved
@sidolov sidolov changed the base branch from 2.3-develop to 2.4-develop December 5, 2019 17:12
@ihor-sviziev ihor-sviziev self-assigned this Jan 23, 2020
ihor-sviziev
ihor-sviziev requested changes Jan 23, 2020
View reviewed changes
Copy link
Contributor

@ihor-sviziev ihor-sviziev left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @jimuld,

Thank you for your contribution! I see that processing your PR is stuck, really sorry for that.
I'll try like to move it forward.

Could you accept Adobe CLA, so we'll be able to process your PR?

@ihor-sviziev
Copy link
Contributor

@magento run all tests

@ihor-sviziev ihor-sviziev mentioned this pull request Jan 23, 2020
Merged
@jimuld
Copy link
Contributor Author

jimuld commented Jan 24, 2020 •
edited
Loading

I have already signed CLA twice. Not sure why it keeps asking for the same. Is it required to sign for every pull request? Signed it once agai.

jimuld and others added 2 commits January 24, 2020 10:33
@jimuld @pmclain
Update lib/internal/Magento/Framework/HTTP/Adapter/Curl.php
a409774 
Co-Authored-By: Patrick McLain <pat@pmclain.com>
@ihor-sviziev
Http adapter curl missing delete method
7832925 
Fix static tests & use strict comparison
@ihor-sviziev ihor-sviziev added Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests and removed Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Jan 24, 2020
@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Jan 24, 2020 •
edited
Loading

Looks like covering this case with tests would be really hard, as it uses native php functions. For now I put label tests not required.

@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-6682 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

@jimuld thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@engcom-Echo engcom-Echo self-assigned this Jan 31, 2020
@engcom-Alfa engcom-Alfa self-assigned this Jan 31, 2020
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

magento-engcom-team pushed a commit that referenced this pull request Feb 9, 2020
@lenaorobei
ENGCOM-6682: Http adapter curl missing delete method #25742
e6941d3
@magento-engcom-team magento-engcom-team merged commit 7832925 into magento:2.4-develop Feb 9, 2020
@m2-assistant
Copy link

m2-assistant bot commented Feb 9, 2020

Hi @jimuld, 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.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ihor-sviziev ihor-sviziev ihor-sviziev approved these changes

@pmclain pmclain pmclain requested changes

@buskamuza buskamuza Awaiting requested review from buskamuza

Assignees

@ihor-sviziev ihor-sviziev

@pmclain pmclain

@engcom-Alfa engcom-Alfa

@engcom-Echo engcom-Echo

Labels
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests Award: bug fix Component: HTTP Progress: accept Release Line: 2.4
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jimuld @ihor-sviziev @magento-engcom-team @engcom-Alfa @pmclain @sidolov @engcom-Echo @jimulrcloudlabs