Add missing REST methods to Curl Client (SwiftOtter's SOP-348) #39330

lbajsarowicz · 2024-11-05T12:57:14Z

Description (*)

There are 9 methods in the HTTP protocol (https://en.wikipedia.org/wiki/HTTP).
Magento implements just 2 of them, so when you need to make requests to remote environments using Framework HTTP Client, you end up with HTTP 411 error.

This Pull Request adds missing methods with their payload support:

PUT
PATCH
DELETE

Related Pull Requests

Fixed Issues (if relevant)

Fixes magento/magento2#<issue_number>

Manual testing scenarios (*)

...
...

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)
README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
All automated tests passed successfully (all builds are green)

Resolved issues:

resolves [Issue] Add missing REST methods to Curl Client (SwiftOtter's SOP-348) #39341: Add missing REST methods to Curl Client (SwiftOtter's SOP-348)

m2-assistant · 2024-11-05T12:57:18Z

Hi @lbajsarowicz. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

lbajsarowicz · 2024-11-05T13:02:57Z

@magento run all tests

michalbiarda

I added a few comments with possible improvements. Also, I think you forgot to add CONNECT method.

michalbiarda · 2024-11-05T13:24:26Z

lib/internal/Magento/Framework/HTTP/Client/Curl.php

+     * @param string $uri
+     * @return void
+     */
+    public function delete($uri)


According to HTTP documentation, DELETE request can include optional payload.

michalbiarda · 2024-11-05T13:24:43Z

lib/internal/Magento/Framework/HTTP/Client/Curl.php

+     * @param string $uri
+     * @return void
+     */
+    public function options($uri)


According to HTTP documentation, OPTIONS request can include optional payload.

michalbiarda · 2024-11-05T13:25:04Z

lib/internal/Magento/Framework/HTTP/Client/Curl.php

+     * @param string $uri
+     * @return void
+     */
+    public function head($uri)


According to HTTP documentation, HEAD request can include optional payload.

lbajsarowicz · 2024-11-05T13:27:42Z

@ihor-sviziev or @bgorski Can I ask you for Code Review in this specific case?

engcom-Hotel · 2024-11-07T13:37:20Z

@magento create issue

engcom-Hotel · 2024-11-18T06:16:50Z

@magento run all tests

engcom-Hotel

Hello @lbajsarowicz,

Thanks for the contribution!

It seems this PR has been already reviewed by @michalbiarda. We request you to please address them and also please fix the failed automated tests.

Thanks

engcom-Dash · 2024-11-22T06:53:54Z

@magento run all tests

…d CONNECT method

engcom-Dash · 2024-11-22T11:05:02Z

I added a few comments with possible improvements. Also, I think you forgot to add CONNECT method.

@michalbiarda @engcom-Hotel: I have added an optional payload for the DELETE, OPTIONS, and HEAD methods. Additionally, I added the CONNECT method. Please review it. Thanks.

cc: @lbajsarowicz

engcom-Dash · 2024-11-22T11:05:27Z

@magento run all tests

engcom-Hotel · 2024-11-25T06:49:56Z

@magento run Functional Tests B2B, Functional Tests CE

engcom-Hotel

Hello @lbajsarowicz @engcom-Dash,

Please refer to the below review comments.

Thanks

engcom-Hotel · 2024-11-25T07:52:34Z

dev/tests/integration/framework/Magento/TestFramework/Helper/Amqp.php

 declare(strict_types=1);

 namespace Magento\TestFramework\Helper;

+use Magento\Framework\HTTP\Client\Curl;


In line no. 53:

$this->curl = new Curl();

I suggest you add the parameter $curl as null and use ObjectManager to initialize it. Please refer to the $this->deploymentConfig

engcom-Hotel · 2024-11-25T08:15:47Z

dev/tests/integration/framework/Magento/TestFramework/Helper/Curl.php

@@ -1,30 +0,0 @@
-<?php


It is not recommended to delete the existing file, as other third-party extensions might use it. Instead, mark it as deprecated.

Thanks

…d tag

lbajsarowicz · 2024-11-28T10:50:36Z

lib/internal/Magento/Framework/HTTP/Client/Curl.php

+     * @param array|string $params
+     * @return void
+     */
+    public function head(string $uri, array|string $params = [])


I did not introduce a payload here because according to RFC9110

The HEAD method is identical to GET except that the server MUST NOT send content in the response.

https://www.rfc-editor.org/rfc/rfc9110.html#section-9.3.2

You're absolutely correct that the RFC you linked doesn't explicitly forbid a payload for HEAD requests. However, it strongly discourages it and hints at the reasons why it's generally not used:

Semantic Ambiguity:

HEAD is for metadata: The core purpose of HEAD is to fetch metadata about a resource without retrieving the actual data. A payload in this context has no clearly defined meaning. What should the server do with the provided data? Unlike POST (create/update) or PUT (replace), HEAD doesn't have an inherent action associated with a payload.

Potential for Confusion: Including a payload with HEAD could lead to confusion and unpredictable behavior, as servers might interpret it in different ways. This ambiguity undermines the standardization that HTTP strives for.

Why GET can have a payload (though discouraged):

While GET requests can technically have a payload, it's generally discouraged for similar reasons to HEAD. The RFC states that a payload in a GET request "has no semantic meaning to the request." This means the server is free to ignore it.

Hello @lbajsarowicz,

Thanks for the detailed explanation.

FYI, as mentioned in #39330 (review), I have added an optional payload for the DELETE, OPTIONS, and HEAD methods. Additionally, I have added the CONNECT method with an optional payload. If possible, could you please revert these changes, or let me know which methods, including HEAD, do not require an optional payload? Your input would be greatly appreciated.

Thanks again!

lbajsarowicz · 2024-12-02T10:27:14Z

Regarding CONNECT

The CONNECT method requests that the recipient establish a tunnel to the destination origin server identified by the request-target and, if successful, that the recipient send a 2xx (Successful) response. The request MUST NOT include a message body.

https://www.rfc-editor.org/rfc/rfc9110.html#section-9.3.6

lbajsarowicz · 2024-12-02T10:29:16Z

@magento run all tests

engcom-Dash · 2024-12-02T13:54:25Z

Hi @lbajsarowicz,

Thank you for your contribution!

We are currently in discussions with the Product Owner regarding this PR, as the issue has been tagged as a feature request in #39341 (comment). Once we receive input, we will review it. In the meantime, we are moving it to "On Hold."

Thank you!

engcom-Dash · 2024-12-12T08:03:29Z

Hello @lbajsarowicz,

We have received a response from the PO, indicating that this class is used in many extensions, and the changes provided in the pull request are backward incompatible, which is considered a breaking change. This could disrupt extensions.

Given these implications, it seems best not to proceed with this PR.

However, we sincerely appreciate your valuable contributions.

Thanks

lbajsarowicz · 2024-12-12T08:41:35Z

@engcom-Dash are you able to provide more information what is considered as backwards-incompatible?

jakwinkler · 2024-12-12T08:56:40Z

@engcom-Dash these changes are backwards-compatible

lbajsarowicz · 2024-12-15T23:23:07Z

I disagree with your decision and - as a response to it - I created two separate PRs to address the complain about backwards compatibility.

See referenced PRs.

SwiftOtter-SOP-348 Add missing REST methods to Curl Client

302f9b5

lbajsarowicz added 2 commits November 5, 2024 14:00

SwiftOtter-SOP-348 Add missing REST methods: TRACE, HEAD, OPTIONS

e3668e8

SwiftOtter-SOP-348 Replace redundant override

064aa04

lbajsarowicz changed the title ~~SwiftOtter-SOP-348 Add missing REST methods to Curl Client~~ Add missing REST methods to Curl Client (SwiftOtter's SOP-348) Nov 5, 2024

m2-community-project bot added the Progress: pending review label Nov 5, 2024

michalbiarda suggested changes Nov 5, 2024

View reviewed changes

m2-community-project bot added Progress: needs update and removed Progress: pending review labels Nov 5, 2024

SwiftOtter-SOP-348 Resolve issues with Static checks

0ec2621

engcom-Hotel added Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it Priority: P3 May be fixed according to the position in the backlog. labels Nov 7, 2024

m2-community-project bot added Progress: pending review and removed Progress: needs update labels Nov 7, 2024

m2-assistant bot mentioned this pull request Nov 7, 2024

[Issue] Add missing REST methods to Curl Client (SwiftOtter's SOP-348) #39341

Open

5 tasks

Priyakshic added the Project: Community Picked PRs upvoted by the community label Nov 13, 2024

Merge branch '2.4-develop' into swiftotter-SOP-348-curl-client

2da6f81

engcom-Hotel self-requested a review November 18, 2024 08:18

engcom-Hotel requested changes Nov 18, 2024

View reviewed changes

m2-community-project bot assigned engcom-Hotel Nov 18, 2024

m2-community-project bot added Progress: needs update and removed Progress: pending review labels Nov 18, 2024

engcom-Dash self-assigned this Nov 22, 2024

Merge branch '2.4-develop' into swiftotter-SOP-348-curl-client

d16233e

39330: Add optional payload support for DELETE, OPTIONS, HEAD, and ad…

4297a88

…d CONNECT method

engcom-Dash added Progress: pending review and removed Progress: needs update labels Nov 22, 2024

engcom-Hotel requested changes Nov 25, 2024

View reviewed changes

m2-community-project bot added Progress: needs update and removed Progress: pending review labels Nov 25, 2024

engcom-Dash added 2 commits November 28, 2024 15:13

39330: apply suggested changes and revert deleted file with deprecate…

9f32ea3

…d tag

39330: update copyright tag

e71d63b

lbajsarowicz commented Nov 28, 2024

View reviewed changes

m2-community-project bot added Progress: review and removed Progress: needs update labels Nov 28, 2024

engcom-Hotel added Progress: needs update and removed Progress: review labels Nov 29, 2024

SwiftOtter-SOP-348 Match CURL Client to RFC-9110 and RFC-5789

252c027

engcom-Dash closed this Dec 12, 2024

m2-community-project bot removed the Progress: needs update label Dec 12, 2024

This was referenced Dec 12, 2024

[2.5] Add support for missing Curl methods to the Curl client #39469

Open

[2.4] Add support for missing Curl methods to the Curl client #39471

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing REST methods to Curl Client (SwiftOtter's SOP-348) #39330

Add missing REST methods to Curl Client (SwiftOtter's SOP-348) #39330

lbajsarowicz commented Nov 5, 2024 •

edited by m2-assistant bot

Loading

m2-assistant bot commented Nov 5, 2024

lbajsarowicz commented Nov 5, 2024

michalbiarda left a comment

michalbiarda Nov 5, 2024

michalbiarda Nov 5, 2024

michalbiarda Nov 5, 2024

lbajsarowicz commented Nov 5, 2024

engcom-Hotel commented Nov 7, 2024

engcom-Hotel commented Nov 18, 2024

engcom-Hotel left a comment

engcom-Dash commented Nov 22, 2024

engcom-Dash commented Nov 22, 2024

engcom-Dash commented Nov 22, 2024

engcom-Hotel commented Nov 25, 2024

engcom-Hotel left a comment

engcom-Hotel Nov 25, 2024

engcom-Hotel Nov 25, 2024

lbajsarowicz Nov 28, 2024

engcom-Dash Nov 29, 2024

lbajsarowicz commented Dec 2, 2024

lbajsarowicz commented Dec 2, 2024

engcom-Dash commented Dec 2, 2024

engcom-Dash commented Dec 12, 2024

lbajsarowicz commented Dec 12, 2024

jakwinkler commented Dec 12, 2024

lbajsarowicz commented Dec 15, 2024

Add missing REST methods to Curl Client (SwiftOtter's SOP-348) #39330

Add missing REST methods to Curl Client (SwiftOtter's SOP-348) #39330

Conversation

lbajsarowicz commented Nov 5, 2024 • edited by m2-assistant bot Loading

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Resolved issues:

m2-assistant bot commented Nov 5, 2024

lbajsarowicz commented Nov 5, 2024

michalbiarda left a comment

Choose a reason for hiding this comment

michalbiarda Nov 5, 2024

Choose a reason for hiding this comment

michalbiarda Nov 5, 2024

Choose a reason for hiding this comment

michalbiarda Nov 5, 2024

Choose a reason for hiding this comment

lbajsarowicz commented Nov 5, 2024

engcom-Hotel commented Nov 7, 2024

engcom-Hotel commented Nov 18, 2024

engcom-Hotel left a comment

Choose a reason for hiding this comment

engcom-Dash commented Nov 22, 2024

engcom-Dash commented Nov 22, 2024

engcom-Dash commented Nov 22, 2024

engcom-Hotel commented Nov 25, 2024

engcom-Hotel left a comment

Choose a reason for hiding this comment

engcom-Hotel Nov 25, 2024

Choose a reason for hiding this comment

engcom-Hotel Nov 25, 2024

Choose a reason for hiding this comment

lbajsarowicz Nov 28, 2024

Choose a reason for hiding this comment

Semantic Ambiguity:

Why GET can have a payload (though discouraged):

engcom-Dash Nov 29, 2024

Choose a reason for hiding this comment

lbajsarowicz commented Dec 2, 2024

lbajsarowicz commented Dec 2, 2024

engcom-Dash commented Dec 2, 2024

engcom-Dash commented Dec 12, 2024

lbajsarowicz commented Dec 12, 2024

jakwinkler commented Dec 12, 2024

lbajsarowicz commented Dec 15, 2024

lbajsarowicz commented Nov 5, 2024 •

edited by m2-assistant bot

Loading