EntityRepository::getTranslationFromContext() function forcibly adds default entity language to fallback candidates

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

Issue summary could be updated to include the proposed solutions.

But what this function can return if the Entity has no allowed translations to the desired language? Any ideas

This is a question so not the solution.

+
     $entity_type = $this->entityTypeManager->getDefinition($entity_type_id);

1. Seems out of scope

Also as a bug this could use a test case

Thanks.

I've rebased the MR on 10.1.x branch, have updated the summary and extended the unit test to check this case. Could you please review again?

Thanks!

Ran the test locally to make sure they failed and got

Failed asserting that Double\ContentEntityInterface\P4 Object &00000000000000b90000000000000000 (
    'objectProphecyClosure' => Closure Object &00000000000000ad0000000000000000 (
        0 => Closure Object &00000000000000ad0000000000000000
    )
) is null.

which is good.

The changes and comments make sense to me. So lets see what committers say.

Adding issue credits and hiding patches as there's an MR

I think we need to do this without breaking BC, left a comment on the MR

> I think we need to do this without breaking BC, left a comment on the MR

Yeah, it was done by previous requests, and reverted now. Please take a look!

Revert was made. and seems to be the only change since last review.

Strict fallback check must be also extended to entities with only 1 (default) language provided.
Imagine the situation when you have a node in English (default) language, all other languages are configured to fall back into English and you want to prevent French from being fallback to English via $context['strict_fallback'] = TRUE, but you never get the context checked due to count($entity->getTranslationLanguages()) > 1 condition, which runs at very beginning.

I've modified the patch to skip language count condition in case of strict fallback check.
+ interdiff

@Joyakas Thank you, this is a good point, and your fix works well.

And, about the breaking change, I vote to merge this fix as is (without the breaking change).

And add a new separate issue for 11.x to still make that breaking change: allow EntityRepository::getTranslationFromContext() to return NULL, if the translation should not be available.

What do you think about this?

I triaged this RTBC issue → sometime in the last weeks but didn't leave a comment.

There is only one unanswered question here, about creating a followup to make the breaking change, #23. I am tagging this for a release manager review and leaving at RTBC.

I've created a follow up issue to allowing returning NULL from EntityRepositoryInterface::getTranslationFromContext(): ✨ Allow returning NULL from EntityRepositoryInterface::getTranslationFromContext() function Active

@larowlan is correct about the return typehint. We might be able to do something with it along the lines of the interface method signature change → , but that should be scoped to the existing followup in ✨ Allow returning NULL from EntityRepositoryInterface::getTranslationFromContext() function Active if so.

That said, it's unclear whether the patch or the merge request is canonical here. I understand that the original intent was for the patch to be used with composer-patches since it's safer than relying on an MR branch, but it creates a lot of noise for reviewers. Please don't post both patches/interdiffs and merge requests. Please hide the patch files if the merge request is canonical.

Since this issue would involve a change to the fallback behavior, I think it needs a change record, as well as the review of a subsystem maintainer (either @Gábor Hojtsy or one of the Entity API maintainers). Thanks!

Fixing attribution.

I added a change record: https://www.drupal.org/node/3391381 →

That said, it's unclear whether the patch or the merge request is canonical here. I understand that the original intent was for the patch to be used with composer-patches since it's safer than relying on an MR branch, but it creates a lot of noise for reviewers. Please don't post both patches/interdiffs and merge requests. Please hide the patch files if the merge request is canonical.

I hid the patch files.

CR looks good, and usage example is always a plus.

Still needs sign off but to keep the issue from stalling going to remark it.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch 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 on the fresh 11.x.

✨ Allow returning NULL from EntityRepositoryInterface::getTranslationFromContext() function Active was actually just a documentation issue, so we need that D11 followup to add the return type change to the method signature on the interface. Perhaps that's what 📌 language fallback logic to return NULL Active was meant to be? Thanks!

Saving credits.

so we need that D11 followup to add the return type change to the method signature on the interface

But we already have the suitable return type in the D11 interface:

  /**
   * Gets the entity translation to be used in the given context.
   *
   * This will check whether a translation for the desired language is available
   * and if not, it will fall back to the most appropriate translation based on
   * the provided context.
   *
   * @param \Drupal\Core\Entity\EntityInterface $entity
   *   The entity whose translation will be returned.
   * @param string $langcode
   *   (optional) The language of the current context. Defaults to the current
   *   content language.
   * @param array $context
   *   (optional) An associative array of arbitrary data that can be useful to
   *   determine the proper fallback sequence.
   *   - strict_fallback: A boolean key to enable strict fallback mode.
   *     The strict mode disables falling back to default language, if it is not
   *     allowed by fallback candidates.
   *   Other values of this context are passed to the function
   *   LanguageManager::getFallbackCandidates().
   *
   * @return \Drupal\Core\Entity\EntityInterface|null
   *   An entity object for the translated data, or NULL if the requested
   *   translation is missing, forbidden, or unavailable.
   *
   * @see \Drupal\Core\Language\LanguageManagerInterface::getFallbackCandidates()
   */
  public function getTranslationFromContext(EntityInterface $entity, $langcode = NULL, $context = []);

Only phpdoc was against it, and now the phpdoc is fixed too ;)

So we can move forward with the "strict fallback" flag without any breaking changes.

The submaintainer for language I don't believe is active so elevating to framework manager.

Actually, this is a change to the entity system translation behavior, not to language.module. (We have both "language system" and "language.module" in the component field, which is part of the confusion.) As per #26:

(either @Gábor Hojtsy or one of the Entity API maintainers)

Been about 2 weeks, bumping to framework manager.

@Murz The MR seems to be missing the necessary changes in #22 (i.e. correctly handle the case where only one language exists)?

Even with the patch, we will have a problem when utilizing EntityRepository::getCanonicalMultiple(), which will discard the provided $contexts array and use a separate $legacy_context variable when calling getTranslationFromContext(). How can we make strict_fallback apply for this method as well? (This has implications when using e.g. EntityBase::access() which will return true even though the translation does not exist.)

Seems #39 + #40 provided additional feedback, moving to NW for addressing.

After 10.2 drupal upgrade and changes from https://www.drupal.org/project/drupal/issues/3383692 ✨ Allow returning NULL from EntityRepositoryInterface::getTranslationFromContext() function Active patch from #22 can't be applied.
Here is a re-roll for drupal 10.2 version

Fix here looks similar to the latest one from 🐛 Sort out and fix language fallback inconsistencies Needs work , perhaps the two should be merged on the bigger issue?

Finally, I fixed my problem with 🐛 Sort out and fix language fallback inconsistencies Needs work