Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
Follow-up from #1816354: Add a REST module, starting with DELETE: The REST module should allow GET requests to all entities where their representation in a specified format can be retrieved. The format will be hard-coded to JSON-LD for now until #1833440: Implement partial matcher based on content negotiation MIME type is resolved.
I will work on this in the "GET-read-1834288" branch in my sandbox.
Comment | File | Size | Author |
---|---|---|---|
#32 | rest-1834288-32.patch | 21.84 KB | klausi |
#32 | rest-1834288-32-interdiff.txt | 8.75 KB | klausi |
#25 | rest-1834288-25.patch | 18.97 KB | klausi |
#25 | rest-1834288-25-interdiff.txt | 12.31 KB | klausi |
#17 | rest-1834288-17.patch | 43.19 KB | klausi |
Comments
Comment #1
klausiFirst ugly patch attached, based on #1816354: Add a REST module, starting with DELETE. The format application/vnd.drupal.ld+json (drupal_jsonld) is hard coded right now. Also includes a test case for entity_test, the other entity typed do not support EntityNG with properties right now.
Comment #2
Crell CreditAttribution: Crell commentedThis class is ContainerAware, so it should just call $this->container->get().
Comment #3
ygerasimov CreditAttribution: ygerasimov commentedI have altered patch according to #2.
Also:
- changed format 'application/vnd.drupal.ld+json' to 'application/ld+json' as application/vnd.drupal.ld+json is going to be used for content staging (according to help message in jsonld module)
- changed dblog resource to use serializer and not returing Response object. I found out that our normalizer does not support non EntityNG objects so now they are converted to array before serializing
- added dependency to jsonld module
- added verbose message for test rest calls
Shall we create normalizer that will support all kind of objects or keep this converting to array for now?
Comment #4
Crell CreditAttribution: Crell commentedAny entity should eventually become Entity NG, and therefore be supported that way. Anything else, eh, can make its own integration.
Comment #5
ygerasimov CreditAttribution: ygerasimov commentedI haven't explained quite right. For dblog resource we return record of the database (object of stdClass). This is where our normalizer fails. And for such cases I propose to convert object to array.
Comment #6
Grayside CreditAttribution: Grayside commentedWill #1833440: Implement partial matcher based on content negotiation MIME type remove the dependency on the jsonld module?
And given that issue and #1803586: Give all entities their own URI, why does the REST module handle GET at all? Isn't the Router essentially Drupal's GET support?
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedBetter to use 'application/vnd.drupal.ld+json' for the moment, as that's likely to stabilize more quickly.
There is an example of a Normalizer which just converts an object to an array in #1819760: Add a REST export display plugin and serializer integration.. All object conversion in the serialization process should be handled by a Normalizer.
However, we probably don't want to return JSON-LD for non-entities. While any valid JSON is technically valid JSON-LD, a JSON-LD parser can't do anything with it if it doesn't have a @context, and the @context is dependent on the Typed Data API.
The Router allows modules to register routes. However, something needs to register the route and provide the controller. That's where REST module would come in.
Other modules might also register routes for the same URI. However, they would probably be registering a route for the _format "text/html", not for the other available mime types.
Comment #7.0
klausiUpdated working repo URL
Comment #8
Grayside CreditAttribution: Grayside commentedOkay, I understand that each route will be bound to a particular representation format. I'm still unclear on what the REST module is providing for GET that shouldn't be part of how the core Routing system works. Is it automatic glue between routes and every serialization format known in the system?
Comment #9
Crell CreditAttribution: Crell commentedThat's really all it's offering for any HTTP method. The routing system handles all of the mapping of method/path/format to a controller; The rest module is a convenience system to make wiring stuff up to that mechanism easier.
Comment #10
BerdirNot sure what to do with the issue title but rest.module has a component now, no longer necessary to use the title for that :)
Comment #11
Crell CreditAttribution: Crell commentedComment #12
webchickThis seems like a pretty important feature to having web services in core. Escalating to a "major" feature request.
Comment #13
klausiUpdated patch attached.
This introduces settings per request type for the purpose of serialization and returning the correct response codes. Goal: a resource plugin should be able to easily override the input/output semantics of a request method. Example: per default we do not return a response body on POST requests, but a plugin might want to return the serialized representation after creating a resource.
I came up with the following default mapping:
This is of course not final, just my current suggestion.
It might be cool to have that settings as annotations on the methods of the plugin, but that is just an implementation detail right now. An idea for the time after feature freeze.
My current vision is to keep request/response specific logic out of the plugin method implementation. Example: the get() method should only return the loaded object. It should not serialize the object, it should not set any response HTTP headers. Example: the post() method should receive an already de-serialized object, save it and return it. Problem: the response to the POST request should contain a Location header with the URI to the newly created object. Who will set that header? The RequestHandler does not differentiate between the request methods and does not know where to set it and where not. The plugin does not create response objects, so it would have to also return the headers. Anyway, I digress, this is not a problem for this issue but later for #1839346: REST module: POST/create.
Comment #14
klausiOops, forgot to add the simpletest file to the patch. Interdiff is still against #3.
Comment #15
Grayside CreditAttribution: Grayside commentedResponses seem good. I rather like having the new/updated representation echo'd back in the response, but since that is a performance overhead can be done without. It would be nice if, say, Contrib could extend to add a debug mode that provided that behavior.
Typo.
Comment #16
webchickOops. That means we need test coverage for that too. :)
Comment #17
klausi@webchick: simpletests are already included in the ReadTest class. Or do you mean something specific?
Fixed the variable name as reported in #15.
This patch now bases on #1801570: DX: Replace hook_route_info() with YAML files and RouteBuildEvent. Please review rest-review-1834288-17.txt.
Comment #19
klausi#17: rest-1834288-17.patch queued for re-testing.
Comment #20
webchickI mean that we should not have a green patch with a typo like in #15, so we're obviously missing some test coverage for that branch.
Comment #21
klausiIn this case I applied the typo consistently everywhere, so there was no functional error. Thank you netbeans for auto completing my mistake everywhere :-)
Comment #22
Crell CreditAttribution: Crell commentedThis whole block can be put straight into the object property. No need to lazy-init here.
That said, I don't actually understand the purpose of this array. It feels very kludgy. Why is this needed, exactly?
This seems like it's reimplementing the kernel.view listener, which already fires when a controller returns something other than a Response object.
Comment #23
Crell CreditAttribution: Crell commentedThis whole block can be put straight into the object property. No need to lazy-init here.
That said, I don't actually understand the purpose of this array. It feels very kludgy. Why is this needed, exactly?
This seems like it's reimplementing the kernel.view listener, which already fires when a controller returns something other than a Response object.
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedAs I mentioned in my last comment, I feel pretty strongly that we should handle even simple object to array conversion with a Normalizer rather than doing manipulation on the data before it hits Serializer. Otherwise it sets a bad example for contrib, IMHO.
13 days to next Drupal core point release.
Comment #25
klausiBetter approach: instead of the complex requestMethod() the plugins can return response objects. That has the advantage that they can set all sort of stuff like status code, location header etc. themselves. I created a ResourceResponse as subclass of Symfony's Response class. It has one additional variable that holds a data structure that should be serialized later. I do not want to misuse the content variable of the original Response class for that.
So now all methods like get(), post(), delete() etc. on the resource plugins have to return ResourceResponse objects. If the method provides outgoing data the request handler will serialize it. So we keep the separation of serialization from resource plugins.
Commit log:
a1fdb56 Fixed @return docs about the new ResourceResponse object.
dd074bf Changed empty response body to NULL for consistency.
c36a239 removed unused use statement, explained dblog response object.
31eb0de Removed unused variable, removed object => array conversion from request handler.
9889b0f Removed unecessary db_select() call from dblog resource. Hello Crell!
ce44b0c Added a custom resource response class that can hold data for serialization. Reverted the complex requestMethod settings in reso
9a84066 Merge branch '8.x' into GET-read-1834288
Comment #26
fubhy CreditAttribution: fubhy commentedOkay this looks like a good temporary solution. However we should consider moving the serialization out of the responsibility of the respond object at all if you ask me. Instead we should consider serializing through a even subscriber for the route to either un-serialize the incoming data or serialize the outgoing data before printing it. We have to keep in mind that this is not specific to the REST module but will be used elsewhere. I have been playing around with that idea in my head for some time now but always forgot to set up an issue for that.
I quickly set up a placeholder issue for this feature request over here: #1842744: Automatically unserialize incomming request data / serialize outgoing response data and will post a more detailed proposal shortly.
As for this patch: I think it's RTBC. We will have to re-factor the hard-coded bits later.
Comment #27
moshe weitzman CreditAttribution: moshe weitzman commentedThis looks solid to me. This is a huge step for web services in core. Nicely done, folks.
looks like debug code?
Comment #28
klausiThis is intended "debug" code same as in WebTestBase::drupalGet() for example. It helps to quickly identify errors in HTTP responses when developing simpletests. You can turn off these debug messages with the "Provide verbose information when running tests" setting.
Back to RTBC I guess, please set the status accordingly if you disagree.
Comment #29
moshe weitzman CreditAttribution: moshe weitzman commentedThanks for the explanation. Yes, RTBC.
Comment #30
ygerasimov CreditAttribution: ygerasimov commentedI am happy with the patch. Thanks guys!
Comment #31
Grayside CreditAttribution: Grayside commented@linclark in #7 suggested
application/vnd.drupal.ld+json
as a first step.Comment #32
klausi@Grayside: right.
Commit log:
Changed mime type to vnd.drupal.ld+json.
Added assertHeader() method for testing HTTP response headers.
Please review.
Comment #34
klausi#32: rest-1834288-32.patch queued for re-testing.
Comment #35
Crell CreditAttribution: Crell commentedBack to RTBC.
Comment #36
Dries CreditAttribution: Dries commentedThis looks good as well. Committed to 8.x. Thanks.
Comment #37.0
(not verified) CreditAttribution: commentedupdated branch name