Over on #2351991-85: Add a config entity for a configurable, topic-based help system, I tried to make the new help topic config entity use the "canonical" path as the base path for config translation.

I thought all I would need to do is:

function help_config_translation_info_alter(&$info) {
  $info['help_topic']['base_route_name'] = 'entity.help_topic.canonical';
}

However, this doesn't work, because the config translation module has a bunch of other spots where it is assuming that the base route is the edit form page, not the canonical page for the entity.

This assumption seems wrong. If the config translation module is going to have a hook to provide the base route, it should be getting that information from the hook/alter always.

Things not currently working right (that have this assumption):

  1. The edit links on the config translation overview page for a particular entity object go to the base route. They should be going to the edit page specifically.
  2. config_translation_entity_type_alter():
    if ($entity_type->hasLinkTemplate('edit-form')) {
            $entity_type->setLinkTemplate('drupal:config-translation-overview', 'config_translation.item.overview.' . $entity_type->getLinkTemplate('edit-form'));
          }
    

    this needs to be using the base route name set up in the hook/alter, not assuming it's the edit form.

  3. Maybe others?
Files: 
CommentFileSizeAuthor
#17 interdiff.txt2.06 KBGábor Hojtsy
#17 2358923-base-vs-edit-16.patch5.08 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,958 pass(es), 0 fail(s), and 2 exception(s). View
#15 interdiff.txt1.1 KBGábor Hojtsy
#15 2358923-base-vs-edit-15.patch4.53 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,965 pass(es), 4 fail(s), and 2 exception(s). View
#14 interdiff.txt8.3 KBGábor Hojtsy
#14 2358923-base-vs-edit-14.patch4.51 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,956 pass(es), 4 fail(s), and 2 exception(s). View
#6 2358923-base-vs-edit.patch7.72 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,551 pass(es), 4 fail(s), and 2 exception(s). View

Comments

tstoeckler’s picture

Yes, you are 100% correct. These are bugs, plain and simple.

Your bug reports are really invaluable, thanks a lot for hunting that down!!!

Gábor Hojtsy’s picture

Issue tags: +sprint, +language-config
vijaycs85’s picture

Issue summary: View changes

Thanks for raising it @jhodgdon.

1. Just checked the implementation. It looks like the edit link on overview page using the base_path from hook_config_translation_info() already. Here is the trace: ConfigTranslationController::itemPage => ConfigNameMapper->getBaseRouteName() => $this->pluginDefinition['base_route_name'].

2. Looks like there is a circular dependency between hook_config_translation_info and hook_entity_info_alter which is why we use edit-form instead of base route from discovery. However this shouldn't give any issue as config_translation_config_translation_info uses same edit-form link as base_route_name.

Gábor Hojtsy’s picture

@vijaycs85: I am not sure I understand your second point?!

I don't think we can/should use the canonical route from the entity by default because for most entities, that would be either frontend or nonexistent (so far in core). What is the canonical URL to a block content type, user role or vocabulary. Well, so far none. I grepped and could not find a config entity with a canonical path in core. (Please correct if mistaken).

So we need to let it be specified but not change the default behaviour. The question is how shoul entity types be able to do this? Should they implement a ConfigurationTranslationRouteInterface or something along those lines to provide a method with a return value of a different route? Should their entity info have a link template key for a config translation base route (which by default would be a copy of the edit route but alterable)?

jhodgdon’s picture

Ummmm... let's step back.

First: you should be able to specify the base route for the translations, using hook_config_translation_info_alter(), and if you do that, whenever the config translation module is making routes/links to the translate page, it should be using the specified base route, not assuming that the base route is always the edit link for the entity.

Second: if config translation module is making links to an edit page, it should be using the configured edit link for the entity, not assuming it can use whatever the base route is for translation.

The problem is that neither of these is true (and the issue summary shows two examples of spots where these are violated). The config translation module is sometimes going back and using the edit route instead of the base route, and sometimes it is using its base route instead of the edit route, because it always implicitly assumes they are the same.

Gábor Hojtsy’s picture

Status: Active » Needs review
FileSize
7.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,551 pass(es), 4 fail(s), and 2 exception(s). View

So we need to introduce yet one more set of route methods to the mappers then that the entity mapper can override. The naming is tricky, because other route methods are related to the translation. We have so far Base, Overview, Add, Edit and Delete, where the base route is the non-translation route, and all others are translation overview, translation add, translation edit, translation delete.

In this patch I introduced an Edit type (and renamed the existing Edit to Translate). I think this is confusing because now Edit does not edit the thing that Add and Delete deal with... Then renaming every route method would be a pretty huge API shakeup.

We can use this route method in the overview form to generate the edit link. The entity mapper then retrieves the edit link route from the entity info direct and not the config translation info.

Is this sufficient separation of edit vs. base? Lets figure out the best naming scheme.

Status: Needs review » Needs work

The last submitted patch, 6: 2358923-base-vs-edit.patch, failed testing.

jhodgdon’s picture

I don't really think it's necessary to introduce more link types, and I think it's fine if you have to use a hook to override the base path. I thought the problem was that the config translation module is not using its own routes and entity link definitions in a consistent way?

Gábor Hojtsy’s picture

My patch is not introducing a new link type either?! The config translation works with mappers, because it is largely agnostic what is is translating. Eg. if you alter a config entity form with 3 more settings but that is stored in a different config file then you will alter the mapper as well to also translate that config key on the translate tab. So while the core defaults deal with groups of config keys or a config key for a config entity for translation, the mappers make this more fluid and alterable so they support use cases that people do with settings forms.

So the mappers need to know their routes and delegate route lookups are needed. For now they take the routes from the config translation definition but that cannot differentiate between an original edit route on an entity and the base route of the config translation. It assumes they are the same. So either the config translation definition needs to have more keys or the mappers need to delegate lookup of paths in the entity mapping. My attached patch does the delegation. Either way the mapper needs its dedicated methods for looking up that route and params, which is also what I introduced in the patch and what necessitates a holistic review of the terminology used on the mappers.

Does this help enlighten the situation?

jhodgdon’s picture

Issue summary: View changes

A few thoughts, beyond fixing the patch so the tests pass -- mostly I think this looks good if we can get it to work.

a) I was a bit confused by the documentation in ConfigMapperInterface, because of the distinction between "edit a translation" and "edit the config itself":

  /**
   * Returns route name for the edit route.
   *
   * @return string
   *   Route name for the mapper.
   */
  public function getEditRouteName();

  /**
   * Returns the route parameters for the edit form route.
   *
   * @return array
   */
  public function getEditRouteParameters();

  /**
   * Returns the route object for the edit form route.
   *
   * @return \Symfony\Component\Routing\Route
   *   The route object for the translation page.
   */
  public function getEditRoute();

I guess these are for the form for editing the base config, right? They're in the middle of a bunch of methods for deleting translations, adding translations, etc. so it's a bit confusing. Maybe some clearer docs would help?

b) I'm also a bit confused as to why the Entity mapper class doesn't override getEditRouteParameters() along with the other two methods. Can we be sure that the entity class doesn't need route parameters for its edit form? Shouldn't this be taken from the entity class rather than making assumptions?

c) In ConfigTranslationController, I think this is another spot where we need a patch:

  public function itemPage(Request $request, RouteMatchInterface $route_match, $plugin_id) {

...
        // Check access for the path/route for editing, so we can decide to
        // include a link to edit or not.
        $edit_access = $this->accessManager->checkNamedRoute($mapper->getBaseRouteName(), $route_match->getRawParameters()->all(), $this->account);

I think that should be using the getEditRoute* methods?

d) I think we also need a patch for config_translation.module here:

function config_translation_entity_type_alter(array &$entity_types) {

...

      if ($entity_type->hasLinkTemplate('edit-form')) {
        $entity_type->setLinkTemplate('drupal:config-translation-overview', 'config_translation.item.overview.' . $entity_type->getLinkTemplate('edit-form'));
      }
    }
  }

e) It might be interesting to write a test module that would attempt to do something like what I was trying to do with the help topic:

function help_config_translation_info_alter(&$info) {
  $info['help_topic']['base_route_name'] = 'entity.help_topic.canonical';
}

to set the config translation base route on a config entity, and then see if you can access the translate pages and all of the links on them.

Gábor Hojtsy’s picture

Yeah we now have Base, Translate, Overview, Add, Edit and Delete. But the Translate, Overview, Add and Delete deal with translations, while the Base and Edit do not. So we have two routes that are other than translation routes and 4 that are translation routes. Naming them properly is key, but this would mean a complete naming overhaul. Not sure if the issue being resolved justifies such a big rename this late...

So the question is should we do that or add the new routes with a different naming scheme, eg. Base and BaseEdit?

jhodgdon’s picture

I like BaseEdit, because it reminds the user that the Edit route is for the base entity not the translation. And a clearer piece of documentation that says it is for editing the entity (since the others say "... translation ..." like "route for adding a translation" I think).

The Base route is on the entity, but what it is really being used for is both "the base path to add suffixes to for all the translation paths" and "the base route to add tabs/links to so that Translate tab shows up", right? So yes it's a route on the entity, but it's being used for translation purposes as well as the basis for the paths.

In contrast, the only thing the Edit route is being used for (or should be, anyway) is to make contextual/tab/etc. links.

Gábor Hojtsy’s picture

Assigned: Unassigned » Gábor Hojtsy
Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
4.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,956 pass(es), 4 fail(s), and 2 exception(s). View
8.3 KB

This just moved getTranslate* back to getEdit* as they were before and names the new ones getBaseEdit*. No other changes.

Gábor Hojtsy’s picture

FileSize
4.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,965 pass(es), 4 fail(s), and 2 exception(s). View
1.1 KB

Actually the "translation edit" sounds better on the interface than "translation".

The last submitted patch, 14: 2358923-base-vs-edit-14.patch, failed testing.

Gábor Hojtsy’s picture

FileSize
5.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,958 pass(es), 0 fail(s), and 2 exception(s). View
2.06 KB

@jhodgdon re 10:

a) Trying to address that with a minor update to docs, but also its now names BaseEditRoute* and placed right after BaseRoute*. So should be much better?
b) I don't see a way to delegate these to entities? They have url related methods but no route related methods?
c) Fixed.
d) What to do there? How do we know on the entity info alter level that they want to be base off of not the edit route? Is it safe to assume if the entity has a canonical route, then we should use that? What ensures that is an admin route and around the same location UI-wise as the edit route?
e) To be done later.

The last submitted patch, 15: 2358923-base-vs-edit-15.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 17: 2358923-base-vs-edit-16.patch, failed testing.

jhodgdon’s picture

The new docs/method names look good to me!

I still think there may be some holes in the logic and assumptions, as outlined in #10... but I could be wrong. I'll be convinced that this is done when:
- This patch comes up green on the existing test suite.
- I can use patched Core to make the View page for help topic entities be the canonical page instead of Edit (what triggered this issue in the first place). I'll be happy to test this once the existing test suite comes up green (currently there are two exceptions).
- Ideally there's a test added here that replicates that and verifies all the UI stuff works; the test should fail without the patch and pass with the patch.

Gábor Hojtsy’s picture

Looks like the exceptions are due the field entity mappers not working exactly as it should. It generates the edit template runtime based on whether the field_ui module is enabled and what field type it is, so not in the entity info directly, therefore cannot find in lookup from the entity key. Duh. So this may need a bigger rework on how links / routes are handled / generated in the config translation module, because config entities do not let us access to the link templates themselves. We can hasLinkTemplate($key) and $keys = uriRelationships() (note link templates are also called URI relationships), but not $value = getLinkTemplate($key). We can only get a derivative value via urlInfo()... :/

jhodgdon’s picture

Yeah. If it had been a three-line patch, I would have fixed it on the other issue. These assumptions and problems are built into the module at a pretty deep level.

Gábor Hojtsy’s picture

Yeah the crux of the problem is the entity API does not think we should get routes and route params while the config translation API was written at a time we were supposed to use those things to base URL generation off of. We can delegate the URL generation to the config translation mappers now but that will be a massive API change. Not yet seeing how the field config translation mapper could be solved otherwise based on what the entity API provides now. Or we modify the entity API to give direct access to those routes, that would be a simple change but I assume against the philosophy of how URLs would be dealt with for entities...

jhodgdon’s picture

Well. If this is too complicated to fix, we could:
a) Document that the base route for translating is *always* the edit route, and cannot be anything else.
b) Consolidate the module so that it isn't leaving you thinking you can do anything else (like not have separate variables/etc. for the translation base path/route).

I'm not sure about the entity API and routes though... that's being looked at on:
#2281645: Make entity annotations use link templates instead of route names
And incidentally, I think Field UI has the same problem (needing routes for the bundles in order to set up the Manage Fields and Manage Displays pages), which no one seems to want to listen to on that other issue.

I mean really, you can't do anything with just paths in Drupal 8 -- everything needs routes. So if you're trying to add task links, action links, etc. to the UI, you need the routes. If the Entity API doesn't want the routes to be known, the Entity API is not allowing other modules to add tasks, actions, etc. to its UI, which is not really OK.

Gábor Hojtsy’s picture

Assigned: Gábor Hojtsy » Unassigned
Issue tags: -sprint

I don't have the time to work on this unfortunately :/

jhodgdon’s picture

Hm. Now that entity routes are more standardized, I wonder if this patch becomes easier or harder?

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

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should 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.

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should 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.