Content entities are not upcast in the page language, inconsistent with config entities

This is part of a larger problem on how we now want to deal with entity translations, see the comments #125 and #145 in #2019055: Switch from field-level language fallback to entity-level language fallback, among others. We need a meta issue to discuss and implement the whole thing.

We can use this or create another issue, and the meta will need to be added to #2095603: [meta] Complete Entity Field API :)

If this is an API change significant enough, we may need to elevate this to Critical and beta blocker? Since nobody was working on this for 5 months, I'm removing from the sprint board as a start at least.

I am not sure this will be an API change itself, but it might imply a change in how code is written. I am sorry I was not able to work on this, but I really feel I should be the one at least starting the work. I will try to get back here as soon as we are done with the entity storage stuff.

We should really try to get this in before beta.

That went in.

There's a method for this that we can use.

We already do this for config entities by default. It would be useful to match behaviour with content entities then. Updated issue summary and title.

Kept the interface check to avoid that NULL is passed into that method.

Wow, did expect more fails, although I have no clue what the delete thing there exactly means ;)

Yeah, possibly checking for EntityInterface or just != NULL might make more sense, getTranslationFromContext() does have the same interface check internally, checking for this interface would avoid the additional step into that function, though.

This looks promising, thanks!

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -39,7 +40,11 @@ public function __construct(EntityManagerInterface $entity_manager) {
+        $entity = $this->entityManager->getTranslationFromContext($entity);

I am wondering whether it would make sense to pass an entity_upcast context operation, to allow modules to perform a more targeted altering.

The delete issue is caused by the following lines. Now, the entity is loaded in the same language that it's removing, so the save will fail.

=== core/modules/content_translation/src/Form/ContentTranslationDeleteForm.php
   public function submitForm(array &$form, array &$form_state) {
     // Remove the translated values.
     $this->entity->removeTranslation($this->language->id);
     $this->entity->save();

I don't know the best solution here, but I tried falling back to the default language in removeTranslation; let's see if the tests are happy.

There is still a fail in EntityTranslationFormTest.php, which I couldn't figure out. It's showing "Access Denied" when visiting [langcode]/node/[entity_id]/edit in the test.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -773,6 +773,9 @@ public function removeTranslation($langcode) {
+        $this->activeLangcode = LanguageInterface::LANGCODE_DEFAULT;

We should not be changing the active language of a translation object, instead we should just instantiate the original object in the CT remove translation controller.

(tangential : the internal language handling in ContentEntityBase could be vastly simplified - #2142885: Simplify ContentEntityBase internal field storage by removing special treatment for LANGCODE_DEFAULT)

update.

Thanks, this looks almost good to me!

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -39,7 +40,11 @@ public function __construct(EntityManagerInterface $entity_manager) {
+      if ($entity instanceof EntityInterface) {

This should be TranslatableInterface.

@Berdir:

What do you think about #17?

correcting based on #23.

RTBC here. Sasha, Daniel?

We went back and forth on that instanceof earlier already. I don't really care, but the thing is that TranslatableInterface does not extend EntityInterface and that method is type hint on EntityInterface (and then internally checks for TranslatableInterface).

The result is that this PHPStorm highlights this as incorrect. That said, I don't care which one it is. If you're OK with this then I am. A bit surprised that there wasn't more that we had to do.

Or that. The main reason that I added the interface check at all is to avoid passing NULL (for non-existing entities) to that method. So just "if ($entity) {" would work too. Either way, a comment would probably make sense, something like "Ensure that translatable entities are set to the current language", or something like that. Suggestions on the wording and exact approach, @plach?

What about this?

If the entity type is translatable, ensure we return the proper translation object for the current context.

Looks good, thanks

Let's get some test coverage of this.

I'm working in the test coverage.
The patch#30 needs reroll

diff --cc core/modules/system/src/Tests/Entity/EntityTranslationFormTest.php
index 246b053,e7d5334..0000000
--- a/core/modules/system/src/Tests/Entity/EntityTranslationFormTest.php
+++ b/core/modules/system/src/Tests/Entity/EntityTranslationFormTest.php
@@@ -106,7 -106,8 +106,12 @@@ class EntityTranslationFormTest extend
  
      // Create a body translation and check the form language.
      $langcode2 = $this->langcodes[1];
++<<<<<<< HEAD
 +    <strong>$node->getTranslation($langcode2)->body->value = $this->randomMachineName(16);</strong>
++=======
+     <strong>$node->getTranslation($langcode2)->body->value = $this->randomName(16);</strong>
+     $node->getTranslation($langcode2)->setOwnerId($web_user->id());
++>>>>>>> applying reroll 30
      $node->save();
      $this->drupalGet($langcode2 . '/node/' . $node->id() . '/edit');
      $form_langcode = \Drupal::state()->get('entity_test.form_langcode') ?: FALSE;

I have resolved the conflict changing using randomMachineName(16) instead randomName(16).
The reroll is attached.

Tagging novice for reroll and tests.

A test should check that a route that receives an entity as a route parameter should get it in the right language. So enable a module with entities, add a test route in a test module that accepts {node} or similar, then output the value of $entity->language()->getId() in the controller and check that is correct when access in diferent languages.

Rerolled patch. Will work on tests next.

I enable Language, Content translation module.
Add a language
Add a test text field to account
Edit Admin profile and set the test text to English
Try to translate Account, and set test text to Hungarian
Test text is always Hungarian. I try user/1 and hu/user/1 path.

How do I test/review this issue?

I wrote a simple demo module for that, reporting the entity language of nodes when calling /demo/entup/{nid}.

It returns Node language ID: en for all three languages (en,de,hu) in my test system when calling one of

/demo/entup/1
/de/demo/entup/1
/hu/demo/entup/1

Applying the patch via patch -p1 < entity_upcasting-216095-38.patch runs through without issues and applies correctly.
The output of the module now reflects the correct language

/demo/entup/1 => Node language ID: en
/de/demo/entup/1 => Node language ID: de
/hu/demo/entup/1 => Node language ID: hu

See attached archive for using the demo module to generate the output mentioned above.

It would be great to have an automated test for this.

Created a test that adds a language (German in this case) and assigns a prefix (de), and then it creates a node in English and its German translation and then verifies that the right translation is loaded by the router. The tests-only patch should fail, as the patch is not applied there; and the patch with tests should succeed. Please review.

@robertdbailey proved to me on his machine that the tests pass/fail as expected. Awesome!
I think we can knock this one out.

reattempting patch test. Previous failure was:

FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh.

Setting back to RTBC per review by @tstoeckler.

Awesome work, RTBC +1.

Committed b35d190 and pushed to 8.0.x. Thanks!