Follow-up to #2649752: Add categorization to config translation mappers

Problem/Motivation

ConfigEntityMapper provides additional methods on top of ConfigNamesMapper that do not have an interface.

The interface and its methods are needed for use-cases where we need to dynamically enhance mappers (once #2577761: We need a way to dynamically alter the list of config names for config mappers is in), for example #2546212: [PP-1] Entity view/form mode formatter/widget settings have no translation UI.

Proposed resolution

Create interface for ConfigEntityMapper so we can code against contracts.

Remaining tasks

Commit.

User interface changes

None.

API changes

API addition. No breaks for classes extending ConfigEntityMapper (or not doing so). The following interface is added:

interface ConfigEntityMapperInterface {
public function getEntity();
public function setEntity(ConfigEntityInterface $entity);
public function setType($entity_type);
public function getType();
}

Data model changes

None.

Comments

penyaskito created an issue. See original summary.

Daniel_Rempe’s picture

Status: Active » Needs review
FileSize
4.83 KB

Patch proposal

tstoeckler’s picture

Title: Create interfaces for config translation mappers » Create an interface for ConfigEntityMapper
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +sprint, +SprintWeekend2016, +SprintWeekendBerlin

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.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updated issue summary.

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs issue summary update
FileSize
1.41 KB

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.

Status: Needs review » Needs work

The last submitted patch, 5: 2653652-protected.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Gábor Hojtsy’s picture

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

maxocub’s picture

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;
  }
Gábor Hojtsy’s picture

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

tstoeckler’s picture

We need this interface for #2546212: [PP-1] 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.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Kristen Pol’s picture

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.

penyaskito’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update +drupalironcamp2016

#11 updated the issue summary with the purpose of this responding to #5, so removing the Needs summary update tag.

Interface looks good to me, and this is a good improvement not only for unblocking core issues but for some special contrib cases (e.g. Lingotek module).

I'm wondering if we need the same for ConfigFieldMapper, but that could be a different issue. RTBCing this.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 2653652-11-config-entity-mapper-interface.patch, failed testing.

penyaskito’s picture

Status: Needs work » Needs review
FileSize
623 bytes
1.53 KB
4.63 KB

Rerolled.

Changed @return docblock per phpcs complaint. Changed docblock in the setEntity() method in the interface as it changed in the class since last patch.

tstoeckler’s picture

Status: Needs review » Needs work

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.

tstoeckler’s picture

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?

maxocub’s picture

Status: Needs review » Reviewed & tested by the community

+1 for this

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2653652-18.patch, failed testing.

penyaskito’s picture

Status: Needs work » Reviewed & tested by the community

That failure is unrelated, maybe a broken HEAD?

tstoeckler’s picture

Issue tags: +dcmuc16

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 2653652-18.patch, failed testing.

tstoeckler’s picture

Status: Needs work » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.