ContentEntityInterface::getTranslation() should throw an exception when an invalid language is specified

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.

There is no EntityNG in the core now.

- 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 ?

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

Let's try this

Oops, wrong patch

And now with less debuggity

Green patch, reviews welcome :)

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

+++ 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.

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

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 ?

+++ 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 ? :-)

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}") ?

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.

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 ?

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.

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

And a change record :)

Thanks!

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

And the CR: https://www.drupal.org/node/2578373

Back to RTBC then :-)

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.

Gee, PIFR you're great but also painful

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

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.

Published the change record