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

Unifying REST API searches with the interface method getList() and http ... #889

Closed

Conversation

SchumacherFM
Copy link
Member

...method PUT.

Deleted POST routes and merged them to a PUT route to be more consistent with all other getList implementations.

…tp method PUT.

 Deleted POST routes and merged them to a PUT route to be more consistent.
@SchumacherFM
Copy link
Member Author

The two tests which are failing are not mine and some date tests:

1) Magento\Framework\Data\Form\Element\DateTest::testGetValue with data set #0 (array('short', 'short', 1420187502), '01/2/15 12:31 AM')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'01/2/15 12:31 AM'
+'1/2/15 12:31 AM'

@choukalos
Copy link

Link to internal issue ( MAGETWO-31679 ); Looks like builds are failing as API tests have been added into most recent builds - failing since POSTS have been changed to PUTS.

Internally on this issue there's some debate between using PUT vs GET for search operations. Do you-all prefer PUT given the complex search parameters that can be passed?

@choukalos choukalos self-assigned this Jan 5, 2015
@Vinai
Copy link
Contributor

Vinai commented Jan 5, 2015

Just on a side note, the test fail is not related to the POST -> PUT change but because of the issue fixed in PR #913.

@birchestx
Copy link

There is a limit in Internet Explorer as I understand with GET requests - 2048 chars. I'm not sure on how that applies to PUTs but know at WSA we use POST in some instances rather than GET for this and other reasons.

@avoelkl
Copy link
Contributor

avoelkl commented Jan 5, 2015

According to HTTP standards, GET is used to request a resource which - in my opinion - a search is.
PUT is used for storing new resources (or update them), POST to add sub-resources.
So the correct method would be GET.

@alankent
Copy link

alankent commented Jan 5, 2015

Agreed - PUT is for creating new resources, not doing a search. GET is most correct semantically if you just have a URL, POST is necessary when the URL is not / cannot be used to pass in parameters. PUT is not appropriate if there are no side effects (e.g. no new database entity has been created, as in a search).

@SchumacherFM
Copy link
Member Author

In that case (Alan) there needs to be more API routes changed from PUT to POST.
The search query can easily grow to more than 2048 chars so that GET could/would fail.

{
    "search_criteria": {
        "filter_groups": [
            {
                "filters": [
                    {
                        "field": "attribute_name",
                        "value": [string|int|float],
                        "condition_type": [string]; optional
                    }
                    more entries
                ]
            }
            more entries
        ],
        "current_page": [int] page number; optional
        "page_size": [int] number of items on a page; optional
        "sort_orders": [ optional
            {
                "field": "attribute_name",
                "direction": [int] -1 or 1
            }
            more entries
        ]
    }
}

@alankent
Copy link

alankent commented Jan 5, 2015

Yep (SchumacherFM). They should definitely be all made consistent.

@TexanHogman
Copy link

Based upon some research looks like IE9+ overcome 2048 limit. We have an internal ticket opened to convert all REST search requests to GET. Most importantly is getting them consistent.

@avoelkl
Copy link
Contributor

avoelkl commented Jan 6, 2015

+1 for making them consistent.

Would be great if we could make it to implement GET. That would be one of a few really correct implemented REST APIs.

@GordonLesti
Copy link
Contributor

REST in my eyes:

  • POST - create resource
  • GET - returns resource
  • PUT - edit resource
  • DELETE - delete resource

A search isn't really part of REST in my eyes, but GET matches the most for me. Maybe POST, but not PUT.

@choukalos
Copy link

@SchumacherFM can you update the PR to 'GET' & address the travis build failure?

Based on the feedback & discussion with our lead architect, @TexanHogman, this is the route we'd like to go because we call out IE9+ for full support right now in M1 ( 1.9 / EE 1.14 for responsive theme [link}http://www.magentocommerce.com/knowledge-base/entry/ee114-ce19-rwd-dev-guide#support] ).

Thanks

@SchumacherFM
Copy link
Member Author

I will do. I'm closing this PR and send you a new one.

Thank you everybody for contributing!

@alankent
Copy link

alankent commented Jan 6, 2015

Just to comment on PUT = update and POST = create, here is a more accurate way to say it: http://restcookbook.com/HTTP%20Methods/put-vs-post/
That is, PUT is good if you know the url. When you create a new entity in Magento, Magento allocates the unique ID so PUT is not appropriate and POST better.

@SchumacherFM SchumacherFM deleted the catalogWebApiRouteFix branch January 7, 2015 10:00
SchumacherFM added a commit to SchumacherFM/magento2 that referenced this pull request Jan 7, 2015
…nd http method PUT.

 Deleted POST routes and merged them to a GET route to be more consistent.
 Fixes magento#889
magento-team pushed a commit that referenced this pull request Mar 6, 2017
Task
- MAGETWO-65085 [PR] Delivery of deployment improvements
Story
- MAGETWO-63084 Sync config file with DB: Validation
- MAGETWO-62736 Rename config.local.php file, use env.php file as configs storage
- MAGETWO-63381 CLI Improvements: Configuration management - Hide sensitive values from config:show command
- MAGETWO-64223 CLI Improvements: Configuration management - Add validations to config:set command
- MAGETWO-63095 User can change the Interface Locale only to locales that are already deployed.
VitaliyBoyko pushed a commit to VitaliyBoyko/magento2 that referenced this pull request Jun 22, 2018
…rification

magento#889 - Remove all cross checks between Inventory and Catalog regarding SKU Verification
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants