Taxonomy vocabulary translation local task tab not shown in UI

I noticed this bug while working on #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions. The solution is to alter the local tasks, the way the field_ui module does it. See the MR for the issue I am working on.

Thanks to @danflanagan8 for helping me find this issue.

Here's a fail test to get things rolling. I'm working on a fix but I'm haven't figured out how to get the local task to show up right without changing its path to /admin/structure/taxonomy/manage/[vid]/overview/translate, with the added "overview" in there. Currently the path is /admin/structure/taxonomy/manage/[vid]/translate. Changing it seems like it would be a small BC no-no.

This might be helped if #3245487: Change Vocabulary overview-form and edit-form paths to be more consistent with other core entity types and improve the information architecture is fixed.

Moreover, I would argue that instead of a dependency on the edit form route, Content Translation should just do what #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions and rely on the field_ui_base_route property, some of the ground-work in that issue made doing that easier. That would then address the UI issue for Taxonomy but also any other module that uses a non-standard pattern.

Depending on what happens with #3245020: Rename the field_ui_base_route entity property, that property may be made more generic anyway.

Here's a fix that just alters the base route of local task in a hook_local_tasks_alter.

I was initially trying to change the base route earlier in a hook_config_translation_info_alter(), but that resulted in the path of the entity.taxonomy_vocabulary.config_translation_overview getting changed as described in #14.

Thanks for popping in here, @AaronMcHale.

I would argue that instead of a dependency on the edit form route, Content Translation should just...rely on the field_ui_base_route property

I think you're probably right that this would be the most robust solution. Can we address the translation tab now and make a follow-up to address the underlying cause? What do you think?

One thing that frightens me a little bit about expanding the scope is that there appears to be very little test coverage for the existence of the local tasks for config translation. For the fail test here I added an assertion to ConfigTranslationListUiTest. That test is really about checking for the Translation operations though. It would probably be good to add a similar assertion to each of the ~15 helper functions in that class to test for local tasks. And that makes me wonder if there's any other basic coverage missing that should be added before we attempt to make such a significant change to the config translation module.

It occurred to me that I wasn't asserting the local task url, just that the text was there. I've updated the test in this patch.

Update: After uploading this I thought, "Oh, no, the link with that url could be a breadcrumb..." But the breadcrumbs block is not on the page. So if a link with that url exists, that link is definitely the local task.

Unassigning from @naresh_bavaskar. I was still working on this as noted in #14.

Here is the comment that I wrote last night, previewed, and then forgot to submit. (I needed to get some sleep.)

@danflanagan8:

A failing test is a good place to start!

+    // Test if the local task for translation on this page.
+    $this->assertSession()->linkExists('Translate taxonomy vocabulary');

The comment is not a complete sentence.

We should not have to change the paths for this issue, just the local tasks. The patches in #2 and #3 on this issue changed paths. Perhaps we should change the paths, but that is a separate issue: #3245487: Change Vocabulary overview-form and edit-form paths to be more consistent with other core entity types and improve the information architecture. I am adding that as related to this one. Also #3245020: Rename the field_ui_base_route entity property while I am at it.

Now that I am looking at related issues, I see #2358923: Config translation module cannot deal with different base path is still open, and it is very relevant. On the other hand, the discussion there may be obsolete: from the end of #24 there, it looks as though they did not have field_ui_base_route during that discussion.

In Drupal\config_translation\Plugin\Derivative\ConfigTranslationLocalTasks::getDerivativeDefinitions(), I see

      $base_route = $mapper->getBaseRouteName();
      if (!empty($base_route)) {
        // ...
        $this->derivatives[$route_name]['base_route'] = $base_route;
      }

That is what we have to fix: the base_route entry in the local task. It is complicated because we have this extra level of abstraction/indirection with the mapper class.

I think the place to fix this is in config_translation_config_translation_info(). I have to be careful because both config entities and content entities are both entities, and I do not have good names for them. For example, $entity_type_id could be node_type or it could be node. In the context of this function, think of $entity_type_id as node_type or taxonomy_vocabulary, the bundle of some content entity type.

1. Check $entity_type->getBundleOf() to see if there is a related content entity type.
2. If so, load that entity type. (I.e., get its definition.)
3. Look for field_ui_base_route on the content entity type. If found, then use that for the base route.

I think that is the right solution: fix the problem for any entity that is the bundle of another entity that defines field_ui_base_route. But an alternative is a more narrow fix. The taxonomy module could implement hook_config_translation_info() or hook_config_translation_info_alter().

One last thing: be careful what you ask for! When there are too many tabs, the display is not very pretty. We should consider shortening the labels on the tabs. I saw some discussion on Slack today about dropping "Manage". (It is repetitive and redundant.) I do not think there is an issue yet for that suggestion.

I see that @AaaronMcHale already added one of the related issues I meant to add.

It looks as though the patch in #19 is similar to the "more narrow fix" that I suggested, although it uses a different hook.

In #14 and #18, you point out that using the config_translation_info hook (or the alter variant) affects the path. Yes, that is a BC break. That is because Drupal\config_translation\ConfigNamesMapper::getOverviewRoute() uses getBaseRoute(). So the safe thing to do is to use the local_tasks_alter hook as in #19.

Maybe we can fix the problem more generally, using the local_tasks_alter hook in the config_translation module and changing the local task for any bundle whose related content type specifies field_ui_base_route.

Thanks for popping in here, @AaronMcHale.

I would argue that instead of a dependency on the edit form route, Content Translation should just...rely on the field_ui_base_route property

I think you're probably right that this would be the most robust solution. Can we address the translation tab now and make a follow-up to address the underlying cause? What do you think?

Thanks to Benji for referencing #2358923: Config translation module cannot deal with different base path. Looking at the patch, yeah I think it's probably fine to just fix it for Taxonomy in this issue. Perhaps we could add a todo referencing that issue, and in that issue we should look to replace it with a generic solution, if we can, assuming that #3245487: Change Vocabulary overview-form and edit-form paths to be more consistent with other core entity types and improve the information architecture also lands.

The comment is not a complete sentence.

Can't argue with that!

Here's a new patch that corrects that comment. I also tightened up the assertion I added in #19 so that there's no way we could get a false positive.

Perhaps we could add a todo referencing that issue

I didn't add a to-do because I'm not totally sure what will need to be to-done. Will taxonomy_local_tasks_alter() need to be revised? Will it need to be removed? I'm happy to add one if someone has a specific suggestion. I don't understand the major refactoring/reworking being anticipated on a deep enough level.

I didn't add a to-do because I'm not totally sure what will need to be to-done. Will taxonomy_local_tasks_alter() need to be revised? Will it need to be removed? I'm happy to add one if someone has a specific suggestion. I don't understand the major refactoring/reworking being anticipated on a deep enough level.

As far as I can tell *, once #3245487: Change Vocabulary overview-form and edit-form paths to be more consistent with other core entity types and improve the information architecture and #2358923: Config translation module cannot deal with different base path are done (assuming that one adds support for field_ui_base_route for bundle config entities), taxonomy_local_tasks_alter() may no longer need to exist, because it then should all "just work". So in theory we might be able to get to a point where that can just be removed completely ;)

* This is all based on remembering what the patches in each of these issues will do, it's been a long day...

I still get surprised by the test runner too often. There's an extra subdirectory that threw off my assertion. I found a pattern that's used elsewhere in core and updated the assertion to follow it.

I also added a @todo. I think it's accurate while still being non-committal.

This is a small bufix and reading #21 it looks like the implementation here is in the right place. It also has test coverage. This looks great.

Re-uploading patch to get testing on 9.5.x.

Colleague just ran into this, lets fix it. Moved #30 to a MR

Hide patches

Cleaned up the issue summary to make it more clear. At first wasn't clear what I was testing.

But MR does address the issue and has coverage.

Committed and pushed 8c5fad9808 to 11.x and b2dc1c1010 to 10.3.x and 3ef010360b to 10.2.x. Thanks!