Create an interface for ConfigEntityMapper

Patch proposal

Thanks a lot, looks great.

ConfigEntityMapper is in fact not used anywhere else in the code, so this is sufficient. ConfigFieldMapper does not provide any methods at the moment, so it does not make sense to add an interface for it.

Updating title and issue summary accordingly.

Updated issue summary.

I'm not convinced this is a good use case for an interface - it's used only by config_translation module internally, and extended by NodeTypeMapper. Just looks like a base class to me.

If we add an interface, then realise we need to add another method to ConfigEntityMapper, then we'd not be able to without adding an additional interface.

On top of that, I didn't look exhaustively, but the only place I can see actually using this class is config_translation_config_translation_info(), which doesn't call any of the methods in the interface.

This suggests to me that the methods should have been protected in the first place, and are only public by accident.

We might decide we can't make them protected during 8.x (or we could decide this class is internal and should never be used directly), but we could add a note that they'll be made protected in 9.x even if we can't do that.

Uploading a patch that just makes them protected to see if that assumption is right. The unit tests will break but if everything else passes we need to rethink this a bit.

@catch: I believe we use the methods in listings to populate mappers. I am not 100% sure how to read the fails if they appear due to the test only or due to the page being tested, but it is clearly not only unit tests.

I think the fail in ConfigTranslationOverviewTest() is due to the fact that getType() is called in ConfigTranslationListController.php (line 65)

  public function listing($mapper_id) {
    $mapper_definition = $this->mapperManager->getDefinition($mapper_id);
    $mapper = $this->mapperManager->createInstance($mapper_id, $mapper_definition);
    if (!$mapper) {
      throw new NotFoundHttpException();
    }
    $entity_type = $mapper->getType();
    // If the mapper, for example the mapper for fields, has a custom list
    // controller defined, use it. Other mappers, for examples the ones for
    // node_type and block, fallback to the generic configuration translation
    // list controller.
    $build = $this->entityManager()
      ->getHandler($entity_type, 'config_translation_list')
      ->setMapperDefinition($mapper_definition)
      ->render();
    $build['#title'] = $mapper->getTypeLabel();
    return $build;
  }

So looks like we either need that or some equivalent to make the logic still work.

We need this interface for #2546212: Entity view/form mode formatter/widget settings have no translation UI, for example, to be able to introspect for which entity a mapper is for. Currently the latest patch does a instanceof ConfigEntityMapper in order to to a ->getEntity(). It would be nice to instead have an interface for that.

Re-uploading #2 but for some minor fixes (see interdiff). To me this is still RTBC but would like some confirmation that this is the way to go.

I reviewed the code and didn't see anything obviously wrong with the syntax or logic though I don't have an opinion on whether or not this is the "right" way to go (adding the interface).

I am wondering the best way to test this via the UI.

Sorry, found one nitpick.

+++ b/core/modules/config_translation/src/ConfigEntityMapperInterface.php
@@ -18,14 +18,15 @@
-   * @param \Drupal\Core\Config\Entity\ConfigEntityInterface $entity
+   * @param \Drupal\Core\Config\Entity\ConfigEntityInterface

This change is actually wrong.

Here we go.

@penyaskito maybe it would be OK for you to RTBC even though you worked on this, because both of our changes are very minimal?

+1 for this

1. +++ b/core/modules/config_translation/src/ConfigEntityMapperInterface.php
   @@ -0,0 +1,61 @@
   +  public function setEntity(ConfigEntityInterface $entity);
   ...
   +  public function setType($entity_type);
   

   What is weird about this object is the entity type and entity can get out-of-sync

2. +++ b/core/modules/config_translation/src/ConfigEntityMapperInterface.php
   @@ -0,0 +1,61 @@
   +   *   The entity type to set.
   

   entity type ID

3. +++ b/core/modules/config_translation/src/ConfigEntityMapperInterface.php
   @@ -0,0 +1,61 @@
   +   * Gets the entity type this mapper is for.
   

   entity type ID

4. +++ b/core/modules/config_translation/src/ConfigEntityMapperInterface.php
   @@ -0,0 +1,61 @@
   +   * @return string
   +   */
   

   Let's make this comply we coding standards and say that it returns the entity type ID.

this looks (almost) done ... but it was abandoned

Created a MR
- From previous commits - but keep the existing comments
- + updates for #25 suggestions

Left some small nitpicks

Can we get a change record written for the new interface?

Updates
- new change record draft https://www.drupal.org/node/3366740
Not sure what details/info should i put there.

- Update the MR
Add typehints.
Use interface for instanceof instead of class.

@smustgrave could you, please, check all these?
thanks

Change record and typehint changes look good to me.

Committed and pushed 49e0f8b6a4 to 11.x. Thanks!

Also published the change record.