PATCH 405 for untranslatable content entities with different default language than English

Thanks for creating a separate issue for this! This makes it much easier to keep an overview :)

You linked to #2794431: [META] Formalize translations support, but I think you meant to link to #3038308: Avoid translations DELETE data loss and unintended changes with PATCH and test all methods against entity route parameter translation upcasting, which is where changes were recently introduced.

You're running into the edge case that \Drupal\Tests\jsonapi\Functional\JsonApiFunctionalMultilingualTest::testPatchTranslationFallback() is explicitly testing.

Trimmed the response body to just the relevant bits.

All entities created via JSON:API or Drupal UI get und(?) langcode in JSON:API, but in fact, the default langcode for site is ru.

I think this is the root cause of all your problems. It sounds like your Content Translation settings are wrong. Pinging @plach to hopefully confirm :)

Having the same problem since 2.4.
My colleague has just found temporary fix - the patch request succeeds after adding to the node a translation to the main site language (the node has different language than main).

@Niklan:

Is the site multilingual? Can you try PATCHing at /ru/node/foo?

@plach:
Just tested it.
When adding /fr/ - main site language, it throws the same error as before.
When adding /en/ - node language - 404 No route found

@Niklan:

The weird thing is that if the site is monolingual, the current content language should always be ru, I think that's the condition to investigate/fix.

@Max_Z.:

/en/... won't work because by default the default language has no URL prefix configured. The intended behavior is that the negotiated content language must match the specified entity language, so ideally you would update English nodes on / and French nodes on /fr/.

@Wim Leers:

We may need some additional tests around the use cases described here to validate that the logic as behaving is expected.

@Niklan:

Ah, now I get it! Your nodes are being created with und language, which of course doesn't match the current content language. To match the current logic you'd have to change all the node languages from und to ru and restore the default content language settings, so that nodes are always created in the default site language. Alternatively we could relax the current constraints and take non-translated entities into account, as you are suggesting. I'm not sure if that's desirable from a BC/FC perspective given that proper multilingual support is still in the works, this is a @Wim Leers choice :)

I'm working with @Max_Z.

Website is multilingual with FR as default language. Not possible to PATCH nodes of custom content entity type created with language = EN. Tried to create FR translation for these contents, but now we are getting error like "Symfony\Component\HttpKernel\Exception\BadRequestHttpException: Updating a resource object that has a working copy is not yet supported. See https://www.drupal.org/project/jsonapi/issues/2795279. in Drupal\jsonapi\Controller\EntityResource->patchIndividual() (line 303 of /opt/bitnami/apps/drupal/htdocs/web/modules/contrib/jsonapi/src/Controller/EntityResource.php)."

I'm encountering this same 405 error when trying to PATCH or DELETE on a custom, non-translatable entity. The patch in #10 worked for me.

I think there's misunderstood about issue
- jsonapi checks interface but should also respect "runtime" entity type settings, needs test
- routing (path prefix) - should respect language negotiation and node type translatability config

Same error here with a custom entity that is not translatable, on a single language site, English.

The requested translation of the resource object does not exist, instead modify one of the translations that do exist: und.

I'm having the same issue, but on a ML site where the custom entity is set to not translatable. The patch didn't fix the issue for me though.

Downgrading to 8.x-2.3 did solve it though, so it must be something introduced since 2.4.

Same problem here. The content we create through JSONAPI is always English but when send the request to /de or /fi etc. language we get the error. And yes, I did work before the update 2.3->2.4.

Let's raise the priority since a lot of people seem to have this issue.

Since JSON API is now a core module in Drupal 8.7, I'm moving the issue accordingly. Also, here's an updated patch of #10 applied to Drupal core.

@Niklan, yes that patch worked for me.

A regression test in JsonApiRegressionTest.php that shows the problem would be extremely valuable. It's really difficult for me to tell if that all the users reporting problems here have the same problem as @Niklan.

Thank you @Niklan for the original patch and thank you @joelstein for re-rolling it for core! I agree that this change is probably one we ought to make. I'm only concerned about how/if it will affect BC/FC as @plach pointed out in #12. Translations are an area of JSON:API (really, in general) where I have little expertise.

I tried to write a regression test that replicates the scenario @Niklan described in #15. Locally, the test passes fine for me, so I must be missing some nuance (or maybe it will fail on d.o).

@Niklan, can you look at this test and tell me if it accurately reflects the preconditions and operations that you have? And, if not, either update the test or tell me what is different?

Update: it looks like the test only patch passed, so there's definitely some part of this bug that I don't yet understand.

#3038308: Avoid translations DELETE data loss and unintended changes with PATCH and test all methods against entity route parameter translation upcasting specifically introduced test coverage to prove this is working correctly. We worked with translation system expert @plach to ensure all edge cases had test coverage.

But perhaps this is an edge case we didn't think about. Because the title says PATCH 405 for untranslatable conent entities with different default language than English, and based on the error response in the issue summary you're not using a language prefix, the content entity has langcode und, yet you're getting "The requested translation of the resource object does not exist, instead modify one of the translations that do exist: und.", meaning this was triggered:

          if ($method === 'PATCH' && $entity->language()->getId() !== $current_content_language) {
            $available_translations = implode(', ', array_keys($entity->getTranslationLanguages()));
            throw new MethodNotAllowedHttpException(['GET'], sprintf('The requested translation of the resource object does not exist, instead modify one of the translations that do exist: %s.', $available_translations));
          }

That must mean that $entity->language()->getId() === 'und', but $current_content_language is something different, probably 'ru'.

#28:

+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -905,4 +907,60 @@ public function testMapFieldTypeNormalizationFromIssue3040590() {
+    // Ensure that the entity's langcode attribute is 'ru'.
+    $this->assertSame('ru', $response_document['data']['attributes']['langcode']);

This is where the test is deviating from the reported problem AFAICT: this should be und. I'm not saying that would be correct, I'm saying that that would match the reported behavior.

I'm not sure what the expected behavior here is. This is pretty deep entity multilingual stuff. Assigning to @Gábor Hojtsy for initial feedback, perhaps we should ask @plach later.

@Wim Leers:

But perhaps this is an edge case we didn't think about.

Definitely, we were concerned about preventing data integrity issues with content translations and we overlooked non-translatable entities.

As mentioned in #12, I think the patch proposed in #10 (and then in #25) makes sense as a fix. My main doubt was about whether you were interested in making this work since you are planning to rework it completely anyway, given that this is not a data-loss scenario.

It should definitely be possible to update tests to account for this scenario and only apply to translatable entities the restrictions we introduced earlier. The only actual behavior change should be that the case reported here starts working again, so we should not have BC issues.

Applied Patch#25, but 405 error is still there when patching EN node in FR website.

Having the same issue with a pretty vanilla Drupal Commerce installation trying to update the status of a commerce_order. #25 worked for me.

I opened duplicate #3067405: Cannot patch entities that are not translated (und != en). Same as #34. This shouldn't be postponed but NW due to missing tests. It's easily producible.

Sorry, selected wrong status.

Rebased #28, and incorporated my feedback from #31. Now it successfully reproduces the reported bug.

I tried to figure out how the issue reporter and @mglaman even get an entity with langcode=und and ended up debugging \Drupal\language\DefaultLanguageItem::applyDefaultValue() but not finding it. But how we get to that point is actually irrelevant, so this test will do.

And the fix is indeed what @Niklan suggested in #9.

Note that the same bug was introduced in \Drupal\Core\ParamConverter\EntityRevisionParamConverter::convert() by #2808163: Support dynamic entity types in the EntityRevisionParamConverter…

The good news is that #38 successfully reproduced the reported bug, and #39 fixed it :)

But it looks like JsonApiFunctionalMultilingualTest is missing some configuration that is causing Node::isTranslatable() to incorrectly return FALSE in a few cases. And that's causing failures in the test now.

Too bad, otherwise I'd have RTBC'd this 🤓

But it looks like JsonApiFunctionalMultilingualTest is missing some configuration that is causing Node::isTranslatable() to incorrectly return FALSE in a few cases. And that's causing failures in the test now.

That was indeed the cause. Simple fix.

\Drupal\Core\Entity\ContentEntityBase::isTranslatable() checks a key that is only ever set by content_translation_entity_bundle_info_alter() (some hidden but understandable coupling there), which led me to \Drupal\content_translation\ContentTranslationManager::isEnabled(), which pointed to \Drupal\content_translation\ContentTranslationManager::setEnabled()…

Assigning to @plach because he's definitely the core committer who should sign off on this :)

🙌Thank you @Wim Leers! I had one thought:

+++ b/core/modules/jsonapi/src/ParamConverter/EntityUuidConverter.php
@@ -62,7 +62,7 @@ public function convert($value, $definition, $name, array $defaults) {
-      if ($entity instanceof EntityInterface && $entity instanceof TranslatableInterface) {
+      if ($entity instanceof EntityInterface && $entity instanceof TranslatableInterface && $entity->isTranslatable()) {

There is Drupal\Core\Entity\TranslatableInterface would satisfy both of the interface checks (the Translatable interface check is against the TypedData one.)

Not sure if that's something we'd change in this patch as the main thing is to add isTranslatable. But it'd reduce cognitive load on reading the logic statement and fix a possibly redundant check.

I'd agree with you if it weren't for the fact that \Drupal\Core\Field\Plugin\Field\FieldFormatter\EntityReferenceFormatterBase::getEntitiesToView() and others in core also use that same instanceof check :)

Tagging to back up assigning.

The patch looks very good, thanks! Only very minor tweaks needed:

1. +++ b/core/modules/jsonapi/src/ParamConverter/EntityUuidConverter.php
   @@ -62,7 +62,7 @@ public function convert($value, $definition, $name, array $defaults) {
   -      if ($entity instanceof EntityInterface && $entity instanceof TranslatableInterface) {
   +      if ($entity instanceof EntityInterface && $entity instanceof TranslatableInterface && $entity->isTranslatable()) {
   

   I agree with @mglaman that it would be better to use Drupal\Core\Entity\TranslatableInterface: this was introduced more recently than the typed data one and we should definitely start adopting it whenever possible.

2. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -989,4 +991,65 @@ public function testLeakedCacheMetadataViaRdfFromIssue3053827() {
   +   * @see https://www.drupal.org/project/drupal/issues/3043168
   +   */
   +  public function testNonTranslatableEntityUpdatesFromIssue3043168() {
   

   I don't think we do this normally.

3. +++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
   @@ -989,4 +991,65 @@ public function testLeakedCacheMetadataViaRdfFromIssue3053827() {
   +      'langcode' => 'und',
   ...
   +    // Ensure that the entity's langcode attribute is 'und'.
   +    $this->assertSame('und', $response_document['data']['attributes']['langcode']);
   ...
   +    $this->assertSame('und', $response_document['data']['attributes']['langcode']);
   

   Let's use LanguageInterface::LANGCODE_NOT_SPECIFIED instead of 'und'.

@Wim Leers, #43:

(some hidden but understandable coupling there)

Well, actually the reason why we choose this approach was exactly to avoid coupling a core system with a module. In fact it should definitely be possible to test this by altering the bundle info via a test module alter hook + a state flag. However, since we are already using CT in the test, it's probably worth relying on it also for this :)

1. TIL! Done. ✅
2. See the rest of this test class: this is just in line with that. Another example is BigPipeRegressionTest().
3. D'oh, of course. Done. ✅

Saving credits

Committed and pushed to 8.7.x and 8.8.x, thanks!

Rolled this back on 8.8.x, it adds a deprecation error

Remaining deprecation notices (1)

      1x: \Drupal\Core\Session\AccountInterface::getUsername() is deprecated
in
Drupal 8.0.0, will be removed before Drupal 9.0.0. Use
\Drupal\Core\Session\AccountInterface::getAccountName() or
\Drupal\user\UserInterface::getDisplayName() instead. See
https://www.drupal.org/node/2572493
        1x in
JsonApiRegressionTest::testNonTranslatableEntityUpdatesFromIssue3043168 from
Drupal\Tests\jsonapi\Functional

I expect that will also fail on 8.7.x, watching the HEAD testing and will revert if so

8.7.x is good 🎉

D'oh!

Committed 5699dcb and pushed to 8.8.x. Thanks!