Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Now that nodes are converted to EntityNG we should make sure in rest.module that they work in the REST API as expected. I think testing deletion should be easy, we can add that to the existing test case. Maybe also read/GET. Create/POST and Update/PATCH might be trickier and could uncover some node related issues, so that might get separate test methods (or classes).
Comment | File | Size | Author |
---|---|---|---|
#20 | node-tests-1972116-20.patch | 24.68 KB | klausi |
#20 | node-tests-1972116-20-interdiff.txt | 3.22 KB | klausi |
#17 | node-tests-1972116-17.patch | 22.55 KB | klausi |
#17 | node-tests-1972116-17-interdiff.txt | 2.48 KB | klausi |
#15 | node-tests-1972116-15.patch | 20.37 KB | klausi |
Comments
Comment #1
klausiHere is a patch that adds tests for GET and DELETE, which is relatively easy and uses the same code as entity_test.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedLooks like a reasonable way to expand test coverage. We can do more in followups.
Comment #3
klausiHeh, maybe I should have mentioned that this patch is not finished yet :-)
We need tests for PATCH and POST here as well, since Lin already mentioned problems when POSTing nodes.
Comment #4
klausiAdapted creation test to also include nodes, works fine in this minimal version of POSTing the node type and title only.
TODO: test coverage for PATCH.
Comment #5
klausiI did some manual POST testing with nodes and it is important that you remove all special fields from your content type that you want to test. Taxonomy term reference fields, file fields etc. do not work right now.
Here is a cURL example request I used successfully to create a primitive article that has a title and a body field:
You have to replace the cookie and the CSRF token (can be retrieved at /rest/session/token) accordingly.
Comment #6
linclark CreditAttribution: linclark commentedI also had to disable comment module to make this work.
Comment #7
linclark CreditAttribution: linclark commentedThe offending code in comment module is this:
If the $node doesn't have a uid set, then there is an integrity constraint violation because it can't be NULL.
REST module should probably set the $node->uid anyway, using the user id of the user who made the request. I don't think we want a user with POST privileges to be able to create a node and assign it to another uid.
Comment #8
webchickThis sounds like it's probably at least major, given the number of bugs digging into this is uncovering.
Comment #9
linclark CreditAttribution: linclark commentedI spun out the UID issue, #1979260: Automatically populate the author default value with the current user.
Comment #10
benjifisher#4: node-tests-1972116-4.patch queued for re-testing.
Comment #11
klausiImplemented a test case for nodes with comment module enabled, so that a missing node author does not trigger errors in comment module.
What do we want to test for PATCH regarding nodes?
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedFor PATCH, the simplest is to change required fields like created, author, title. More advanced PATCH could be handled in these tests or in serialization (e.g. date fields, entity ref fields, etc.).
Comment #13
klausiPATCH is really broken right now, I added a test case to demonstrate that a title field change fails.
The problem is that serializer (the denormalier) uses entity_create() for building the node object. During that default values get applied to the fields, which we do not want in the PATCH case. We only want to update some specific fields on an existing node.
We could pass the node ID to serializer and it could perform an entity_load() to get the existing node, but then we don't know which fields where updated later in REST module in order to perform field access checks on them. So we could also move the access checks into serializer, but then it does way to much business logic outside the scope of just deserializing.
One more time I realize that we need #1965780: Add EntityDiff entity type for handling PATCH requests.
Comment #15
klausiNew idea: use a deserialization context to indicate PATCH request data. The denormalizer can use that to simply unset all fields on the prepared entity and then fill it with incoming data. That is one way to get rid of the default values.
This would also mean that we don't need an extra EntityDiff approach. What do you think?
Comment #16
linclark CreditAttribution: linclark commentedI'm at NYC Camp, so haven't had a chance to do a full review yet, but the basic principle sounds good to me.
Do you have a test to ensure that a field can be deleted using the empty array, and that a field that wasn't in the request does not get deleted? I thought we got rid of the distinction between empty values and NULL values in Portland.
Comment #17
klausiDeleting field with an empty array works as we expect, we already have a test case for that. I added a test to make sure that omitted fields in a PATCH request do not delete the field.
I also documented the "request_method" context array key on the denormalizer.
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedThis looks good to me, and much needed.
Comment #19
alexpottShouldn't we be testing the code that prevents changing of values to default here too. That is, shouldn't we be testing other values have not changed.
So I removed the code in EntityNormalizer::denormalize concerning the patch method (see above) and ran Drupal\rest\Tests\UpdateTest - which still passes. I thought this might be testing that code? I guess not - which means we have no test coverage of it.
Comment #20
klausiThanks Alex, very good points! I added an explicit test for the HAL denormalizer + a check for the UUID in the node test.
Comment #22
klausi#20: node-tests-1972116-20.patch queued for re-testing.
Comment #23
moshe weitzman CreditAttribution: moshe weitzman commentedmore test coverage is good. back to rtbc.
Comment #24
alexpottCommitted cde3c7d and pushed to 8.x. Thanks!