-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Conversation
…tp method PUT. Deleted POST routes and merged them to a PUT route to be more consistent.
The two tests which are failing are not mine and some date tests:
|
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? |
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. |
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. |
According to HTTP standards, GET is used to request a resource which - in my opinion - a search is. |
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). |
In that case (Alan) there needs to be more API routes changed from PUT to POST.
|
Yep (SchumacherFM). They should definitely be all made consistent. |
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. |
+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. |
REST in my eyes:
A |
@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 |
I will do. I'm closing this PR and send you a new one. Thank you everybody for contributing! |
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/ |
…nd http method PUT. Deleted POST routes and merged them to a GET route to be more consistent. Fixes magento#889
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.
…rification magento#889 - Remove all cross checks between Inventory and Catalog regarding SKU Verification
...method PUT.
Deleted POST routes and merged them to a PUT route to be more consistent with all other getList implementations.