Provide Config translation UI

just the current state. :)

admin/config

admin/config/regional/config-translation

admin/config/regional/config-translation/entity-type/block

admin/structure/block/manage/bartik.breadcrumbs/translate

I'm reviewing this.

#11: config-translation-ui-2095145-7.patch queued for re-testing.

Did this review side by side with @webflo. Most of this was fixed *while* writing up the review. Posting the notes here for ... information.
Also, it was rerolled during the review. So some stuff is a bit different.

Patch coming, later.

1. +++ b/config_translation.module
   @@ -26,6 +26,15 @@ function config_translation_help($path) {
   +    'weight' => -5,
   

   related to #2003812: Reorder element under configuration => Regional and language we might want this lower? Let's make it last and set it to 30.

2. +++ b/config_translation.routing.yml
   @@ -0,0 +1,18 @@
   +config_translation.mapper_index:
   +  path: '/admin/config/regional/config-translation'
   +  defaults:
   +    _title: 'Index'
   

   I looked for examples in core, the content types page is not called list, it is just called Content types.

   This also helps the breadcrumb make sense..

   So I think our title should be:
   Configuration Translation

   Also, looked for examples of what kind of path to use. and if it was ok to use config-translation (with the short config vs configuration).

   In the UI we are careful not to use appreviations, but the url is OK I think.

   For example, I looked at:
   admin/config/development/sync (which has title Synchronize configuration)

3. +++ b/config_translation.routing.yml
   @@ -0,0 +1,18 @@
   +config_translation.entity_index:
   

   we checked views_ui and content_type and they used in the route id, for the unique part, list or overview.

   Let's use list. (instead of entity_index)

   admin/config/regional/config-translation (mapper_index)

   is a list/overview of
   a) config entity types (except field instances)
   b) field instances by entity type [because otherwise all fields were listed on one page, not grouped by entity type/bundle and it was confusing to see for example, body and also body and not know which entity type the body was for which one.]
   c) plain configuration cmi pages

1. +++ b/config_translation.routing.yml
   @@ -0,0 +1,18 @@
   +config_translation.entity_index:
   +  path: '/admin/config/regional/config-translation/{config_translation_mapper}'
   +  defaults:
   +    _content: '\Drupal\config_translation\Controller\ConfigTranslationListController::listing'
   

   this makes the route for the config entities and for the field instances.

   let's call the route id, entity_list (to match the list name we are using on the other, instead of entity_index.

   ----
   title seems to be inherited from the parent.

   let's dynamically set it to be the config mapper label.

2. +++ b/lib/Drupal/config_translation/ConfigEntityMapper.php
   @@ -16,6 +16,11 @@ use Symfony\Component\HttpFoundation\Request;
      /**
   +   * Configuration mapper type.
   +   */
   +  protected $mapperType = 'entity';
   

   needs a type.
   @var .....

3. +++ b/lib/Drupal/config_translation/ConfigEntityMapper.php
   @@ -49,9 +54,14 @@ class ConfigEntityMapper extends ConfigNamesMapper {
       * @param string $entity_type
       *   (optional) Type of the entity.
       */
   -  function __construct($base_path_pattern, $title, $id, $names = array(), $menu_item_type = MENU_LOCAL_TASK, $add_edit_tab = FALSE, $entity_type = NULL) {
   +  function __construct($base_path_pattern, $title, $id, $names = array(), $menu_item_type = MENU_LOCAL_TASK, $add_edit_tab = FALSE, $entity_type = NULL, $definition = array()) {
   

   adding definitions argument.

   add an @param for it.

4. +++ b/lib/Drupal/config_translation/ConfigEntityMapper.php
   @@ -49,9 +54,14 @@ class ConfigEntityMapper extends ConfigNamesMapper {
   +
   +    if ($definition['entity_type'] == 'field_instance') {
   +      $base_entity_type = \Drupal::entityManager()->getDefinition($definition['base_entity_type']);
   +      $this->typeLabel = t('@label fields', array('@label' => $base_entity_type['label']));
   +    }
   

   needs a comment

   something like:

   get the config entity type label for the field instances to use in the listing because that what they are grouped by and is a good label for the group.

   this is a suckky suggestion. reword.

   ----
   also, should entityManager be injected?

   in upcoming patch it will be , it will use the entityManager property that was added by #2085925: Autogenerate config entity translation mapping as much as is sane

5. +++ b/lib/Drupal/config_translation/ConfigEntityMapper.php
   @@ -65,7 +75,8 @@ class ConfigEntityMapper extends ConfigNamesMapper {
          $definition['add_edit_tab'],
   -      $definition['entity_type']
   +      $definition['entity_type'],
   +      $definition
   

   this looks weird.
   we have to get the parts because we are already getting the parts. but why get the whole thing then instead of just the extra parts you want?

   maybe this was already discussed with Gabor

6. +++ b/lib/Drupal/config_translation/ConfigEntityMapper.php
   @@ -142,4 +153,29 @@ class ConfigEntityMapper extends ConfigNamesMapper {
   +  public function getTypeLabel() {
   

   needs doc block.

7. +++ b/lib/Drupal/config_translation/ConfigEntityMapper.php
   @@ -142,4 +153,29 @@ class ConfigEntityMapper extends ConfigNamesMapper {
   +    if (isset($this->typeLabel)) {
   +      return $this->typeLabel;
   +    }
   

   add comment to explain why field instances get special treatment. but they may not always be field instances. others could extend this, set their own typeLabel and this will work to use whatever override they wanted to.

8. +++ b/lib/Drupal/config_translation/ConfigEntityMapper.php
   @@ -142,4 +153,29 @@ class ConfigEntityMapper extends ConfigNamesMapper {
   +    return $this->title;
   

   what is an example of something that does not have a label that we have to fall back on title for?

9. +++ b/lib/Drupal/config_translation/ConfigEntityMapper.php
   @@ -142,4 +153,29 @@ class ConfigEntityMapper extends ConfigNamesMapper {
   +  public function getOperations() {
   +    return array(
   

   most of the time, I think I see a call to parent first, then add stuff.

   @webflo says though, calling parent here makes no sense. we will not have any other operations.

   if parent was called. then it would get a translate operation and we would have to then remove that.

   ok. we just want list here.

10. +++ b/lib/Drupal/config_translation/ConfigMapperManager.php
    @@ -88,4 +88,16 @@ class ConfigMapperManager extends DefaultPluginManager {
    +   * Returns the mapper for the given id.
    +   */
    +  public function getMapperById($mapper_id) {
    

    where needed?

11. +++ b/lib/Drupal/config_translation/ConfigNamesMapper.php
    @@ -172,6 +177,13 @@ class ConfigNamesMapper implements ConfigMapperInterface {
    +  public function getTypeLabel() {
    +    return $this->title;
    +  }
    

    huh? there was another getTypeLabel that defaulted to title.

    we extend this class and override it there.

12. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationEntityListController.php
    @@ -0,0 +1,47 @@
    +    $operations = parent::buildOperations($entity);
    +    foreach (array_keys($operations['#links']) as $operation) {
    +      if (!($operation == 'translate')) {
    +        unset($operations['#links'][$operation]);
    +      }
    

    a comment here explaining why we want to remove operations we got from the parent would be good.

    wait. why not just not call the parent and just add operation? Oh because we are not adding translate operation. they are already there and we would not know exactly how to set it anyway for all the different things.

    ok.

13. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
    @@ -0,0 +1,81 @@
    +use Drupal\Core\Entity\EntityStorageControllerInterface;
    +use Drupal\Core\Extension\ModuleHandlerInterface;
    +use Drupal\field\Field;
    

    since we are adding all these new anyway... alpha order is nice.

14. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
    @@ -0,0 +1,81 @@
    +  protected $baseEntityType = '';
    

    these all need doc blocks, and types (@var)

15. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
    @@ -0,0 +1,81 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function __construct($entity_type, array $entity_info, EntityStorageControllerInterface $storage, ModuleHandlerInterface $module_handler, $definition) {
    

    adding a new param, instead of inheritdoc, copy from the parent and add your new @param.

16. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
    @@ -0,0 +1,81 @@
    +  public function displayBundle() {
    +    return isset($this->baseEntityInfo['bundle_keys']);
    +  }
    

    add comment explaining that that page, user for example, would have the title user, all rows would always list the user bundle in the bundle column so this tried to take out that column.

17. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationListController.php
    @@ -0,0 +1,61 @@
    +    $class = isset($this->mapperDefinition['list_controller']) ? $this->mapperDefinition['list_controller'] : '\Drupal\config_translation\Controller\ConfigTranslationEntityListController';
    +    $controller = new $class($entity_type, $this->entityManager()->getDefinition($entity_type), $this->entityManager()->getStorageController($entity_type), $this->moduleHandler(), $this->mapperDefinition);
    

    this looks complicated.
    comment please about why we do this.

18. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationMapperIndex.php
    @@ -0,0 +1,86 @@
    +    $this->mappers = $this->mapperManager->getMappers();
    ...
    +    foreach ($this->getMappers() as $mapper) {
    +      if ($row = $this->buildRow($mapper)) {
    +        $mappers[$mapper->getMapperType()][] = $row;
    +      }
    +    }
    

    the mapper list/list/overview, the items are grouped, and in the groups are ordered by whatever getMappers returns.

    the mappers get returned all together is some order. the mappers define a weight for themselves (MapperType, in new patch will be a weight).

    items in the overview are sorted by weight. then no sort, just whavever order.

    I think they should be in alphabetical order by their config entity type label.

    I think this could use a comment explaining why it is going in a two dimensional array, grouped by type/weight.

19. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationMapperIndex.php
    @@ -0,0 +1,86 @@
    +  function getMappers() {
    

    needs doc block.

20. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationMapperIndex.php
    @@ -0,0 +1,86 @@
    +  public function render() {
    

    needs doc block.

    Maybe one with a nice extra paragraph explaining the why and what of how the page is being build here, with an example described of entity types and couple field instances.

21. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationMapperIndex.php
    @@ -0,0 +1,86 @@
    +    foreach ($mappers as $mapper) {
    +      $build['#rows'] = array_merge($build['#rows'], $mapper);
    +    }
    

    add comment here.

    it is making a flat array because now the order is right. but the thing we have to return needs to be a one dim array.

then we ran out of time and were tired of doing it. so most of the rest just needs docs. :P

Do we have a list of which ones still need fixing?

Titles fixed. :) (I want to re-read through the whole comment #15 next and the code)
----

----
We discussed and decided to mix the entities and field instances, and sort within that (and other lists) alphabetical by label.

After that I think this is ready for @Bojhan to look at after the sorting.

1. +++ b/lib/Drupal/config_translation/ConfigNamesMapper.php
   @@ -79,6 +79,13 @@ class ConfigNamesMapper extends DependencySerialization implements ConfigMapperI
   +   * The weight of this mapper.
   +   *
   +   * This property stores the weight of this mapper.
   

   the second line is supposed t be the type.

2. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationEntityListController.php
   @@ -0,0 +1,49 @@
   +  }
   +}
   

   classes have newline before last }

3. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
   @@ -0,0 +1,163 @@
   +   * @var
   

   needs type

4. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
   @@ -0,0 +1,163 @@
   +  public static function createInstance(ContainerInterface $container, $entity_type, array $entity_info, $definition = array()) {
   

   seems like we should type hint definition.

5. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
   @@ -0,0 +1,163 @@
   +    // It is not possible to use the standard load method, because this needs
   +    // a all field instance entities just for the given baseEntityType.
   

   a all^all

6. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
   @@ -0,0 +1,163 @@
   +   * Show bundle on field instance list pages.
   

   does not always show it.
   maybe
   Controls showing the bundle..

7. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
   @@ -0,0 +1,163 @@
   +   * @return bool
   +   */
   

   add a description.

8. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
   @@ -0,0 +1,163 @@
   +    // The bundle key is explicit defined in the entity definition.
   

   explicitly

9. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
   @@ -0,0 +1,163 @@
   +    // The defined bundle ones not match the entity type name.
   

   At this point we have 1 or 0 bundles. If we showed the bundle and the entity type name and they were the same, it would be repeated everywhere and not add info. so only show it if it does not match.

   typo: ones not match
   should be:
   does not match

10. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationFieldInstanceListController.php
    @@ -0,0 +1,163 @@
    +    return FALSE;
    

    if only considering core right now, the default to not show the bundle only happens for user.

11. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationMapperList.php
    @@ -0,0 +1,118 @@
    +   * @param \Drupal\config_translation\ConfigMapperManager $mapper_manager
    ...
    +   * @return \Drupal\config_translation\ConfigNamesMapper[]
    

    needs description line too.

12. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationMapperList.php
    @@ -0,0 +1,118 @@
    +   * Builds the config translation mappers as renderable array for
    +   * theme_table().
    

    needs to be one line summary 80 chars or less.

13. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationMapperList.php
    @@ -0,0 +1,118 @@
    +   * Builds a row for an mapper in the mapper listing.
    

    a mapper

14. +++ b/lib/Drupal/config_translation/Controller/ConfigTranslationMapperList.php
    @@ -0,0 +1,118 @@
    +   * @param $mapper
    +   * @return mixed
    

    @param needs a type.
    both need descriptions.

GO testbot.

I'd love to commit this sooner than later. If we can prioritize this and make it happen sooner than other patches, we can get this nice list UI in.

This probably needs a lot more context, whenever I land on the page I have no idea what I am looking at. Some useful help text would be useful. I also am confused that it feels like the only way into those settings, when its not - you can just get there through the entity too.

Other issues I noted:
- Translating a config thingie isn't responsive at all.
- There is no clear indication whether the left or right is the source language other than seeing the text boxes.

Reviewed with @Bojhan:

This page lists all configuration items on your site which have translatable text, like your site name, role names, etc.

Also looked at the language listing page and the translation page, and figured a help text would not help much there. The pages are pretty evident. So this is it.

Yay, committed! This still needs tests, but that can be a followup in our case. It will not be committed to core anyway without that :D

Amazing work, thanks!

Sorry for the spam, but webflo+++++++++++

do we have an issue open for the tests yet?
what specifically should we test?

maybe this is it? #2098675: Add missing tests for config_translation.mapper_list and config_translation.entity_list

Retroactively tagging.

no more todos.