[META] Formalize translations support

Added related REST core issue.

Not sure if this is still relevant. But I took #2 and modified two lines so that it's possible to use the Accept-Language header.

        $acceptLanguage = $context['request']->headers->get('accept-language');
        $entity = $this->entityRepository->getTranslationFromContext($entity, $acceptLanguage);

When the header isn't set acceptLanguage will be null but the $langcode parameter of getTranslationFromContext is optional so it works out.

/me is horrible at naming variables

Actually without this I can't use jsonapi. All I have is multilingual sites.

Re-patching #2 against HEAD.

Just got a TypeError exception.

This looks good. It needs a test though.

When the header isn't set acceptLanguage will be null but the $langcode parameter of getTranslationFromContext is optional so it works out.

I believe the current patch does the right thing, as in, just follow what core provides us as a language, see #2135829: EntityResource: translations support for a discussion how to determine this language.

I'm working on some test coverage here.

Here is a test

This failure really doesn't make any sense :( So yeah this is certainly needs work.

@dawehner The tests are failing because they instantiate a JsonApiDocumentTopLevelNormalizer. Since that class's constructor has changed to additionally pass in an EntityRepositoryInterface, this parameter will need to be added in the tests as well.

@hampercm
Good point, well, what I don't get it why the -fail patch, with just the test changes includes, doesn't fail.

+++ b/tests/src/Functional/JsonApiFunctionalTest.php
@@ -192,31 +197,53 @@ class JsonApiFunctionalTest extends BrowserTestBase {
+   * @dataProvider providerTestRead

We are not providing mock data in the providerTestRead. We should find a less confusing technique to test the multilingual capabilities.

Additionally, most of these tests are testing business logic that is independent from multilingual stuff. I'm not sure about the value of duplicating the execution time for this. Maybe a multilingual specific test + Kernel test for the JsonApiDocumentTopLevelNormalizer would be more appropriate.

Good point, well, what I don't get it why the -fail patch, with just the test changes includes, doesn't fail.

Can it be that the EntityStorageBase::loadMultiple loads the entity for the appropriate language already? We are using that method to load entities for individual items and for collections.

Finally (and I'm sorry to provide this feedback now, since this code has been there 5 months already):

+++ b/src/Normalizer/JsonApiDocumentTopLevelNormalizer.php
@@ -160,7 +172,11 @@ class JsonApiDocumentTopLevelNormalizer extends NormalizerBase implements Denorm
+          $entity = $entity_repository->getTranslationFromContext($entity);

If we are to override the $entity object with a translation, we should do it right after we load the entity. In any case we need to make sure that we need to do this, since @dawehner's tests indicate that we do not. That is:
1. In the ParamConverter.
2. In the Controller for collections.

@dawehner's tests indicate that we do not. That is:
1. In the ParamConverter.
2. In the Controller for collections.

So right, this patch enables translations on collection level. It works already on the individual level.

We are not providing mock data in the providerTestRead. We should find a less confusing technique to test the multilingual capabilities.

Test coverage is a tricky topic. I started indeed with a dedicated method, but then thought we maybe want to follow the route of #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method which basically went the full multi-dimensional test coverage approach instead of the monte-carlo based testing approach, which tries to coverage everything but just choosing educated random points in the entire testing space.

For now I think keeping things simple, aka. a dedicated test coverage with a good name, is the better idea.

... So this time for real

+++ b/src/Controller/EntityResource.php
@@ -698,7 +709,8 @@ class EntityResource {
+      $translated_entity = $this->entityRepository->getTranslationFromContext($entity, NULL, ['operation' => 'entity_upcast']);
+      $output[$entity->id()] = static::getEntityAndAccess($translated_entity);

1. getEntityAndAccess() is called in multiple places. Would it make more sense to get the translated entity inside that method?

1. getEntityAndAccess() is called in multiple places. Would it make more sense to get the translated entity inside that method?

Good point. I totally think it makes sense.

I expanded the test coverage and figured out we have another place where we missed loading the right entity.

Overall, the patch from #24 looks good. Some comments:

1. +++ b/src/Controller/EntityResource.php
   @@ -715,6 +715,9 @@ class EntityResource {
   +    $entity_repository = \Drupal::service('entity.repository');
   

   Make the $entity_repository a static member so we can use DI, instead?

2. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -414,10 +418,38 @@ class JsonApiFunctionalTest extends BrowserTestBase {
   +    $included_tags = array_filter($output['included'], function ($entry) {
   +      return $entry['type'] === 'taxonomy_term--tags';
   +    });
   

   Good idea! There are other functional tests where this could be reused, too (note to self)

3. +++ b/tests/src/Functional/JsonApiFunctionalTest.php
   @@ -765,4 +813,21 @@ class JsonApiFunctionalTest extends BrowserTestBase {
   +  protected function setupMultilingual() {
   

   Moving the multilingual tests to a separate class might make sense. A benefit of this would be that the functional tests (which are typically the longest-running) could run in parallel

Thanks @dawehner and @hampercm. I'm gonna finish this one off!

+++ b/tests/src/Functional/JsonApiFunctionalTest.php
@@ -707,14 +739,21 @@ class JsonApiFunctionalTest extends BrowserTestBase {
+        $term->addTranslation('es', ['name' => $term->getName() . ' (es)']);

FYI, I'm going to change 'es' to 'ca'. For non-technical reasons ;-)

Make the $entity_repository a static member so we can use DI, instead?

I will skip this for now, since there is no unit test in that class yet. With kernel tests we can replace the service and be done. Is that ok?

I run this command to compare the speed to run the tests:

sudo -u www-data php ./core/scripts/run-tests.sh --clean && time sudo -u www-data php ./core/scripts/run-tests.sh --verbose --color --keep-results --concurrency "31" --types "PHPUnit-Functional" --php /usr/bin/php --url "http://d8dev.local" --sqlite "/var/lib/drupalci/artifacts/simpleteststandard.sqlite" --dburl "mysql://d8dev:d8dev@localhost/d8dev" --directory modules/contrib/jsonapi; cp /var/lib/drupalci/artifacts/simpleteststandard.sqlite /vagrant/simpleteststandard.sqlite

Before splitting: 46.33s
After splitting: 47.91s

So I don't think there are significant differences. However, I believe that the code is a bit cleaner after splitting.

I'll be merging if tests come back green.

Moving the multilingual tests to a separate class might make sense. A benefit of this would be that the functional tests (which are typically the longest-running) could run in parallel

Sure why not

Commit is was not showing up. This was committed in http://drupalcode.org/jsonapi/commit/?id=905e5e0

I'm a bit concerned, because IIRC there was no consensus yet at #2135829: EntityResource: translations support on how to support requesting translation. Quoting #2135829-22: EntityResource: translations support:

We should not go ways to support languages in the domain/path in any way. That's going against restful principles.

Yet that is what this patch introduced! I don't see this discussed here at all. We need to be very careful here.

Also, a review of the last patch:

1. +++ b/src/Controller/EntityResource.php
   @@ -715,6 +715,9 @@ class EntityResource {
   +    $entity_repository = \Drupal::service('entity.repository');
   +    $entity = $entity_repository->getTranslationFromContext($entity, NULL, ['operation' => 'entity_upcast']);
   

   Looks good!

2. +++ b/src/Normalizer/EntityReferenceFieldNormalizer.php
   @@ -86,7 +86,10 @@ class EntityReferenceFieldNormalizer extends FieldNormalizer implements Denormal
   +      // Get the referenced entity.
   +      $entity = $item->get('entity')->getValue();
   +      // And get the translation in the requested language.
   +      return $this->entityRepository->getTranslationFromContext($entity);
   

   Woah this is tricky!

3. +++ b/tests/src/Functional/JsonApiFunctionalBaseTest.php
   @@ -0,0 +1,280 @@
   +  /**
   +   * Performs a HTTP request. Wraps the Guzzle HTTP client.
   +   *
   +   * Why wrap the Guzzle HTTP client? Because any error response is returned via
   +   * an exception, which would make the tests unnecessarily complex to read.
   +   *
   +   * @see \GuzzleHttp\ClientInterface::request()
   +   *
   +   * @param string $method
   +   *   HTTP method.
   +   * @param \Drupal\Core\Url $url
   +   *   URL to request.
   +   * @param array $request_options
   +   *   Request options to apply.
   +   *
   +   * @return \Psr\Http\Message\ResponseInterface
   +   */
   +  protected function request($method, Url $url, array $request_options) {
   +    $url->setOption('query', ['_format' => 'api_json']);
   +    try {
   +      $response = $this->httpClient->request($method, $url->toString(), $request_options);
   +    }
   +    catch (ClientException $e) {
   +      $response = $e->getResponse();
   +    }
   +    catch (ServerException $e) {
   +      $response = $e->getResponse();
   +    }
   +
   +    return $response;
   

   This can be massively simplified, see \Drupal\Tests\rest\Functional\ResourceTestBase::request.

As I see it, we should not leave the code as is in the current state since there were translation inconsistencies where some things were translated and some not. Meaning that the user was still able to hit the language variant URL (/ca/jsonapi/node/article) before this patch and get some mixed results.

This commit/issue sets some honesty to that limitation. I agree that the URL negotiation is REST impure, but with this code we could have support for any language negotiation. Someone may add a contrib that negotiates language based on Accept-Language header, and with this code we support that.

That's going against restful principles.

I'm going to be the unpopular guy and say that we're not being RESTful purists to start with. Core does not achieve the RESTful holy grail either. The main difference is that core tries to. We are playing the practicality tune here, and I'd rather have this fixed in an imperfect way than wait on consensus in a 14 Nov 2013 issue. When that happens we need to see if it makes sense for us. Core outputs 1 entity, we output many, someone may argue for a different language for each (?) …

That being said, let's keep this as Active to show contributors that we are still open to adopt a more REST compliant resolution.

As I see it, we should not leave the code as is in the current state since there were translation inconsistencies where some things were translated and some not. Meaning that the user was still able to hit the language variant URL (/ca/jsonapi/node/article) before this patch and get some mixed results.

That's fair. But we could have changed it to never do any translation.

I'm going to be the unpopular guy and say that we're not being RESTful purists to start with. Core does not achieve the RESTful holy grail either. The main difference is that core tries to. We are playing the practicality tune here, and I'd rather have this fixed in an imperfect way than wait on consensus in a 14 Nov 2013 issue.

That's also fair. But it should've been discussed prior to commit. And now that it is committed, we should explicitly state this in the docs (i.e. in README.md).

Once it is documented, I think it's fine to close this.

We should not go ways to support languages in the domain/path in any way. That's going against restful principles.
Yet that is what this patch introduced! I don't see this discussed here at all. We need to be very careful here.

Well to be honest if you care about this restfull ness you can configure Drupal to take into account something else. The language negotation system is really flexible, so you could configure your site to be more pure.

Note: We've learned that the pure REST stuff doesn't serve us really well anyway, just look at HAL and its complexity.

@Wim is this something you want to take on?

This can be massively simplified, see \Drupal\Tests\rest\Functional\ResourceTestBase::request.

Maybe we can create a separate issue for it.

We still need docs on this.

Marked #2897230: [PP-1] Creating translations (via PATCHing entities?) as a duplicate of this

Let's just postpone this on #2135829: EntityResource: translations support.

We don't even need to block this on #2904423: Translated field denormalization creates duplicate values then, because that issue is about fixing a bug in the hal module's normalizers. Which don't affect JSON API: JSON API only reuses some of the serialization module's normalizers.

I'm not even convinced that that issue block us, since we don't use the same normalizations as HAL+JSON. Maybe @Wim Leers could do a little digging?

You were exactly right!

I omitted the [FEATURE] prefix … but it's marked as a task. Let's mark it as a feature request.

@e0ipso, do you have feedback on @gabesullice's proposal in #43?

I started working on this!

The initial patch was committed in #31.

Let's start by expanding the test coverage for what is working today.

Adds test coverage for all hitherto untested routes, to verify that translations are in fact respected there too!

With #49, we have at least basic test coverage for each of the routes (plus includes). This allowed me to attempt to remove either of the three getTranslationFromContext() calls that were added to JSON API in #31.

I was able to remove only 1 out of 3: the one in \Drupal\jsonapi\Normalizer\EntityReferenceFieldNormalizer::normalize()! This makes sense, because by the normalizer is given an existing entity object, and that should already be respecting translations. (This also happens to be the one I criticized in #33.2.)

The remaining two are in:

1. \Drupal\jsonapi\Controller\EntityResource::getAccessCheckedEntity() (necessary for loading the relevant translation after performing an entity query to get a particular translation)
2. \Drupal\jsonapi\ParamConverter\EntityUuidConverter (necessary when the entity object is a route parameter passed to the controller directly, for example the individual route)

I think I should be able to remove the latter too.

I was able to remove the getTranslationFromContext() call from EntityUuidConverter if it weren't for the test coverage that I added in #49. Already the test coverage is proving useful… 🙏

Having only getAccessCheckedEntity() call getTranslationFromContext() is not enough because \Drupal\jsonapi\Controller\EntityResource::getRelationship() does not call getAccessCheckedEntity().

So for now, we'll stick with the two remaining getTranslationFromContext() calls.

Next: trying to make #43 happen. @gabesullice made good points about why header-based negotiation is not a good fit, and why this is a place where core's pluggability/extensibility wrt language negotiation makes things harder: because we can't guarantee a certain DX.

This is what that would look like.

And that actually allowed us to remove EntityUuidConverter's getTranslationFromContext() too! Now there's only one getTranslationFromContext() left!

Before we go further in this direction, this needs to get reviews from @gabesullice and @e0ipso.

Further questions:

1. Do we simply not deal with POST, PATCH and DELETE for now?
2. How do we ensure we still keep the ability to add that in the future? (I don't think anything in HEAD or in the current patch prevents us from adding it in the future per se.)
3. Should we add support for sending multiple translations in a single JSON API document?

EDIT: quoting @e0ipso in #2987205-22: FormatSetter doesn't set the format to `api_json` when accessing just `/jsonapi`:

Oddly enough no one ever asked about more than that when it comes to translations.

That suggests the answer to my first question is "it's not worth doing right now".

This should fix most failures.

Questions still to be answered:

1. Plan for POST/PATCH/DELETE.
2. links entries pointing to other translations?
3. What happens when requesting a non-existing translation (for a particular entity or for a collection)?
4. What happens when requesting a non-installed langcode?
5. … probably more questions still?

Hello,

Here is a rebased version of the patch from comment 55.

I can't get an interdiff.

I need to test it.

Also what should be done for POST / PATCH / DELETE?

I need it for a multilingual project.

Also what should be done for POST / PATCH / DELETE?

What do you propose? How do you think it should work?

How I think it should work.

Create a new content in the specified langcode (or current langcode if not specified): POST jsonapi/node/article?lang_code=LANGCODE

Create a new translation/Update an existing translation (lang_code parameter mandatory): PATCH jsonapi/node/article/UUID?lang_code=LANGCODE

Delete a translation: DELETE jsonapi/node/article/UUID?lang_code=LANGCODE

Delete the whole entity: jsonapi/node/article/UUID

What happens when requesting a non-existing translation (for a particular entity or for a collection)?

Use the native behavior to display the entity in another language depending on languages weight.

For deletion of a non-existing translation, returns an explicit error message.

What happens when requesting a non-installed langcode?

Use the native behavior to display the entity in another language depending on languages weight.

If it PATCH / POST / DELETE operation on a non installed langcode, returns an explicit error message.

@gabesullice we should probably use the first applicable active language negotiator on the site. Applicable in the sense of being a URL based negotiator, but I'm unsure how to detect that a negotiator is URL based.

Actually, like I've said since the very beginning: I am concerned about using Drupal's language negotiation system. That system is intended for web sites, not for HTTP APIs. For a HTTP API, we IMHO want it to be documentable, and not for it to have dozens of possible permutations of how it might work depending on how some module is configured.

Am I the only one who feels this way?

AFAICT, the language negotiation stuff is pretty much just a menu of workarounds for browsers being bad

I disagree with this.

The language negotiation system used on a specific website depends on the locale: in Belgium it's common to see company.be/nl and company.be/fr. I've got the impression that in the primarily Anglo-Saxon countries (U.K., U.S., Australia, Canada, and others), it's based on TLD: company.co.uk, company.com.aucompany.ca and company.com.
This makes sense for those cases where the URL is the interface used by entire non-technical peoples. Because in that case, it's essentially joint conventions — much like hyperlinks are always underlined.

But in the context of HTTP APIs, the user is not a non-technical one, but a developer. And hence in this case, we need less flexibility and more predictability.

Heh, fair :)

@e0ipso, can you explain your rationale for why you think using Drupal's existing language negotiation system does not cause DX and maintainability problems & risks?

Sorry to hijack the issue, to expose a practical real problem related to the discussion:

We are using JSON:API to fetch content for a Gatsby multilingual site.
We are relying currently on the "implicit entity translation magic" that core does. So we have 2 endpoints in Gatsby config: one for Finnish/fi/jsonapi, one for English content /en/jsonapi.
Gatsby recursively will grab every single entity exposed via JSON:API by default, in 2 iterations. The order is important, as you will see:
1st) in Finnish /fi/jsonapi
2nd) in English /en/jsonapi
It collects all the data into a single big JS variable, indexed by "language-id" (if it would be indexed only by id, it would get overwritten)
Setup explained here https://github.com/gatsbyjs/gatsby/issues/10020

This setup works perfectly if all the articles are translated. But if an article is only provided in Finnish, then the second iteration Gatsby does (fetching articles in English), will ALSO get the Finnish version (because of the un-wanted language fallback). This will result in duplicated content in Gatsby, by indexing two nodes with the same id:
fi-98303 (Finnish article)
en-98303 (Finnish article)

To fix this problem we tried a simple solution: append ?filter[langcode][value]=fi for every request that Gatsby does, but it didn't work because:
1) There are many entities that will throw error Invalid nested filtering. The field 'langcode', given in the path 'langcode, does not exist
2) Paragraph entities are not filterable by langcode (even if they have the attribute). They just return an empty array. (no error)

So our current solution is a hack to detect in Gatsby Drupal plugin code if it's fetching URLs that contain the string /node/. Then, we add the langcode filter. This works well for nodes, and taxonomy terms so far.

It would be good if JSON:API module would offer a simpler way to avoid fallback translations.

Thanks, @corbacho! 🙏 Much appreciated, that concrete use case helps ensure we will settle on a solution here that takes that use case into account 👍

I would like to share a rough solution I thought yesterday to fix node translation in our multilingual site project. We use different endpoint to "PATCH" node in different language:

• /it/node/1095?_format=hal_json, to update original node
• /en/node/1095?_format=hal_json, to update english translation

In our site the default language is IT and we noticed that: if the node has no EN translation the PATCH request overwrite the IT node values. So I decided to intercept the jsonapi call and check if that node has a translation, if not, I create it.

Probably you can suggest me a better way to fix that. Here is my event subscriber snippet:

<?php

/**
 * @file
 * Contains \Drupal\sv_jsonapi\EventSubscriber\TranslationSubscriber.
 */

namespace Drupal\sv_jsonapi\EventSubscriber;

use Symfony\Component\HttpKernel\Event\GetResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Drupal\node\Entity\Node;

/**
 * Event Subscriber Transaltion.
 */
class TranslationSubscriber implements EventSubscriberInterface {


  /**
   * Code that should be triggered on event specified
   */
  public function onRequest(GetResponseEvent $event) {

    // Only preload on json/api requests.
    if ($event->getRequest()->getRequestFormat() == 'hal_json') {
      $default_language = \Drupal::languageManager()->getDefaultLanguage()->getId();
      list(,$language,,$nid) = explode('/', $event->getRequest()->getPathInfo());

      if (empty($language)) {
        $language = $default_language;
      }

      if ($language != $default_language) {
        $node = Node::load($nid);
        if (!$node->hasTranslation($language)) {
          \Drupal::logger('sv_jsonapi')->debug(
            "Node with ID @id has no '@lang' translation: create it!", [
              '@id' => $nid,
              '@lang' => $language,
          ]);
          $node->addTranslation($language, ['title' => $node->label()])->save();
        }
      }
    }
  }

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    // Set a high priority so it is executed before routing.
    $events[KernelEvents::REQUEST][] = ['onRequest', 1000];
    return $events;
  }

}

#72: thanks for sharing that! 🙏 But it's about the hal_json format provided by core's rest module, not the jsonapi contrib module :) Feel free to leave that comment here, but could you repeat it in #2135829: EntityResource: translations support? Thanks!

This is still very valuable to keep here, because it highlights exactly the reason why I want to make sure we have a plan for not just reading translations, but also writing translations. Because otherwise edge cases like the one you encountered won't be taken into account and may be impossible to solve without breaking backwards compatibility.

Just to add my usecase here:
It would make sense to also include a property that indicates which translations of a page are available. If I built a language switch that would be really helpful to only show available translations there. And not having to make an extra request for that.

It would make sense to also include a property that indicates which translations of a page are available.

Absolutely! The good news is that that's exactly what #2994193: [META] The Last Link: A Hypermedia Story is about! IOW: for any given entity, there'll be links to the other available translations, much like content_translation_page_attachments() does for HTML responses _{Many details still to be worked out, especially for cases like possible translations but that don't exist yet, or translations with automatic fallbacks (e.g. U.K. English falling back to U.S. English).}

It's great to have explicit use cases listed here, so don't be shy, keep commenting :)

@gabesullice

would it be possible to make Gatsby work using an Accept-Language header and not use a different URL/query param per language?

Yes, it could work. Gatsby drupal plugin is implemented with axios, so it's a matter of adding the header to the axios request.
A minor negative consequence would be that it makes impossible to see/debug JSON API language issues directly in the browser, true? you would need Postman, or some tool to see JSON API responses per language, but it's acceptable.

This issue was in desperate need of an issue summary update!

Also marking this a "plan" issue, making this a sibling of #2795279: [PP-2] [META] Revisions support.

I think this may be a more accurate title.

Yesterday I discussed this issue with @Wim Leers, @gabesullice, and @effulgentsia, and we came up with the need for at least #3037804: Document the extent of JSON:API's multilingual support. and

I've since discussed this issue with both @larowlan and @gaborhojstsy. Since core REST suffers from similar problems, we are leaning toward making an exception to policy and adding JSON:API to core even with these feature gaps in place.

Some additional concerns we have before core inclusion (via @gaborhojtsy):

• What "contracts" does JSON:API want to keep for multilingual functionality, since they need to keep BC with these bugs for X (define X, if we know that is only until D9 do we have something to mark deprecated?)
• Are there code paths that need @todos added?
• Is there tests to cover that this is how broken it is and to the ensure we keep supporting how broken it is now until we promise we do, etc.?

What "contracts" does JSON:API want to keep for multilingual functionality, since they need to keep BC with these bugs for X (define X, if we know that is only until D9 do we have something to mark deprecated?)

It wants to keep everything working that happens to work automagically today.

Are there code paths that need @todos added?

No, that's what we have this issue for.

Is there tests to cover that this is how broken it is and to the ensure we keep supporting how broken it is now until we promise we do, etc.?

No. What works only happens to work due to magic in core's entity route param converter.

Disclaimer: all the following reasoning is based on the IS, I didn't have a look at the code and I'm not familiar at all with it, so take it for what it's worth :)

@xjm asked me to have a look at this with particular focus on data loss/integrity issues. I think an obvious candidate for more scrutiny is the following remaining task:

Plan for POST + PATCH + DELETE.

In particular, I think it's concerning that we are silently applying fallback rules while performing update and delete operations. Drupal's HTML UI does apply fallback rules as well, but performs a GET first, so users have a chance to notice that they are dealing with a translation not matching the content language: both on the entity form and the deletion confirmation form this is explicitly signaled. In this case we might be happily performing changes to the wrong target without the client even noticing it, until the next GET.

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.

I'm wondering whether a possible mitigation strategy while we figure out a proper way forward (the solution proposed in the IS makes sense to me FWIW), could be to ignore the fallback logic in controllers responsible to handle the PATCH and DELETE requests and return a 404 if an explicit check reveals that there is no translation matching the negotiated content language.

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

(Writing this on my phone.)

I realized after posting my last comment that I hadn't said I'd be happy to work on test coverage for this.

#82: Note that JSON:API behaves *identically* to core's REST module wrt translations. I'd be fine with implementing the mitigation strategy you propose, but note that some sites (I expect it to be an extremely tiny number though) may be relying on the current automagic behavior.

Note that JSON:API behaves *identically* to core's REST module wrt translations.

I'm not sure whether that's a good reason to add another way to break things, if that's actually the case, of course :)

I'd be fine with implementing the mitigation strategy you propose, but note that some sites (I expect it to be an extremely tiny number though) may be relying on the current automagic behavior.

Could it be possible to opt-out via a setting or somehow conditionally enable the fix?

Btw, tests to confirm that what #82 is describing actually happens (or not) would be more than welcome :)

Thanks @Wim Leers and @plach.

Re:

Are there code paths that need @todos added?

No, that's what we have this issue for.

For core, our best practice is to place @todos referencing the issue node in the relevant code paths and in the tests where the behavior needs to be changed. So it would be good to add those if possible.

Do we have core criticals already for the REST translation issues?

I think it's worth considering #83 and #84 (although #84 may be tricky if configuration is disallowed). However, we've made changes before to deny access for update operations that worked sometimes but caused data loss other times, as temporary workarounds until criticals are fixed. Either way, it sounds like maybe we'd want to have issues to do the same to REST as well -- or maybe there is a way to patch core to temporarily mitigate both until it's fixed?

Tests are definitely a good first step either way.

I completely forgot that we already do have explicit test coverage for what is supported WRT translations! 😅Hurray!

See \Drupal\Tests\jsonapi\Functional\JsonApiFunctionalMultilingualTest, which has existed since January 2017, it was committed in #31. The issue was reopened shortly after that due to concerns about relying on the automagic context-dependent translation loading.

Working to expand that now.

(the solution proposed in the IS makes sense to me FWIW)

Thanks for saying that in #83, that's great to know for the future, when we make JSON:API's translation support feature-complete :)

Working to expand that now.

Done: #3038308: Avoid translations DELETE data loss and unintended changes with PATCH and test all methods against entity route parameter translation upcasting. A sequence of tiny interdiffs with focused tests per use case is awaiting, and I’ve posted my analysis + proposals: https://www.drupal.org/project/jsonapi/issues/3038308

#3038308: Avoid translations DELETE data loss and unintended changes with PATCH and test all methods against entity route parameter translation upcasting landed, after @plach RTBC'd it! 🎉

Comment deleted, move to separate issue as asked in #91.

#90/@Niklan: Could you please post that as a separate issue? This issue is for planning, not for concrete questions. Thanks!

During DrupalCon we talked about adding hypermedia support for translation. Ideally we would have a standard way to link to the translation versions of an entity in the document.

Copying from an initial slack conversation

"attributes": {
        	"name": [
		      {
		        "data": "Top",
		        "locale": "en"
		      },
		      {
		        "data": "Débardeur",
		        "locale": "fr"
		      }
		    ]
		} 

"So i have loosely looked at Spryker and Akeno. It looks like their implementations are slightly different in terms of dealing with different locals. Akeno uses the following format within attributes which i think mostly conforms to the JSON:API specification? although i'm not sure if having data as an object property is right? If we were to implement this type of approach then we would probably have to refactor the `GET` logic so it matches with this format. This would work for when we create `POST` and `PATCH` requests as we would know which entity the translations get attached to. This definitely feels like it could work.

In Spryker they use an `Accept-Language` header that's used to get an entity for that language, this is kind of similar to how Drupal's JASON:API currently works albeit the difference being that the url has the language code in it. (@e0ipso mentioned that Drupal can be configured to do this too). Obviously if we were to use a `POST` request to create a 'French' version of an 'English' entity then one of the main issues is that we don't know which entity to attach the translation to unless we send some sort of relationship identifier in the request. Can we do this with the JSON:api spec? If not this might mean instead we use PATCH to create/update languages against an entity via the correct language endpoint or Accept-Language header. One thing to note is that we aren't actually creating new entity instances because we are just updating an entity although again this doesn't feel 100% correct. What are your thoughts?"

I created a sandbox which is a decoupled module for JSON:API https://www.drupal.org/sandbox/kingmackenzie/3061283, similar in the way JSON:API Extra's is working in terms of overriding Normalizers but purpose-built to handle entity translation. It's inspired based on patches in this issue: https://www.drupal.org/project/drupal/issues/2135829

Filtering does not seem to take current language into account.

I have a news node whose field_slug is dog in English and hund in Swedish. When I use JSON:API with filter[field_slug]=hund then that news node is in the result regardless of what language I call JSON:API in (i.e. both for /jsonapi/node/news and /sv/jsonapi/node/news).

I have a client who wishes to push translations in JSON:API forward and is willing to sponsor me to work on this.
To get to the actual (development) work, we first need to agree on what is the best way forward.

Below is my proposal on a way forward. Where applicable/possible I will try to explain why I've made specific choices that might differ from what was discussed (but not really agreed upon) in this very issue.

1) Use a query parameter for translations, let's stick with the already proposed lang_code=[2-letter-langcode].

I've decide not to go with the discussed Accept-Language header. After a brief discussion with @gabesullice yesterday, we both think that the chance that CDNs/Proxies don't use/accept this specific header is too big.

2) One URL to rule them all!

I propose to drop all the fallback magic used for the HTML side of Drupal, meaning we ignore all the language negotiation logic that's in place on a site. Which would mean that the endpoint URL /jsonapi/foo/bar/ will consistently be used for any kind of JSON:API call, in the case of working with translations with the above mentioned query parameter lang_code=[2-letter-langcode].

As far as I can see this is the only way to get a consistent endpoint for any Drupal multilangual site no matter what Content language detection method is used on that site.

Also by not using the fallback logic we can get rid of the use of ContentEntityBase::getTranslationFromContext (which returns a fallback if a node doesn't have a translation) and use ContentEntityBase::getTranslation which only returns an entity when there is a translation in the requested language available. If not, an Exception is thrown.

The fallback magic is very useful when serving HTML pages and returning a 404-page will make the user experience bad, but in JSON:API land, where the nerdy cool kids hang out, I think we need a binary answer. You ask for a specific translated entity, you either get it or a "Does Not Exist", nothing else.

3) Return a list of available translations in the meta of each entity, need validated by @bbuchert's comment in #74 and proposed by @gabesullice in #61

{
  "links": {
    "self": {
      "href": "https:/my.example.com/jsonapi/foo/bar/{{some-uuid-here}}",
      "meta": {
        "hreflang": "nl,fr,de",
      }
    }
  }
}

4) There will be no opt-in for this new behaviour described above.

This will be BC-breaking, but only for users already working with the current JSON:API for translations. Usage of JSON:API without the translations will be "Business As Usual". The added meta in 3) should not be breaking anything.

The reason for not using an opt-in is because the current translation JSON:API is flawed at best (See for example #98 and Language negotiation doesn't respect langcode filter). IMHO that would mean most/all people wanting to use the "new" translation JSON:API will opt-in anyway.

Also we would have the (again IMHO) "nasty" situation of supporting 2 solutions for the same functionality, where one is better than the other. Let alone this being confusing for users/developers and most probably create a lot of noise with support topics on d.o.

On top of that, if we do choose for an opt-in, at some point we have to deprecate one of the 2 solutions anyway, so I think we need to bite the bullet, and make the people who are currently using translations in JSON:API to update their code. (Side-note: By the looks of it people are forced to jump through hoops with the current setup anyway, and might end up with cleaner code, see for example #69)

Disclaimer: I am by no means a JSON:API nor multilingual expert, so there might/probably will be gigantic errors/loopholes in the above proposal. That's why I call upon "The Big Brains" around here to look and shoot at this proposal.

After we reach a consensus on the above, I plan to write up a proposal on the POST + PATCH + DELETE, which should be a lot easier when the above is formalized.

Then we (read I...) break up this whole monolith in sub-issues and we can move forward and get this done! (#FamousLastWords)

Because of the need to add translations using the JSON API, I started investigating and came with a solution. I hope it's a start to get this into core.

The JSON:API documentation states that a resource object is unique defined by a type and an id and since the uuid is the same for all translations of a resource I decided to add the translations using the PATCH method instead of the POST method. I see the translations as part of the resource object and not a different resource object.
To know if a translation needs to be created I use the langcode attribute (only if the entity type is translatable). If the langcode attribute is set, I create a new translation (if it doesn't exists already). Otherwise I change the loaded entity to the existing translation.

If this needs to be addressed in another ticket, please let me know.

When I was reading the proposal of Spokje the first thing i though is it would be awesome to know the available languages for a resource. I saw in #61 @gabesullice made a point on this. But couldn't we just use a convention for that?

{
  "links": {
    "self": {
      "href": "https:/my.example.com/jsonapi/foo/bar/{{some-uuid-here}}?lang_code=nl",
      "meta": {
        "hreflang": "nl",
      }
    }
    "self:fr": {
      "href": "https:/my.example.com/jsonapi/foo/bar/{{some-uuid-here}}?lang_code=fr",
      "meta": {
        "hreflang": "fr",
      }
    }
    "self:de": {
      "href": "https:/my.example.com/jsonapi/foo/bar/{{some-uuid-here}}?lang_code=de",
      "meta": {
        "hreflang": "de",
      }
    }
  }
}

So we at least get the value of knowing what languages are available for that content?

This might get a little confisuing though when looking at recourse lists in a certain language, since you wouldn't include resources that are not included in the selected language...

Just my 2cts

It might be a good idea to make it possible to check access without actually translating the entity (either with getTranslationFromContext or the langcode argument).

Use case: jsonapi_views would be able to return different language versions in the same listing and still check access (see #3175437: Translations not applied).

1) Use a query parameter for translations […]

👍

2) One URL to rule them all! […]

💯

Also by not using the fallback logic we can get rid of the use of ContentEntityBase::getTranslationFromContext (which returns a fallback if a node doesn't have a translation) and use ContentEntityBase::getTranslation which only returns an entity when there is a translation in the requested language available. If not, an Exception is thrown.

💯

3) Return a list of available translations in the meta of each entity, […] proposed by @gabesullice in #61

But didn't @gabesullice in #61 point out a problem with this approach? Plus, it's only for header

4) There will be no opt-in for this new behaviour described above.

This will be BC-breaking, but only for users already working with the current JSON:API for translations. Usage of JSON:API without the translations will be "Business As Usual". The added meta in 3) should not be breaking anything.

… as I was responding to the first 3 points, I was very worried about backwards compatibility. Fair point that the current design is broken. (I think I've always argued the same too, actually, and I think I was opposed to even adding this at all originally?) And it's indeed actively causing problems as you can see for example in #69's write-up.

But even then, we just cannot break BC. I do think that we can simply mirror the existing behavior by creating an additional event subscriber that is only enabled by default on existing sites, and it does continue to run the current ::getTranslationFromContext() behavior and then does a 301 redirect to the new URL. Then in Drupal 10 we'll be able to simply drop this event subscriber! This means all weirdness would be concentrated in a single place.

#101:

1) Yes, unfortunately, […] but I don't think the internet is ready for it (see @Wim Leers comment #35.

Well … as the author of said comment … I do want to point out that this means we're choosing not to pursue the technically best solution for the sake of a single use case: anonymous JSON:API instances served over HTTP/1 without TLS, and even then only for users behind a crappy proxy. It is fair to question whether we want to support that scenario at all.

b) not very well supported naive JSON:API clients that manually construct links using a "base path"

I think this is the best argument.

Are 2-letter langcodes what we want? If so, why? (it might be as simple as "that's what Drupal supports", I honestly don't remember)

I think we should simply use langcodes that hreflang prescribes: https://en.wikipedia.org/wiki/Hreflang. That means languages (ISO 639-1), countries (ISO 3166-1) and potentially also script variations (ISO 15924). We should definitely pay close attention to complying with these specs (and specifically with RFCs defining the behavior of hreflang) rather than exposing Drupal's internal defaults.

I got some funding to work on this and got in touch with @Wim Leers and @gabesullice. We had a call last Friday during which we discussed how to bring this forward. The main aspects we discussed were how to keep BC and how to handle the existing language negotiation logic.

We agreed that a possible way forward is to introduce a new experimental module (JSON:API Translation), that would implement all the new/missing logic. This would allow people to opt-in without requiring any additional configuration. The goal would be to deprecate the legacy (GET) logic and merge the module into JSON:API in D10.

Implementation-wise we agreed that it should be ok to rely on the Accept-Language and Content-Language headers for the reasons stated at #3028501-35: Enable header-based proactive content negotiation with optimizations and opt-outs available.

@gabesullice and I had a follow-up call during which we drafted a "spec" proposal for the entity translation API exposed by the JSON:API Translation module. You can find it at:

https://github.com/gabesullice/drupal-jsonapi-translations/blob/master/i...

I'll start working on the implementation outlined over there, so that we have something to look at, but of course feedback on this proposal would be welcome :)

From https://github.com/gabesullice/drupal-jsonapi-translations/blob/master/i...

* Existing article, non-default translation, using the canonical URL:

GET /jsonapi/node/article/{id}
Accept-Language: fr

// If translation exists:
200 OK
Content-Language: fr
Content-Location: /jsonapi/node/article/{id}?lang=fr

// If translation does not exist:
// option a)
// provide a default without redirecting
200 OK
Content-Language: en

// option b)
// do not provide a default or redirect, simply provide alternatives
406 Not acceptable
Link: </jsonapi/node/article/{id}?lang=en>; rel="alternate" hreflang="en"
Link: </jsonapi/node/article/{id}?lang=nl>; rel="alternate" hreflang="nl"
{links: {// same links, in json}}

// option c)
// redirect to a default and also provide alternatives
300 Multiple choices
Location: /jsonapi/node/article/{id}?lang=en
Link: </jsonapi/node/article/{id}?lang=en>; hreflang="en"
Link: </jsonapi/node/article/{id}?lang=nl>; hreflang="nl"
{links: {// same links, in json}}

Gabe and I would both go with option C, since that would both inform clients that the translation they requested is missing and allow them to decide whether performing the suggested redirect (to the default translation) or applying their own fallback logic.

#108: +1 to option C 👍

Thoughts:

• One addition to the Existing article, non-default translation, using a translation-specific URL: case, if possible: it'd be cool if we'd be able to still know when a translation used to exist and in that case not respond with 404 Not Found but with 410 Gone.
• Why does POSTing a translation result in Location: /jsonapi/node/article/{id} without the ?lang query string? — ah because we're creating a new entity in a non-default translation!
• The "Content-Language" header is just an alternative way to specify the entity "langcode" field. sounds cool but … do they have the exact same representations? Does Drupal core's langcode field conform to the same spec/does it use the same values as the Content-Language header? It'd be good to double-check.
• Is the If no "langcode" and no "content-language" header are specified, the default logic configured at "/admin/config/regional/content-language" is applied. statement not equivalent to New article in default language:?
• OMG THIS LOOKS SO GREAT! 🤩🥳 I'm so glad this is happening now! 💪😄

P.S.: this is the first time I've ever seen 300 Multiple Choices 🤓

Here is a first patch for this. I was wondering whether a new issue would be in order, but actually all the relevant discussion is here and patch is not huge, so it maybe worth turning this meta into an operative issue. Reviews welcome :)

The patch defines a new experimental module which overrides the JSON:API controller with a translation-aware one. The language negotiated by core is simply ignored and all the actual translation negotiation logic is based on the spec draft. The main difference with the latter is that the query string parameter is not lang, which apparently is not JSON:API-compliant, but lang_code.

Also, wrt #108, the behavior implemented so far was A. This is of course not set in stone, but @Berdir brought up that not doing A could introduce performance issues, due to additional requests, especially when dealing with listings.

Listings is an area that the current patch does not address, since it wasn't covered by the spec Gabe and I came up with. Personally I think that we could use the freshly-introduced concept of translation as a standalone resource to provide language-specific listings:

• GET /jsonapi/node/article would return a list of all nodes respecting the current Accept-Language header + fallback to the default translation, basically option A from #108.
• GET /jsonapi/node/article?lang_code=fr would return a list of French translations (including default translations, i.e. articles originally created in French). Articles with no French translation would not be listed.

I think the latter may address the use case presented in #69.

I did not work on test coverage yet, since I have a limited budget, this is my next task unless there are big concerns with the current patch. People wishing to test the patch may use the Postman collection I used and the attached DB dump, if they do not wish to set an installation up. Please remember to configure the collection variables if you use it.

@Wim Leers, #109:

Sorry, I missed your comment, so the patch does not include any of the following, but here are a few replies:

1: If versioning is enabled, it may be possible to tell whether a previous version had a translation that now is missing, however IIRC with pending revisions it may not always be possible to distinguish the case where a translation was removed from one where it was added but not published yet. Worth investigating more.
3: Content-Language relies on the language tags specified in BCP 47, which in turn relies (also) on 2-digit ISO-639 language codes, which are the language codes of predefined languages exposed by the language subsystem. The latter is configurable, so actually it is possible to install custom languages with invalid language codes, but I don't think should be a concern of ours.
4: It depends :) By default yes, however the site default language may be different from the one resulting from the default logic configured in the content language settings. For instance that may hard code a specific non-default language as the default content language. OTOH this logic would apply for all entities, regardless of how they are created, if the language value is missing, so it is basically what have been happening so far already.

Re #108, according to MDN, option A is recommended:

If the server cannot serve any matching language, it can theoretically send back a 406 (Not Acceptable) error code. But, for a better user experience, this is rarely done and more common way is to ignore the Accept-Language header in this case.

I forgot to mention this:

+++ b/core/modules/jsonapi_translation/jsonapi_translation.info.yml
@@ -0,0 +1,8 @@
+dependencies:
+# TODO Should we add this?
+#  - drupal:content_translation
+  - drupal:jsonapi

+++ b/core/modules/jsonapi_translation/src/Routing/Routes.php
@@ -0,0 +1,147 @@
+    $translation_creation_route = new Route($individual_route_path);
+    $translation_creation_route->addDefaults([RouteObjectInterface::CONTROLLER_NAME => static::CONTROLLER_SERVICE_NAME . ':createIndividualTranslation']);
+    $translation_creation_route->setMethods(['POST']);
+    // TODO Allow users with translation permissions and no edit permissions to
+    //   handle translations.
+    $translation_creation_route->setRequirement('_entity_access', 'entity.update');

Currently only users with update entity access can perform CUD operations. I think we should allow translators, i.e. users not having update access to create, update, delete non-default translations (based on their permissions). These permissions are defined by the Content Translation module, so maybe it should also define the JSON:API routes to perform these operations? Or would it make more sense to make JSON:API's access control alterable so that CT can tweak it?

Thanks, in the meantime I'm posting a minor update to fix an issue that was preventing the patch from working on case-sensitive file systems.

And now with 100% more patch attached!

This should fix cache support for GET requests. Now I think it might make sense to split this into a few subtasks.

Updated IS.

I will create a couple of children issues and move the related patches over there.

Small fix to address some edge-cases related to UUID param conversion.

Be sure to rebuild caches after applying the patch.

Created child issues:

• #3199696: Add language support to ResourceObject
• #3199697: Add JSON:API Translation experimental module
• #3199698: [PP-1] Add translation support to JSON:API collections

I tried the latest patch at #126 on a project that I'm working on and it seems that the included fields that I've added to the default include list via the jsonapi_extras do not work, when the jsonapi_translation module is enabled.

It seems the patch doesn't apply anymore. Also #123 has a lot of feedback and i think that feedback has not been applied, so i'm setting this back to needs work.

#3199696: Add language support to ResourceObject has been committed :)

Un-postponing #3199697: Add JSON:API Translation experimental module

This is a little bit confused, the final question still on the table: How to update an existing entity translation using Json:API PATCH method?
What headers/meta/attributes... should developers use in their PATCH request to determine the concerned translation after applying #126 patch?
I think a clear documentation is also needed for this topic.
Thanks guys

I am new to the topic and was able to add new translation by commenting out those lines from #126 patch:

    if ($resource_langcode !== $parsed_langcode) {
      $message = 'Translation resource language mismatch: "%s" (request metadata) vs "%s" (request payload).';
      throw new UnprocessableEntityHttpException(sprintf($message, $resource_langcode, $parsed_langcode));
    }

@b.khouy you can add transaltion either by using queryParam "lang_code" or by setting POST request "Content-Language" header, of course both set to translation destination language. Sending PATCH request to whatever entity endpoint language modifies default language.

Hey guys,

What is the current state of play for this issue? We have Drupal 8.9.2 and are using JSON:API to create English nodes, but we need to create translations of those nodes into Welsh. The Content Type is correctly set up to allow translation, and we can of course create a translation with the GUI, but is this now possible using the JSON:API module or via some other means?

Can any of these patches here be successfully applied to deliver such functionality? I've tried patching in the drupal/core section of my composer.json but the patch won't apply. Is that because it is now in the core or is it because the patches no longer work with the current version I have as part of Drupal 8.9.2?

Any pointers will be greatly appreciated

Cheers

Sam

Guys

Any update at all - can REST API actually create translated nodes and establish the relationship between the original node and the translated one?

If not are there any work arounds?

Surely someone out there knows something?

Thanks

Sam

Hi Sam - This is not an area I'm actively working on, yet I can speak to your repeated requests for an update. Drupal is maintained by open-source contributors and there's no dedicated paid developers working on JSON:API specifically. You mention "REST API" which would be something different from JSON:API module, but i'm guessing you're talking about the items in this issue. There is some work on this already, as you can see on this issue... but it's waiting on people (maybe people like you?) to help finish it up.

I can say that if there are ever updates, you'll see them here. If this is important to your use case, you can help us get this done, or I'm sure there are developers experienced in this area who would be open to doing sponsored work.

A little note about this we need to take into consideration. If we add a query parameter, the JSON:API specification has some rules around them. They need to be a valid member name, but also contain a special character. See:

https://jsonapi.org/format/#query-parameters-custom

Per #143 My read is that it must contain at least one non a-z character, but a capital letter suffices.

It is RECOMMENDED that a capital letter (e.g. camelCasing) be used to satisfy the above requirement.

So not much of a strong requirement but something we should enforce.

Wow you're right. Yay! :)

@bbrala @bradjones1

I resumed work on this today at the DrupalCon Lille sprint. Regarding #143 - #144, the latest code uses a langCode parameter. That looks compliant, doesn't it?

yeah langCode is fine.

Would've loved to be able to sit with you, but the fact this week is the kids vacation made that too hard :(

@gabesullice

Addressed your reviews at https://www.drupal.org/project/drupal/issues/3199697#mr1591-note220760