Problem/Motivation

Some settings pages have default tabs. We recently introduced default tabs on config entities at all times in #2085907: Ensure all configuration entities have an Edit/Configure tab by default, but that is not true for general settings pages. It is hard in core and contrib to extend settings pages, because you never can be sure if it already has an edit tab or not and may need to add one of your own. Eg. 'Account settings' has a default 'Settings' tab because it expects if you have Field UI module enabled, it would add more tabs on it. If that would not be the case, it would not have a default 'Settings' tab. Now if you look at Site information or RSS settings, those don't *expect* additional tabs, so they don't have a default tab.

This is wrong.

Making core assumptions about which pages may be extended in contrib, so those where this expectation was not there may end up contrib modules trying to add default tabs with different paths/labels, etc. is a problem.

Proposed resolution

The tab system should be intelligent enough that if there is no default tab, it should add one. It could take the label (and sub-path segment) of that from the parent route with a new syntax. Alternatively people need to manually define a default tab at all times on all routes.

Remaining tasks

Decide if we can/want to autogenerate or want to go manual.

User interface changes

No ui changes for core. Enabling contrib modules that want to add more tabs will make the default tab show up. However until such a module is enabled, the default tab would not show.

API changes

Figure out.

Files: 
CommentFileSizeAuthor
#27 interdiff.txt4.27 KBdawehner
#27 default_tab-2095117-27.patch30.1 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,081 pass(es), 23 fail(s), and 7 exception(s). View
#23 local_task-2095117-23.patch26.31 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,149 pass(es). View
#23 interdiff.txt5.03 KBdawehner
#22 local_task-2095117-22.patch25 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 59,159 pass(es). View
#22 2095117-20-22.increment.txt7.19 KBpwolanin
#20 local_task-2095117-20.patch22.57 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,277 pass(es). View
#20 interdiff.txt8.05 KBdawehner
#18 2095117-18.patch17.19 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 59,092 pass(es). View
#18 2095117-16-18.increment.txt902 bytespwolanin
#16 2095117-16.patch17.2 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,852 pass(es), 11 fail(s), and 12 exception(s). View
#16 interdiff.txt1.71 KBdawehner
#14 2095117-14.patch17.18 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 59,008 pass(es), 14 fail(s), and 109 exception(s). View
#14 2095117-13-14.increment.txt13.17 KBpwolanin
#13 2095117-13.patch15.33 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,840 pass(es), 42 fail(s), and 56 exception(s). View
#13 2095117-11-13.increment.txt5.87 KBpwolanin
#11 2095117-11.patch14.43 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 59,052 pass(es), 10 fail(s), and 12 exception(s). View
#11 2095117-9-11.increment.txt4.44 KBpwolanin
#10 2095117-9.patch11.34 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 59,230 pass(es), 10 fail(s), and 12 exception(s). View
#10 2095117-7-9.increment.txt3.73 KBpwolanin
#7 2095117-7.patch9.3 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,949 pass(es), 4 fail(s), and 0 exception(s). View
#7 2095117-4-7.increment.txt3.84 KBpwolanin
#4 2095117-4.patch7.31 KBpwolanin
FAILED: [[SimpleTest]]: [MySQL] 58,733 pass(es), 17 fail(s), and 24 exception(s). View

Comments

YesCT’s picture

Issue tags: +blocker

blocker for getting config_translation into core

Gábor Hojtsy’s picture

Title: Ensure all config *pages* have a (hidden) Edit/Configure tab by default » Menu system should provide a default tab if none exists
Issue tags: -blocker

Not a blocker for config_translation, we will have a stopgap for the three pages we need in #2095271: Add default tabs for routes expected by config_translation. This should apply to all menu elements though as per discussion with @alexpott and @xjm.

Gábor Hojtsy’s picture

Issue summary: View changes

added related issues.

Gábor Hojtsy’s picture

Issue summary: View changes

UPdated summary for new menu approach.

pwolanin’s picture

An easy approach to this would be to bake it into the manager, or as a derivative generator.

The problem we'd need to solve is the potential performance hit of loading a tab on every page even when it would be alone and not rendered. Uless we do away with the current run-time alter hook (need feedback maybe from the VDC team about its use) then it's hard to make that optimization simply.

If we can do away with the run-time alter, then the manager can simply omit up-front every tab that's alone from what's proporsed for access checks and rendering.

pwolanin’s picture

Status: Active » Needs review
FileSize
7.31 KB
FAILED: [[SimpleTest]]: [MySQL] 58,733 pass(es), 17 fail(s), and 24 exception(s). View

Here's a first patch.

converts tab_too_id as a plugin ID to tab_root as a route name.

This should make it much easier to provide a default. The loss is only the unused/undocumented feature of having multiple table groups on one page.

I'm not sure what kind of behavior we get if 2 different tabs say they are on the same route - that could be fun.

Status: Needs review » Needs work

The last submitted patch, 2095117-4.patch, failed testing.

neclimdul’s picture

Status: Needs work » Needs review
  1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskDefault.php
    @@ -141,7 +141,7 @@ public function getTitle() {
         if (!isset($this->pluginDefinition['weight'])) {
    -      if ($this->pluginDefinition['tab_root_id'] == $this->pluginDefinition['id']) {
    +      if ($this->pluginDefinition['tab_root'] == $this->pluginDefinition['route_name'] && empty($this->pluginDefinition['tab_parent_id'])) {
             $this->pluginDefinition['weight'] = -10;
           }
           else {
    diff --git a/core/lib/Drupal/Core/Menu/LocalTaskManager.php b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
    

    tab_root == 'route_name'? that's weird unclear magic to me.

  2. +++ b/core/modules/comment/comment.local_tasks.yml
    @@ -1,15 +1,15 @@
     comment_permalink_tab:
       route_name: comment_permalink
       title: 'View comment'
    -  tab_root_id: comment_permalink_tab
    +  tab_root: comment_permalink
     comment_edit_page_tab:
       route_name: comment_edit_page
       title: 'Edit'
    -  tab_root_id: comment_permalink_tab
    +  tab_root: comment_permalink
       weight: 0
    

    I like tab_root over tab_root_id.

    I don't like that to figure out what the root is, you have to find where the route is registered instead of just finding the top level id. Unnecessary indirection which for me makes it harder to decipher the hierarchy.

pwolanin’s picture

FileSize
3.84 KB
9.3 KB
FAILED: [[SimpleTest]]: [MySQL] 58,949 pass(es), 4 fail(s), and 0 exception(s). View

Here's closer to an implementation (not tested), plus a fix to one of the test yml files.

Status: Needs review » Needs work

The last submitted patch, 2095117-7.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
    @@ -39,8 +39,8 @@ class LocalTaskManager extends DefaultPluginManager {
    +    // The route name where the root tab appears.
    +    'tab_root' => '',
    

    I do like that as it is a) easier and at the same time similar to local actions

  2. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
    @@ -127,6 +127,48 @@ public function processDefinition(&$definition, $plugin_id) {
    +   * Finds plugin definitions.
    +   *
    +   * @return array
    +   *   List of definitions to store in cache.
    +   */
    

    Let's use @inheritdoc but also describe what we change: (provide default tabs)

  3. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
    @@ -127,6 +127,48 @@ public function processDefinition(&$definition, $plugin_id) {
    +  protected function addMissingTab($definition) {
    

    It would be cool to have a slightly better function name, maybe just addMissingDefaultTask?

  4. +++ b/core/lib/Drupal/Core/Menu/LocalTaskManager.php
    @@ -127,6 +127,48 @@ public function processDefinition(&$definition, $plugin_id) {
    +    // @todo: Load/validate the route object and get the title.
    

    Should we fix the @todo in this issue?

pwolanin’s picture

Status: Needs work » Needs review
FileSize
3.73 KB
11.34 KB
FAILED: [[SimpleTest]]: [MySQL] 59,230 pass(es), 10 fail(s), and 12 exception(s). View
pwolanin’s picture

FileSize
4.44 KB
14.43 KB
FAILED: [[SimpleTest]]: [MySQL] 59,052 pass(es), 10 fail(s), and 12 exception(s). View

Fixing the route and tab root values in the manager test.

Status: Needs review » Needs work

The last submitted patch, 2095117-11.patch, failed testing.

pwolanin’s picture

FileSize
5.87 KB
15.33 KB
FAILED: [[SimpleTest]]: [MySQL] 58,840 pass(es), 42 fail(s), and 56 exception(s). View

Missed changing some local task entries, but now getting a different error.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
13.17 KB
17.18 KB
FAILED: [[SimpleTest]]: [MySQL] 59,008 pass(es), 14 fail(s), and 109 exception(s). View

renaming the key to "base_route_name" per discussion w/ tim.plunkett and dawehner

Status: Needs review » Needs work

The last submitted patch, 2095117-14.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
17.2 KB
FAILED: [[SimpleTest]]: [MySQL] 58,852 pass(es), 11 fail(s), and 12 exception(s). View

Let's see how much this fixes.

Status: Needs review » Needs work

The last submitted patch, 2095117-16.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
902 bytes
17.19 KB
PASSED: [[SimpleTest]]: [MySQL] 59,092 pass(es). View

ugh, just set the wrong root route name for 2 tabs.

pwolanin’s picture

Issue tags: +Needs tests

we should adds some unit or system tests of this behavior

dawehner’s picture

FileSize
8.05 KB
22.57 KB
PASSED: [[SimpleTest]]: [MySQL] 59,277 pass(es). View

Here are a couple of more unit tests.

pwolanin’s picture

Hmm, you changed the manager so we add the extra tabs before the alter hook? I thought we should run the alter hook before, so that we don't try to add a default tab if one was already added via alter.

However, either approach could work.

pwolanin’s picture

FileSize
7.19 KB
25 KB
PASSED: [[SimpleTest]]: [MySQL] 59,159 pass(es). View

trying to validate the route.

dawehner’s picture

FileSize
5.03 KB
26.31 KB
PASSED: [[SimpleTest]]: [MySQL] 59,149 pass(es). View

Moved the test code to the actual place where it is needed as well as added one for the invalid route case.

alexpott’s picture

#2095271: Add default tabs for routes expected by config_translation has landed so this patch could remove the default tabs that patch adds.

pwolanin’s picture

@alexpott - likely the tab titles will change if we fall back to the generic route titles - do we want to do that?

Gábor Hojtsy’s picture

I think @alexpott said in our meeting that we should be able to provide a tab title on the parent route/tab(?) and fall back on the parent title if not provided. Not sure how that is feasible.

dawehner’s picture

FileSize
30.1 KB
FAILED: [[SimpleTest]]: [MySQL] 58,081 pass(es), 23 fail(s), and 7 exception(s). View
4.27 KB

I think @alexpott said in our meeting that we should be able to provide a tab title on the parent route/tab(?) and fall back on the parent title if not provided. Not sure how that is feasible.

This is sort of a general difference between the old menu system and the new router system. In the old menu systems properties in general have been inherited from the parent to the child path,
while on the other hand this is not the case at all currently in the router system. Maybe though this is a thing worth discussion before we readd this for a special case here.

This adds a new webtest and converts the manually created default tabs. Should we remove them already in this patch?

Gábor Hojtsy’s picture

Also there may not be a tab for the parent (as in case of the site settings for example). There will be a router item, but not a tab. So not sure if it is nice from an architectural perspective to put tab info on the route, so we can generate default tabs without writing manual tabs.

Status: Needs review » Needs work

The last submitted patch, default_tab-2095117-27.patch, failed testing.

Gábor Hojtsy’s picture

Discussed the "how to provide a default tab title without a tab question with pwolanin":

[3:19pm] pwolanin: GaborHojtsy: you seem to be contradicting yourself
[3:19pm] GaborHojtsy: pwolanin: I'm saying we are generating default tabs here
[3:19pm] GaborHojtsy: pwolanin: prior comments indicated we would need a way to provide that tab title different from the parent route title
[3:19pm] GaborHojtsy: pwolanin: since we have no parent tab in most cases (we are not generating a default *sub*tab), we need some way to source that title from *without* manually defining the tab
[3:21pm] pwolanin: GaborHojtsy:  why?  if you want to specify it you have to define the tab
[3:21pm] GaborHojtsy: pwolanin: yeah, which is as I quoted not what alexpott said 
[3:22pm] pwolanin: GaborHojtsy:  I still am not sure
[3:22pm] pwolanin: GaborHojtsy:  you either get a magic default OR you have control by defining it if it's not there
[3:22pm] pwolanin: GaborHojtsy:  and/or you can define the alter hook
[3:25pm] pwolanin: GaborHojtsy:  please comment again and clarify
[3:27pm] GaborHojtsy: pwolanin: hard to clarify because it is not something I proposed or would push for 
[3:27pm] pwolanin: GaborHojtsy: no?  well alexpott seemed to think it was better DX
[3:29pm] GaborHojtsy: pwolanin: lol, I'm still referring to where you source the default tab title, not the whole issue 
[3:30pm] GaborHojtsy: pwolanin: I agree the whole issue a big step up
[3:30pm] pwolanin: GaborHojtsy: wait - above you said it was bad?
[3:30pm] GaborHojtsy: pwolanin: the discussion on the issue (and as far as I'm concerned here) is about where you source the tab title
[3:30pm] GaborHojtsy: pwolanin: bad, what?
[3:31pm] pwolanin: GaborHojtsy:  no
[3:31pm] pwolanin: GaborHojtsy: the issue is abotu atuo-generating a tab
[3:31pm] pwolanin: GaborHojtsy:  we don't have any via option except _title
[3:31pm] pwolanin: viable option
[3:32pm] GaborHojtsy: pwolanin: right, in the hard problems meeting alexpott said we could add _default_tab_title, which I'm commenting on the issue would put some of the responsibility of tabs within routes, which is what I'm commenting on the issue may not be a good idea
[3:32pm] pwolanin: GaborHojtsy: oh - I didn't recall that suggestion
[3:32pm] GaborHojtsy: pwolanin: "I think @alexpott said in our meeting that we should be able to provide a tab title on the parent route/tab(?) and fall back on the parent title if not provided. Not sure how that is feasible."
[3:32pm] pwolanin: GaborHojtsy:  indeed - we shouldn't mix this stuff up
[3:32pm] GaborHojtsy: pwolanin: from the issue
[3:32pm] pwolanin: GaborHojtsy: ok, I see - right - the notion of "parent" isn't operative
pwolanin’s picture

Status: Needs work » Postponed

Let's postpone this and work on just the keys and grouping for now: #2107531: Improve DX of local task YAML definitions

Gábor Hojtsy’s picture

Issue tags: +Prague Hard Problems

Tagging as discussed as part of the config translation meeting in Prague.

Gábor Hojtsy’s picture

Issue summary: View changes

related issue

pwolanin’s picture

Issue summary: View changes
Status: Postponed » Active

re-activating since #2107531: Improve DX of local task YAML definitions is committed.

pwolanin’s picture

Status: Active » Needs work
hass’s picture

pwolanin’s picture

very little of the last patch actually applies now - probably needs a reworking by hand.

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.