Spec Compliance: Return 204 or 200, not 201 for relationship POST requests.

Nice find!

Can't we fix this in 8.x-1.x?

Following the @todo instructions in the existing test coverage in HEAD. This will cause failures; in theory I should not need to make any additional changes to test coverage; only to controller/normalizer logic.

I missed a few in #5.

I managed to fix most test failures by updating expectations. But I'm stuck on

// 10. Successful PATCH to related endpoint.

in JsonApiFunctionalTest::testWrite().

For some reason the EntityResource::patchRelationship() → $entity->validate() → \Drupal\Core\Entity\Plugin\Validation\Constraint\EntityChangedConstraintValidator::validate() → ::loadUnchanged() call chain is resulting in an exception because a the appropriate node DB table does not exist 😵😱 This is the exact exception:

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'd8.test84784340node' doesn't exist: SELECT revision.vid AS vid, revision.langcode AS langcode, revision.revision_uid AS revision_uid, revision.revision_timestamp AS revision_timestamp, revision.revision_log AS revision_log, revision.revision_default AS revision_default, base.nid AS nid, base.type AS type, base.uuid AS uuid, CASE base.vid WHEN revision.vid THEN 1 ELSE 0 END AS isDefaultRevision
FROM 
{node} base
INNER JOIN {node_revision} revision ON revision.vid = base.vid
WHERE base.nid IN (:db_condition_placeholder_0); Array
(
    [:db_condition_placeholder_0] => 1
)

I'm clueless. I've spent more than an hour and a half on this already, still nothing. So just posting it here, hoping somebody else figures this out!

1. +++ b/src/Controller/EntityResource.php
   @@ -543,13 +543,59 @@ class EntityResource {
   +    $original_field_list = clone $field_list;
   

   This is the only change I can see could possibly be causing this problem, in some insanely roundabout/complex way.

   Another round of debugging didn't help. Tomorrow with a fresh set of eyes, this should be solvable.

2. +++ b/src/Controller/EntityResource.php
   @@ -543,13 +543,59 @@ class EntityResource {
   +   *   The new (udpated) entity references.
   

   s/udp/upd/

I can no longer reproduce the bizarreness of #11…

In fact, it actually was pretty easy now to make the entire patch green! Both on 8.5 and 8.6.

Maybe something else in JSON API changed that solved the root cause. Maybe it was my machine. Maybe it was me. 😳

Self-review:

1. +++ b/tests/src/Functional/ResourceTestBase.php
   @@ -1392,16 +1392,11 @@ abstract class ResourceTestBase extends BrowserTestBase {
   -      // @todo Remove line below in favor of commented line in https://www.drupal.org/project/jsonapi/issues/2977653.
   -      $this->assertResourceResponse(201, FALSE, $response);
   -      // $this->assertResourceResponse(204, NULL, $response);
   +      $this->assertResourceResponse(204, NULL, $response);
   

   All changes in this class are like this: removing @todos and uncommenting associated intended test coverage, replacing the old test coverage.

   These were added by @gabesullice in #2972808: Comprehensive JSON API integration test coverage phase 6: POST/PATCH/DELETE of relationships. While working on this patch, I vetted every one. I consulted the JSON API spec while doing so.

2. +++ b/src/Controller/EntityResource.php
   @@ -555,13 +555,59 @@ class EntityResource {
   -    return $this->getRelationship($entity, $related_field, $request, 201);
   +    $status = static::relationshipArityIsAffected($original_field_list, $field_list)
   +      ? 200
   +      : 204;
   +    return $this->getRelationship($entity, $related_field, $request, $status);
   
   @@ -600,7 +646,7 @@ class EntityResource {
   -    return $this->getRelationship($entity, $related_field, $request);
   +    return $this->getRelationship($entity, $related_field, $request, 204);
   
   @@ -693,7 +739,7 @@ class EntityResource {
   -    return $this->getRelationship($entity, $related_field, $request, 201);
   +    return $this->getRelationship($entity, $related_field, $request, 204);
   

   These are the necessary logic changes to make the test coverage in point 1 pass.

3. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -744,7 +744,7 @@ class JsonApiFunctionalTest extends JsonApiFunctionalTestBase {
   -    $this->assertEquals(201, $response->getStatusCode());
   +    $this->assertEquals(200, $response->getStatusCode());
   
   +++ b/tests/src/Kernel/Controller/EntityResourceTest.php
   @@ -691,7 +691,7 @@ class EntityResourceTest extends JsonapiKernelTestBase {
   -    $this->assertEquals(201, $response->getStatusCode());
   +    $this->assertEquals(204, $response->getStatusCode());
   

   The changes in these test classes are necessary to make them pass; they follow the example set by the test coverage in point 1. I also verified each of these changes.

I can't find any problems. I mostly just executed on the stubs that @gabesullice had already added, and also ensured this actually matches the JSON API spec. Still, I'd like @gabesullice to do the final sign-off (and hopefully commit).