Make EntityConverter load the latest translation-affecting revision for translated entities

This is a hard blocker for #2860097: Ensure that content translations can be moderated independently.

#4.1: done: #2938929: Make the entity repository required for EntityConverter.

NW mostly for nits, then this is RTBC!

1. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -36,6 +39,29 @@
   + * make the converter load the latest revision affecting the translation
   

   Supernit: s/the converter/this converter/

2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -36,6 +39,29 @@
   + * it will fall fall back to the latest revision. For instance, if an entity has
   

   s/fall fall/fall/ 😆

3. +++ b/core/tests/Drupal/Tests/Core/ParamConverter/EntityConverterTest.php
   @@ -130,4 +138,18 @@ public function testConvertWithInvalidDynamicEntityType() {
   +   * Tests that skipping the language manager triggers a deprecation error.
   

   Nit: s/skipping/omitting/

4. +++ b/core/tests/Drupal/Tests/Core/ParamConverter/EntityConverterTest.php
   @@ -130,4 +138,18 @@ public function testConvertWithInvalidDynamicEntityType() {
   +    $container = $this->createMock('Symfony\Component\DependencyInjection\ContainerInterface');
   

   Nit: ContainerInterface::class would be slightly better.

5. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   @@ -325,6 +325,7 @@ public static function getSkippedDeprecations() {
   +      'The language manager parameter has been added to EntityConverter since version 8.5.0 and will be made required in version 9.0.0.',
   

   Do we actually need this? Will a core test fail if we don't add this? (We're only supposed to add this for deprecations that would otherwise make core tests fail. And AFAICT we're already updating all parameter converters that subclass EntityConverter here.)

#13 looks good.

+++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
@@ -46,14 +72,31 @@ class EntityConverter implements ParamConverterInterface {
-  public function __construct(EntityManagerInterface $entity_manager) {
+  public function __construct(EntityManagerInterface $entity_manager, LanguageManagerInterface $language_manager = NULL) {
     $this->entityManager = $entity_manager;
+    $this->languageManager = $language_manager ?: \Drupal::languageManager();
+    if (!isset($language_manager)) {
+      @trigger_error('The language manager parameter has been added to EntityConverter since version 8.5.0 and will be made required in version 9.0.0.', E_USER_DEPRECATED);
+    }

Are we sure we want to make the language manager (effectively) required for all entity converters? It's only needed for entities that are translatable, revisionable, and where there's a route flag asking for the latest revision. In which case, why require the parameter for all other cases, like all config entity param converters? What about adding a protected method, languageManager(), and trigger the deprecation warning from there, so only when used?

Thanks. The interdiff looks great. I think that means we can also remove the changes to AdminPathConfigEntityConverter, ViewUIConverter, and maybe others from the patch?

1. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -245,4 +238,22 @@ protected function getEntityTypeFromDefaults($definition, $name, array $defaults
   +      if (!isset($language_manager)) {
   

   Not needed.

2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -245,4 +238,22 @@ protected function getEntityTypeFromDefaults($definition, $name, array $defaults
   +        @trigger_error('The language manager parameter has been added to EntityConverter since version 8.5.0 and will be made required in version 9.0.0.', E_USER_DEPRECATED);
   

   Perhaps change the message to "The language manager parameter has been added to EntityConverter since version 8.5.0 and will be made required in version 9.0.0 when requesting the latest translation-affected revision of an entity."?

1. +++ b/core/core.services.yml
   @@ -976,7 +976,7 @@ services:
   -    arguments: ['@entity.manager', '@config.factory', '@router.admin_context']
   +    arguments: ['@entity.manager', '@config.factory', '@router.admin_context', '@language_manager']
   

   This change is unnecessary.

2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -132,4 +238,20 @@ protected function getEntityTypeFromDefaults($definition, $name, array $defaults
   +   * @return \Drupal\Core\Language\LanguageManagerInterface|null
   

   When can this return NULL?

3. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -132,4 +238,20 @@ protected function getEntityTypeFromDefaults($definition, $name, array $defaults
   +  protected function languageManager() {
   

   Let's mark this @internal.

4. +++ b/core/modules/content_moderation/src/ParamConverter/EntityRevisionConverter.php
   @@ -30,7 +30,7 @@ class EntityRevisionConverter extends EntityConverter {
   -    parent::__construct($entity_manager);
   +    parent::__construct($entity_manager, \Drupal::languageManager());
   

   AFAICT thanks to the changes proposed by @effulgentsia and implemented now, this change is also no longer necessary.

   (It will be necessary in #2860097: Ensure that content translations can be moderated independently though AFAICT.)

#25 interdiff is trivial, so back to RTBC.

Adding reviewer credit for @Wim Leers, and also for @timmillwood and @hchonov for their work on #2860097: Ensure that content translations can be moderated independently from which this was split.

Pushed to 8.6.x and cherry picked to 8.5.x.

+++ b/core/modules/content_translation/src/Routing/ContentTranslationRouteSubscriber.php
@@ -70,6 +70,7 @@ protected function alterRoutes(RouteCollection $collection) {
+              'load_latest_revision' => TRUE,
+              'load_latest_revision' => TRUE,
+              'load_latest_revision' => TRUE,
+              'load_latest_revision' => TRUE,

Per #2860097-111: Ensure that content translations can be moderated independently, this might be a controversial change. I'll await comments from @hchonov and @plach, but I'm considering reverting just these lines if there isn't agreement about them yet.

@plach proposed in #2860097-115: Ensure that content translations can be moderated independently:

Given that loading the latest (translation-affecting) revision is needed only when Content Moderation is enabled, at least in core, we could alter CT's route definitions and add the load_latest_revision flag in the CM's code.

and I've responded to this in #2860097-121: Ensure that content translations can be moderated independently:

If this is the only way, then I am not going to argue. There is only drawback with this - this makes it impossible for other modules making use of forward revisions to work together with content moderation, but I guess this will be not the only point? If so, then we are fine to go, but probably should somehow make it clear that content moderation could not be used together with other modules/approaches relying on forward revisions.

So I think that those changes to the content translation routes should be reverted.

I have one remark and one question about the patch that got in:

1. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -68,14 +104,12 @@ public function convert($value, $definition, $name, array $defaults) {
   +      $entity = $this->getLatestTranslationAffectedRevision($entity, $langcode);
   
   @@ -87,6 +121,78 @@ public function convert($value, $definition, $name, array $defaults) {
   +      $revision_id = $storage->getLatestTranslationAffectedRevisionId($entity->id(), $langcode);
   

   As we are executing a query to retrieve the revision ID, then we don't necessary have to load the default entity revision. The code could be easy converted to skip loading the default revision if it isn't needed.

2. +++ b/core/lib/Drupal/Core/ParamConverter/EntityConverter.php
   @@ -132,4 +238,22 @@ protected function getEntityTypeFromDefaults($definition, $name, array $defaults
   +  protected function languageManager() {
   +    if (!isset($this->languageManager)) {
   +      $this->languageManager = \Drupal::languageManager();
   

   Why do we provide BC here instead of enforcing the language parameter to be included through DI? I've found the following in https://www.drupal.org/core/d8-bc-policy:

   The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release. These are objects where developers are not expected to call the constructor directly in the first place. Constructors for value objects where a developer will be calling the constructor directly are excluded from this statement.

   If I am not mistaken, then we don't have to provide BC when altering the constructor.

#34.2: the BC policy is the theory, in practice we also look at "likely real-world disruption". In this case, with half a dozen subclasses in core alone, it is very likely that we'd disrupt contrib/custom modules. In this case, and many cases before it, we've therefore opted to make new constructor arguments optional, and then automatically load them.

@plach, if it happens that the current default revision and the forward revisions are different, which is pretty likely to be the case when using forward revisions, then it will always happen that we load two entity objects. Optimizations for loading only one entity object by ID and by revisions ID if it is the same revision both point to are done in #2620980: Add static and persistent caching to ContentEntityStorageBase::loadRevision() and could be further optmized. The patch there will load only one entity from the persistent cache if a revision is first loaded by ::loadRevision('revision_ID_of_the_current_default_revision') and then the entity is loaded through ::load('entity_id').

So I think that those changes to the content translation routes should be reverted.

I posted a patch to revert them in #2939247: Revert Content Translation routes to act on default rather than latest revisions.