Add JSON:API Translation experimental module

#3199696: Add language support to ResourceObject is in review which should unblock this.

#3199696: Add language support to ResourceObject has been committed. No longer postponed :)

Ooohhh! 🤩

Todo from the parent issue

See comments in the parent issue: https://www.drupal.org/project/drupal/issues/2794431#comment-13935684

1. #124.2: This needs to be addressed. But this needs some changes, since request attribute is not something we want to use.
2. #124.4: Needs to be addressed.
3. #124.8: Needs to be addressed.

Probable issue in regards to using request attributes

Also the implementation at #3199696: Add language support to ResourceObject has the following comment by @larowlan (and @bradjones1 for that matter).

Larowlan:
@bbrala asked a core committer to chime in here, and I'm in the same camp as @bradjones1 on this.
Storing stuff in the global request will lead to people relying on it (if people can do weird things, they will). It could even result in someone mutating it.
Unless I'm missing something, it doesn't seem related to this issue either, so the increased scope will impact its ability to land.
If you feel its a useful pattern, my advice would be to split it into a new issue so we can debate the merits independently of this issue.

So i the end we removed that part of the patch. We have not discussed this further, so there might still be merit. But i feel this is probably a dead end.

Missing tests

Eventually we do needs tests also :)

#124.2: I'm removing the whole request attribute part of the change.

#124.4: I don't think this can happen since it checkes the languages supllied to the header language it seems? But this is a good candidate for a test. Added to tasks in IS.

Whats left by @gabesullice

1. +++ b/core/modules/jsonapi_translation/src/Controller/EntityResource.php
   @@ -0,0 +1,628 @@
   +  protected function buildWrappedResponse(TopLevelDataInterface $primary_data, Request $request, IncludedData $includes, $response_code = 200, array $headers = [], LinkCollection $links = NULL, array $meta = []) {
   +    $translation = $request->attributes->get(static::ATTR_TRANSLATION_RESOURCE);

   I know that this attribute will not be set on collection requests, but can we add a guard like this so that we never accidentally apply this to collection responses?

2. if ($primary_data instanceof Data && $primary_data->getCardinality() !== 1) {
     return parent::buildWrappedResponse($primary_data, $request, $includes, $response_code, $headers, $links, $meta);
   }

   (We should probably add getCardinality to TopLevelDataInterface while we're at it so we don't need a check against the concrete class, but that could be a follow up)

My editor is not being coorperative. Have not removed the resource attribute.

Going through the code the following todo's are in the code:

1. Routes.php: Allow users with translation permissions and no edit permissions to handle translations.
2. jsonapi_translation.info.yml: Should we add this? - drupal:content_translation

Other todo's are dependend on other issues.

<3

THANK YOU SO MUCH, @plach! 🙏🥳

Posted an initial very high-level review, looking at the broad strokes of this impressive (and big! 2000 LoC!) MR. 🤩

Completely reviewing this requires understanding lower level pieces of our JSON:API implementation, knowledge of which has evaporated my brain over the past half decade 😅😶‍🌫️

For now, I'm especially interested in understanding why this is not extending ResourceTestBase but JsonApiFunctionalTestBase. It might be fine/safe, but I'd love to A) understand it, B) have the rationale written down somewhere 🤓

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Only way i can see that happen is if we integrate these tests into resourcetestbase and adjust the jsonapi tests. But testing an experimental module in the main jsonapi module kinda feels weird. :x

Since i worked on this issue earlier, i'll keep it in the back of my mind to do a round of tweaks on.

Rebased and refactored a lot, and did all the feedback.

Unfortunately i ended up in a testfailure in this test:

    // Check that translations cannot be created for non-translatable nodes.
    $node = $this->drupalCreateNode();
    $output = $this->doTestNewTranslationRequest($node, 'it', Response::HTTP_UNPROCESSABLE_ENTITY);
    $this->assertSame('Translation is not enabled for the specified resource.', $output['errors'][0]['detail']);

This should be the result of refactoring the checks on enities to a check on the resourcetype. Might be because it not uses the entity type instead of the entity itself in checkEntityTranslatability (now renamed to checkResourceTypeTranslatability).

Time for today is up unfortunately.

Just a note. I did not handle the changes in the tests as discussed in #24

One of the mentioned issues is #2430335: Browser language detection is not cache aware. Did some work there to see if that can actually get fixed sometime soon.

All green :)

I see that tests have been added so scratched that task out.

Would you all say the other tasks have been addressed?

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Rebased, added experimental flag, went through threads and resolved what was needed.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed phpstan issues and rebased (me like cache).

About my comment in the MR:

I am using jsonapi_extras module, which defines the class Drupal\jsonapi_extras\ResourceType\ConfigurableResourceType extending Drupal\jsonapi\ResourceType\ResourceType.

The module also provides a decorator repository
Drupal\jsonapi_extras\ResourceType\ConfigurableResourceTypeRepository which overrides createResourceType().
The implementation calls the parent method and then instantiates a new ConfigurableResourceType based on the returned ResourceType.

My implementation currently looks like this (note the use of $resource_type->isTranslatable()):

<?php
  protected function createResourceType(EntityTypeInterface $entity_type, $bundle) {
    $resource_type = parent::createResourceType($entity_type, $bundle);

    $configurable_resource_type = new ConfigurableResourceType(
      $resource_type->getEntityTypeId(),
      $resource_type->getBundle(),
      $resource_type->getDeserializationTargetClass(),
      $resource_type->isInternal(),
      $resource_type->isLocatable(),
      $resource_type->isMutable(),
      $resource_type->isVersionable(),
      $resource_type->getFields(),
      $resource_type->getTypeName(),
      $resource_type->isTranslatable(),
    );
   ...
?>

While testing this, I occasionally encountered the following error:

Error: Typed property Drupal\jsonapi\ResourceType\ResourceType::$isTranslatable must not be accessed before initialization.

This error seems to happen when all caches are cleared and I perform:
https://{{DOMAIN}}/jsonapi/node/mybundle/{{mybundle_UUID}}
NOTE: No langCode neither Accept-Language have been added to the Request.

ResourceType objects are cached by ResourceTypeRepository, which means they are serialized and later unserialized from the cache backend.

If a typed property is introduced without a default value or the property is not accesible (private visibility), serializing/unserializing objects will not contain that property. When such an object is later unserialized and the property is accessed, PHP throws the “must not be accessed before initialization” error.

To prevent this, I've modified the property declaration in ResourceType to ensure it always has a default value (which may or may not have effect) but most important I've changed the visibility to be protected which seems to solve the issue at serialization time:
protected bool $isTranslatable = FALSE;

This avoids accessing an uninitialized typed property.

However, initializing the property with a default value may also mask the underlying cache compatibility issue: older cached objects that do not contain the property will now behave as if $isTranslatable were FALSE, which may not reflect the actual runtime value. In practice this means the fatal error disappears, but stale cached objects may temporarily behave incorrectly until caches are rebuilt.

I am still investigating the exact conditions that trigger this behaviour and will report further findings once I have a clearer understanding of the root cause.

BTW I might be totally wrong since I've just jump in into this issue and I am getting lost with the code and "double layer" caching at this point.