Avoid translations DELETE data loss and unintended changes with PATCH and test all methods against entity route parameter translation upcasting

Let's start by addressing the TODO in the pre-existing test coverage.

And let's make it more clear which modules are installed by those tests.

Tests coverage that shows PATCHing a translation is possible, and it does so without modifying the default translation (i.e. data integrity is preserved).

Translation fallback time!

Tests coverage that shows

1. GETting a non-existing translation for an existing langcode returns the fallback translation
2. PATCHing a translation fallback is possible.

POSTing to a langcode-prefixed JSON:API URL works, but it'll always create the entity in the default translation, even when specifying a langcode field.

DELETEing a langcode-prefixed JSON:API URL works, but it'll always delete the entity, not the translation.

#2 through #7 added test coverage asserting the current behavior, powered by core's automatic entity route parameter translation.

#2 and #3 are just preparation steps.

How does the current behavior #4, #5, #6 and #7 assert compare to what @plach's concerns are?

GET

@plach: /

The existing test coverage already proved it worked when explicitly negotiating a language, #5 expands it to also test the fallback behavior. This behaves as expected.

DELETE

@plach:

Additionally, deleting a translation and deleting the whole entity are very different operations, so we can't really apply a DELETE operation to a non-default translation and fall back to the default one if the former is missing, because that would mean deleting all existing translations and the entity itself.

#7 shows that DELETE on a translation indeed deletes the entire entity. I think we should change that behavior. This is unambiguously confusing. So I completely agree with @plach that we should make this impossible.

Proposal: throw a 405 Method Not Allowed when a DELETE request is performed when it is a non-default translation or when the default translation's langcode does not match $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_CONTENT)->getId().

End result is that only DELETE /jsonapi/… is allowed, and never DELETE /xx/jsonapi/….

POST

@plach:

POST operations do not seem as much concerning at first sight, since there is no fallback possible given that no translation exists yet.

#6 shows that POST always results in an entity in the default translation, even when both a language prefix is specified and the langcode field is specified.

Proposal: throw a 405 Method Not Allowed when a POST request is performed when a non-default language is negotiated

End result is that only POST /jsonapi/… is allowed, and never POST /xx/jsonapi/….

PATCH a translation

@plach: /

#4 added test coverage for this. This works as expected. The default translation remains unchanged.

PATCH a translation fallback

@plach:

[…]
In this case we might be happily performing changes to the wrong target without the client even noticing it, until the next GET.
[…]
could be to ignore the fallback logic in controllers responsible to handle the PATCH

#5 shows that a translation fallback can be PATCHed, but it is modifying the fallback, it's not creating the requested translation. Whether that's sensible is another matter of course.

Proposal: disable the translation fallback logic for PATCH requests like @plach proposed, this would result in a 4xx response because the client is attempting to modify a non-existing translation.

Discussed this on a call with @plach, @effulgentsia, @xjm, @gabesullice and I. Outcome:

1. +++ b/tests/src/Functional/JsonApiFunctionalMultilingualTest.php
   @@ -40,30 +43,36 @@ class JsonApiFunctionalMultilingualTest extends JsonApiFunctionalTestBase {
   +    // Test reading an individual entity fallback.
   +    $output = Json::decode($this->drupalGet('/ca-fr/jsonapi/node/article/' . $this->nodes[0]->uuid()));
   +    $this->assertEquals($this->nodes[0]->getTranslation('ca')->getTitle(), $output['data']['attributes']['title']);
   

   @plach confirmed this is correct.

2. +++ b/tests/src/Functional/JsonApiFunctionalMultilingualTest.php
   @@ -73,4 +82,167 @@ class JsonApiFunctionalMultilingualTest extends JsonApiFunctionalTestBase {
   +  public function testPatchTranslation() {
   

   @plach confirms this looks good.

   BUT we should add test coverage for attempting to change the langcode field. en+ca+it languages. en+ca translations exist. PATCH en, set it to 'it'.

3. +++ b/tests/src/Functional/JsonApiFunctionalMultilingualTest.php
   @@ -73,4 +82,167 @@ class JsonApiFunctionalMultilingualTest extends JsonApiFunctionalTestBase {
   +    // Omitting a langcode results in an entity in the default translation.
   +    // (Creating a new translation is impossible despite the language prefix in
   +    // the URL.)
   +    unset($request_document['data']['attributes']['langcode']);
   +    $request_options[RequestOptions::BODY] = Json::encode($request_document);
   +    $response = $this->request('POST', Url::fromUri('base:/ca/jsonapi/node/article/'), $request_options);
   +    $this->assertSame(201, $response->getStatusCode());
   +    $document = Json::decode((string) $response->getBody());
   +    $this->assertSame($title, $document['data']['attributes']['title']);
   +    $this->assertSame('en', $document['data']['attributes']['langcode']);
   +    $this->assertSame(['en'], array_keys(Node::load($document['data']['attributes']['drupal_internal__nid'])->getTranslationLanguages()));
   

   Correc.t

4. +++ b/tests/src/Functional/JsonApiFunctionalMultilingualTest.php
   @@ -73,4 +82,167 @@ class JsonApiFunctionalMultilingualTest extends JsonApiFunctionalTestBase {
   +    $this->assertSame($node->getTitle() . ' (ca) UPDATED', $document_ca_updated['data']['attributes']['title']);
   +    $this->assertSame($node->getTitle() . ' (ca) UPDATED', $document_cafr_updated['data']['attributes']['title']);
   

   @plach confirmed that this is wrong/undesirable. Same solution as POST.

5. +++ b/tests/src/Functional/JsonApiFunctionalMultilingualTest.php
   @@ -73,4 +82,167 @@ class JsonApiFunctionalMultilingualTest extends JsonApiFunctionalTestBase {
   +    // Omitting a langcode results in an entity in the default translation.
   +    // (Creating a new translation is impossible despite the language prefix in
   +    // the URL.)
   +    unset($request_document['data']['attributes']['langcode']);
   +    $request_options[RequestOptions::BODY] = Json::encode($request_document);
   +    $response = $this->request('POST', Url::fromUri('base:/ca/jsonapi/node/article/'), $request_options);
   +    $this->assertSame(201, $response->getStatusCode());
   +    $document = Json::decode((string) $response->getBody());
   +    $this->assertSame($title, $document['data']['attributes']['title']);
   +    $this->assertSame('en', $document['data']['attributes']['langcode']);
   +    $this->assertSame(['en'], array_keys(Node::load($document['data']['attributes']['drupal_internal__nid'])->getTranslationLanguages()));
   

   Working as expected per @plach: this is how this API is designed to work.

6. +++ b/tests/src/Functional/JsonApiFunctionalMultilingualTest.php
   @@ -73,4 +82,167 @@ class JsonApiFunctionalMultilingualTest extends JsonApiFunctionalTestBase {
   +    // Specifying a langcode is allowed once configured to be alterable … but it
   +    // still results in an entity with only a default translation.
   +    // (Creating a new translation is impossible despite the language prefix in
   +    // the URL.)
   +    ContentLanguageSettings::create([
   +      'target_entity_type_id' => 'node',
   +      'target_bundle' => 'article',
   +    ])->setLanguageAlterable(TRUE)
   +      ->save();
   +    $request_document['data']['attributes']['langcode'] = 'ca';
   +    $response = $this->request('POST', Url::fromUri('base:/ca/jsonapi/node/article/'), $request_options);
   +    $this->assertSame(201, $response->getStatusCode());
   +    $document = Json::decode((string) $response->getBody());
   +    $this->assertSame($title, $document['data']['attributes']['title']);
   +    $this->assertSame('en', $document['data']['attributes']['langcode']);
   +    $this->assertSame(['en'], array_keys(Node::load($document['data']['attributes']['drupal_internal__nid'])->getTranslationLanguages()));
   

   Incorrect per @plach, since this is using the intended API, yet not yielding the expected result.

   We agreed that we want to use the same mitigation as for POST.

   Also needs test for POSTing to /jsonapi/node/article, with langcode being set. This should also result in a 405 if it currently is resulting in a non-ca.

7. +++ b/tests/src/Functional/JsonApiFunctionalMultilingualTest.php
   @@ -73,4 +82,167 @@ class JsonApiFunctionalMultilingualTest extends JsonApiFunctionalTestBase {
   +  public function testDeleteMultilingual() {
   +    $this->grantPermissions(Role::load(RoleInterface::ANONYMOUS_ID), [
   +      'bypass node access',
   +    ]);
   +
   +    $response = $this->request('DELETE', Url::fromUri('base:/ca/jsonapi/node/article/' . $this->nodes[0]->uuid()), []);
   +    $this->assertSame(204, $response->getStatusCode());
   +    $this->assertFalse(Node::load($this->nodes[0]->id()));
   +  }
   

   @plach confirmed that the proposed solution in #8 is what we want to do.

First: fix the tests. I broke this in the second interdiff 😅

Incorrect per @plach, since this is using the intended API, yet not yielding the expected result.

Turns out this is working as expected, I just had a tiny bug in my test coverage. 😅

I had

$request_document['data']['attributes']['langcode'] = 'ca';

but forgot to do:

 $request_options[RequestOptions::BODY] = Json::encode($request_document);

So a request body was sent without the langcode field. That explains it!

Conclusion: All POST requests are working as expected, no data integrity issues, so no action needs to be taken.

This adds the additional POST test coverage @plach asked in #15.7:

Also needs test for POSTing to /jsonapi/node/article, with langcode being set.

Thanks to #21's test coverage fix, this works as expected (before I found that, this was failing in just the same way).

This adds the additional PATCH test coverage @plach asked in #15.2:

BUT we should add test coverage for attempting to change the langcode field. en+ca+it languages. en+ca translations exist. PATCH en, set it to 'it'.

This is disallowed by \Drupal\Core\Entity\ContentEntityBase::onChange(): it throws an exception, which @plach explicitly called out during the call in response to a concern raised by @effulgentsia 👍, and now we have test coverage for it ✅

It's great when things work as we think they do!

Respond with a 405 to DELETE requests that act on translations. Concrete error message: Deleting a resource object translation is not yet supported. See https://www.drupal.org/docs/8/modules/jsonapi/translations.

Reason: in HEAD, this is a data integrity issue, since it seems you're deleting a particular translation, but it deletes the entire entity.

Respond with a 405 to PATCH requests that act on translation fallbacks. Concrete error message: The requested translation of the resource object does not exist, instead modify one the translations that do exist: l1, l2, l3.

Reason: in HEAD, this is a data integrity issue, since it seems you're patching the requested translation, but you're actually patching another translation: the fallback translation.

Double negation oopsie.

#29:

1. ✅ I wanted to spend my brain cycles on solving the real problem, not on mechanics. Turns out that I missed that the parent class already contains the language manager, so there was nothing to inject, yay! Fixed.
2. ✅ Good catch!

#30: thanks, much better indeed.

D'oh:

42x: The property languageManager (language_manager service) is deprecated in Drupal\jsonapi\ParamConverter\EntityUuidConverter and will be removed before Drupal 9.0.0.

That wasn't happening in my local environment … ah, apparently #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing just made that happen two days ago! 😬

Fixed. Tested locally on both 8.6 and 8.7.

Apparently a JSON:API kernel test is suddenly failing on 8.7 only, not on 8.6:

esting Drupal\Tests\jsonapi\Kernel\Context\FieldResolverTest
EEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEE          56 / 56 (100%)

Time: 25.36 seconds, Memory: 10.00MB

There were 56 errors:

1) Drupal\Tests\jsonapi\Kernel\Context\FieldResolverTest::testResolveInternalIncludePath with data set "entity reference" (array(array('field_test_ref2')), 'field_test_ref2')
TypeError: Argument 1 passed to Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getTableMapping() must implement interface Drupal\Core\Entity\EntityTypeInterface, null given, called in /var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php on line 1477

/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:193
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:1477
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorageSchema.php:621
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1540
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1593
/var/www/html/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php:1541
/var/www/html/core/lib/Drupal/Core/Field/FieldStorageDefinitionListener.php:87
/var/www/html/core/modules/field/src/Entity/FieldStorageConfig.php:340
/var/www/html/core/modules/field/src/Entity/FieldStorageConfig.php:294
/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:490
/var/www/html/core/lib/Drupal/Core/Entity/EntityStorageBase.php:445
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityStorage.php:263
/var/www/html/core/lib/Drupal/Core/Entity/EntityBase.php:394
/var/www/html/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php:613
/var/www/html/modules/contrib/jsonapi/tests/src/Kernel/Context/FieldResolverTest.php:359
/var/www/html/modules/contrib/jsonapi/tests/src/Kernel/Context/FieldResolverTest.php:55

That wasn't happening in my local environment … ah, apparently #2976035: Entity type CRUD operations must use the last installed entity type and field storage definitions broke this 17 hours ago! 😬GAaaah. That seems like a pretty significant BC break in core? :/ Investigating…

Yep, confirmed that this is only failing because that broke JSON:API's tests on 8.7: #2976035-122: Entity type CRUD operations must use the last installed entity type and field storage definitions .

Note that #32 is passing on 8.6, and once #2976035-122: Entity type CRUD operations must use the last installed entity type and field storage definitions is fixed, it'll also pass on 8.7. The failures are 100% unrelated to this patch.

#3038604: Fix tests following a change in Drupal 8.7.0: #2976035 fixed those failures. Re-testing #32.

@plach just pointed out in chat:

1. One small bug:
   

   +++ b/tests/src/Functional/JsonApiFunctionalMultilingualTest.php
   @@ -73,4 +82,195 @@ class JsonApiFunctionalMultilingualTest extends JsonApiFunctionalTestBase {
   +    $document_ca = Json::decode($this->drupalGet('/ca-fr/jsonapi/node/article/' . $uuid));
   +    $document_cafr = Json::decode($this->drupalGet('/ca-fr/jsonapi/node/article/' . $uuid));
   

   For $document_cafr, /ca-fr/jsonapi/… is requested. Good!

   For $document_ca, /ca/jsonapi/… is requested. Oops! This is a small typo in the test coverage, but precisely because ca-fr falls back to ca, all this needs is a URL change :)

2. some additional edge cases he'd like to see test coverage for in ::testPatchTranslation(). Did that.

Fix CS violations.

@plach just pointed out in chat:

1. Two comments that aren't quite accurate
2. He'd like to add an explicit test after the two rejected DELETE requests (resulting in 405 responses) that a DELETE request on the non-prefixed URL still works.

_{While doing the latter, I fixed one small oversight to ensure the correct error response is being asserted.}

#40: WFM, did that on commit!