Node revisions don't display translated

I reviewed the code in question to see what getTranslationFromContext() does. Code looks solid and is doing the right thing. Tests look sufficient.

1. I enabled content translation & language.
2. Then added Spanish language.
3. Then I marked that Page is a translatable content type.
4. Then I added an English page with a Spanish translation.
5. Then I added some English and Spanish revisions.

Without the patch, the behaviour is identical as described in #0. With the patch, the issue is fixed.

IMHO we should fix this in \Drupal\Core\ParamConverter\EntityRevisionParamConverter as well, given that this will be used hopefully in the future for nodes as well.

re#9: Is this what you mean?

+++ b/core/lib/Drupal/Core/ParamConverter/EntityRevisionParamConverter.php
@@ -52,7 +52,7 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager) {
-    return $entity_storage->loadRevision($value);
+    return $entity_storage->getTranslationFromContext($value);

Just that doesn't work, we need to load the revision still ...

@dawehner
Like this?

I'm not sure what happens if:

- you have a non-default revision
- you load it
- you change something
- you save it

Do we save the non-changed translated values from the revision we loaded, or the default one?

Bumping to crtical, because if it's the default revision that's a data integrity issue, we can downgrade if it's not.

Here's a test for the scenario described in #16.

Fixed the unit test fail from #13 by properly injecting the entity repository service and also moved the test method to an existing test class (\Drupal\node\Tests\NodeTranslationUITest).

Since the test-only patch #17 passed, this is not a critical issue anymore.

Forgot a tag :/

+++ b/core/lib/Drupal/Core/ParamConverter/EntityRevisionParamConverter.php
@@ -32,13 +33,23 @@ class EntityRevisionParamConverter implements ParamConverterInterface {
+   *
+   * @var \Drupal\Core\Entity\EntityRepositoryInterface
+   */
+  protected $entityRepository;
+
+  /**
    * Creates a new EntityRevisionParamConverter instance.
    *
    * @param \Drupal\Core\Entity\EntityTypeManagerInterface $entity_type_manager
    *   The entity type manager.
+   * @param \Drupal\Core\Entity\EntityRepositoryInterface $entity_repository
+   *   The entity repository.
    */
-  public function __construct(EntityTypeManagerInterface $entity_type_manager) {
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, EntityRepositoryInterface $entity_repository) {
     $this->entityTypeManager = $entity_type_manager;
+    $this->entityRepository = $entity_repository;
   }
 
   /**
@@ -46,8 +57,8 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager) {

@@ -46,8 +57,8 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager) {
    */
   public function convert($value, $definition, $name, array $defaults) {
     list (, $entity_type_id) = explode(':', $definition['type'], 2);
-    $entity_storage = $this->entityTypeManager->getStorage($entity_type_id);
-    return $entity_storage->loadRevision($value);
+    $entity = $this->entityTypeManager->getStorage($entity_type_id)->loadRevision($value);
+    return $this->entityRepository->getTranslationFromContext($entity);
   }
 
...
diff --git a/core/modules/node/src/Controller/NodeController.php b/core/modules/node/src/Controller/NodeController.php

It seems to be that we don't really have test coverage for this code change. The unit tests seems to not really cut it. Could we expand the test in EntityRevisionTranslationTest to include something for that as well?

@dawehner - Doesn't the test in NodeTranslationUITest cover this?

@timmillwood, nope, that only covers the changes in \Drupal\node\Controller\NodeController::revisionShow().

Here's a test for the changes in EntityRevisionParamConverter. The test-only patch is also the interdiff.

Thank you @amateescu!

Apparently, entity URLs don't go well with simpletest when running in a subdirectory, let's just use the URL directly like we do everywhere else.

+1 for this job. I could have seen that. Ideally you set the langcode in the URL object and just pass that to drupalGet.

+++ b/core/modules/system/src/Tests/Entity/EntityRevisionsTest.php
@@ -144,13 +144,13 @@ public function testEntityRevisionParamConverter() {
 
     // Check that the entity revision is upcasted in the correct language.
-    $revision_url = $forward_revision->toUrl('revision')->toString();
+    $revision_url = 'entity_test_mulrev/' . $entity->id() . '/revision/' . $forward_revision->getRevisionId() . '/view';
 
     $this->drupalGet($revision_url);

you just can't use toString(), you have to use getInternalPath(), then it should work fine. Or you can just pass the Url object to drupalGet(), that should actually work as well (in simpletest, not sure about phpunit ,didn't check what kind of test this is)

Sounds like needs work

I tried to do that, but I think the test setup is wrong, as it doesn't yet have URL prefixing configured.

+++ b/core/modules/system/src/Tests/Entity/EntityRevisionsTest.php
@@ -144,13 +145,11 @@ public function testEntityRevisionParamConverter() {
+    $this->drupalGet($forward_revision->getTranslation('de')->toUrl('revision'));

I tried that as well but, unfortunately, it doesn't generate a proper URL for the entity revision in a specific language.

I don't know if that's a bug or just the test setup, so I still think the patch in #26 is good to go.

Yeah me too, but we could file a follow up for that.

Yes, toUrl() doesn't add that option, you would have to to something like setOption('language', ..) yourself on the url object.

We discussed that before. I think it would be nice if it worked like that.

Yes, toUrl() doesn't add that option, you would have to to something like setOption('language', ..) yourself on the url object.

I did explictly that in #31 but it still didn't worked.

Moving this to 8.1.x since it's a straight bugfix.

Just uploading the correct patch to be the last patch on the issue.

Regarding #37, it's a straight bugfix, but it does involve a constructor change on a paramconverter to add an additional service. In our BC policy we say:

Paramconverters, access checkers, event subscribers and similar services which are never expected to be used directly either as services or value objects, are not considered part of the API. You should not extend from these classes and provide your own implementation instead.

So we are okay doing this in 8.1.x for that not-an-API-change. I wondered though about whether we need to force a cache rebuild.

The version change will be enough to cause a container rebuild so no cache rebuild is necessary.

Committed and pushed 9b07bc544c49296bd4fb76b08d676665d9e2d0c1 to 8.2.x and 97d2695 to 8.1.x. Thanks!