Problem/Motivation

ContentEntityInterface::getTranslation() returns a new translation if the specified language is valid but there's no corresponding translation for it yet. This is not ideal, we should be creating new translations explicitly. Moreover this has also performance implications, since ContentEntityInterface::addTranslation() is slow (see also #2382675: hook_entity_create() affects the data of new translations of existing entities in unexpected and undocumented ways).

Proposed resolution

Remove the ContentEntityInterface::addTranslation() invocation from ContentEntityInterface::getTranslation().

Remaining tasks

  • Figure out what's the desired behavior when LanguageInterface::LANGUAGE_NOT_SPECIFIED is passed.
  • Reviews

User interface changes

None

API changes

Yes

Data model changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because this can trigger unexpected behaviors.
Issue priority Major because this has several implications: it makes the Entity Translation API more fragile, it may cause perfomance issues when listing multiple entities in multilingual environments and is also preventing us from finding a sane solution for #2072945: Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends.
Prioritized changes The main goals of this issue are reducing API fragility and improving performance and security.
Disruption Disruptive for contributed and custom modules leveraging the Entity Translation API in an improper way. Core fixes are mainly in tests.

Comments

yched’s picture

Yes, we need to refactor how widgets and formatters are invoked on the fields of an entity (field_attach_*() / field_invoke_method*())
But for that multilingual logic and language fallback needs to be sorted out.

jibran’s picture

Issue summary:View changes

There is no EntityNG in the core now.

Berdir’s picture

Status:Active» Needs review
FileSize
874 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,811 pass(es), 36 fail(s), and 32 exception(s). View

Yeah, let's see if this still happens.

Status:Needs review» Needs work

The last submitted patch, 3: get-language-new-2090983-3.patch, failed testing.

yched’s picture

- addTranslation() is unnecessarly called :
field_invoke_method_multiple() is long gone now, not sure whether that's still the case ?

- addTranslation() is slow :
Yeah, posted a patch in #2382675: hook_entity_create() affects the data of new translations of existing entities in unexpected and undocumented ways, that removes the entity_create() - let's discuss there ?

plach’s picture

Title:EntityNG::addTranslation() is slow and unnecessarly called (?)» ContentEntityInterface::getTranslation() should throw an exception when an invalid language is specified
Issue tags:+rc deadline

And with invalid I mean both a language not installed on the site and a language for which a translation does not exist.

plach’s picture

Status:Needs work» Needs review
FileSize
22.59 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,158 pass(es), 2 fail(s), and 1 exception(s). View

Let's try this

Status:Needs review» Needs work

The last submitted patch, 7: et-invalid_translation_exception-2090983-7.patch, failed testing.

plach’s picture

Status:Needs work» Needs review
FileSize
2.83 KB
28.5 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch entity-revisionable_fields-2497737-9_0.patch. Unable to apply patch. See the log in the details link for more information. View
plach’s picture

FileSize
2.21 KB
24.48 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,259 pass(es). View

Oops, wrong patch

plach’s picture

FileSize
530 bytes
24.28 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,255 pass(es). View

And now with less debuggity

The last submitted patch, 9: entity-revisionable_fields-2497737-9.patch, failed testing.

plach’s picture

Issue tags:+sprint

Green patch, reviews welcome :)

yched’s picture

@plach : the IS is from 2 years ago :-) Can you provide a small description of what the patch actually does, to help reviewing ?

plach’s picture

Issue summary:View changes
Issue tags:+API change
+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -737,21 +737,18 @@ public function getTranslation($langcode) {
+    // If the entity or the requested language is not a configured language, we
+    // fall back to the original entity itself, since in this case we are not
+    // dealing with a translation.

I'm not sure this is correct/desirable.

@yched:

Updated the IS. I'm working the API sanity aspects here and punting the rest to #2382675: hook_entity_create() affects the data of new translations of existing entities in unexpected and undocumented ways.

andypost’s picture

So no exception thrown... when requested unknown language, maybe assert() this?

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -737,21 +737,18 @@ public function getTranslation($langcode) {
    +    // If the entity or the requested language is not a configured language, we
    

    "a configured language" confusing, maybe "configurable?

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityForm.php
    @@ -173,7 +173,7 @@ protected function init(FormStateInterface $form_state) {
    -    $this->entity = $this->entity->getTranslation($langcode);
    +    $this->entity = $this->entity->hasTranslation($langcode) ? $this->entity->getTranslation($langcode) : $this->entity->addTranslation($langcode);
    

    Makes sense! but docs for \Drupal\Core\TypedData\TranslatableInterface::getTranslation() should be updated

yched’s picture

Still wrapping my mind around the behavior change, no real opinion atm :-)

Just, side notes (not introduced by the patch, but might be worth clarifying since dealing with that very piece of logic here) :
- ContentEntityBase::getLanguages() says "{@inheritdoc}" but that seems to be a lie, according to my PHPstorm there's nothing to inherit from :-)
- The ->isLocked() checks in getTranslation() are not really clear, could use a comment ?

yched’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -737,21 +737,18 @@ public function getTranslation($langcode) {
+    // Otherwise if an existing language was specified instantiate it.
+    elseif (isset($this->translations[$langcode])) {
+      $translation = $this->initializeTranslation($langcode);
+      $this->translations[$langcode]['entity'] = $translation;
+    }

Also, that comment looks fuzzzy, the whole crux of the issue is that this is about "existing translation for the entity" rather than "existing language", right ? :-)

yched’s picture

Patch looks consistent, but I'm not sure how we can justify that the API break is worth it ?
That would be something like "you now cannot write code that inadvertently calls $entity->getTranslation($arbitrary_langcode) and triggers a costly translation creation" ?

Looks like this would be a perf issue moslty in cases where this is done in a very naive loop (like "foreach (enabled languages) {get the translation}") ?

plach’s picture

My main reason for doing this is helping with #2072945: Remove the $langcode parameter in EntityAccessControllerInterface::access() and friends: the moment you cannot request a non-existing translation also access checks become more explicit. Combine API sanity, Performance improvement and Security hardening and I think we have a good reason to do this.

yched’s picture

OK, makes sense.

Then wondering whether returning NULL ("there is no translation") would be better than an exception ? I guess the exception makes the API change more "in you face", but then again doing anything with an $entity that is NULL will also break quite fast ?

plach’s picture

FileSize
16.94 KB
38.02 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch et-invalid_translation_exception-2090983-22.patch. Unable to apply patch. See the log in the details link for more information. View

This addresses #15, I just discovered quite a few bugs in our tests by just removing that magic. It should address also #16 and #18, thanks.

@#17:

1: Yep, looks like an unrelated doc bug
2: Locked languages are built-in system languages that do not map to any actual language, like und. Added a comment to clarify what's happening in ::addTranslation().

@#21:

Yep, I think with an exception it is way easier to figure out what's going wrong.

plach’s picture

Issue summary:View changes
yched’s picture

Status:Needs review» Reviewed & tested by the community

Looks good to me. Temptative RTBC, but committers will probably want a beta evaluation

plach’s picture

Status:Reviewed & tested by the community» Needs work

And a change record :)

Thanks!

plach’s picture

Priority:Normal» Major
Issue summary:View changes

Added beta evaluation and promoted to major since this is soft-blocking a major.

plach’s picture

Status:Needs work» Needs review
yched’s picture

Back to RTBC then :-)

yched’s picture

Status:Needs review» Reviewed & tested by the community

The last submitted patch, 9: entity-revisionable_fields-2497737-9.patch, failed testing.

The last submitted patch, 7: et-invalid_translation_exception-2090983-7.patch, failed testing.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Disambiguating getting and creating translations is a really good idea. Committed 5c0eb9b and pushed to 8.0.x. Thanks!

diff --git a/core/modules/quickedit/src/Tests/EditorSelectionTest.php b/core/modules/quickedit/src/Tests/EditorSelectionTest.php
index c50c25d..25e0095 100644
--- a/core/modules/quickedit/src/Tests/EditorSelectionTest.php
+++ b/core/modules/quickedit/src/Tests/EditorSelectionTest.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\quickedit\Tests;
 
-use Drupal\Core\Language\LanguageInterface;
 use Drupal\quickedit\EditorSelector;
 
 /**
diff --git a/core/modules/quickedit/src/Tests/MetadataGeneratorTest.php b/core/modules/quickedit/src/Tests/MetadataGeneratorTest.php
index d92fc7a..0cf21e0 100644
--- a/core/modules/quickedit/src/Tests/MetadataGeneratorTest.php
+++ b/core/modules/quickedit/src/Tests/MetadataGeneratorTest.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\quickedit\Tests;
 
-use Drupal\Core\Language\LanguageInterface;
 use Drupal\quickedit\EditorSelector;
 use Drupal\quickedit\MetadataGenerator;
 use Drupal\quickedit\Plugin\InPlaceEditorManager;

Fixed on commit.

  • alexpott committed 5c0eb9b on 8.0.x
    Issue #2090983 by plach, Berdir, yched: ContentEntityInterface::...

Status:Fixed» Needs review

Status:Needs review» Needs work

The last submitted patch, 22: et-invalid_translation_exception-2090983-22.patch, failed testing.

yched’s picture

Status:Needs work» Fixed

Gee, PIFR you're great but also painful

mkalkbrenner’s picture

Sorry for hitting the retest link!

But I think there's something wrong with this patch. I opened a follow-up:
#2579187: Revert to an older entity revision with less translations leads to fatal error caused by EntityStorageException

yched’s picture

Oops, sorry @mkalkbrenner, I didn't intend to be rude, I didn't see your retest :-)
I thought that was the bot, coz it keeps doing these kind of things currently.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.