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.
Related Issues
- Thought of after #2085907: Ensure all configuration entities have an Edit/Configure tab by default.
- in config_translation #2035433: translators without edit permission, will still see an edit operation for the source, resulting in 403 access denied
- in config_translation #2095271: Add default tabs for routes expected by config_translation
- #2095137: When default tabs are more restrictive than other tabs, users with lower permissions see default tab and get permission denied
Comment | File | Size | Author |
---|---|---|---|
#27 | interdiff.txt | 4.27 KB | dawehner |
#27 | default_tab-2095117-27.patch | 30.1 KB | dawehner |
#23 | local_task-2095117-23.patch | 26.31 KB | dawehner |
#23 | interdiff.txt | 5.03 KB | dawehner |
#22 | local_task-2095117-22.patch | 25 KB | pwolanin |
Comments
Comment #1
YesCT CreditAttribution: YesCT commentedblocker for getting config_translation into core
Comment #2
Gábor HojtsyNot 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.
Comment #2.0
Gábor Hojtsyadded related issues.
Comment #2.1
Gábor HojtsyUPdated summary for new menu approach.
Comment #3
pwolanin CreditAttribution: pwolanin commentedAn 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.
Comment #4
pwolanin CreditAttribution: pwolanin commentedHere'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.
Comment #6
neclimdultab_root == 'route_name'? that's weird unclear magic to me.
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.
Comment #7
pwolanin CreditAttribution: pwolanin commentedHere's closer to an implementation (not tested), plus a fix to one of the test yml files.
Comment #9
dawehnerI do like that as it is a) easier and at the same time similar to local actions
Let's use @inheritdoc but also describe what we change: (provide default tabs)
It would be cool to have a slightly better function name, maybe just addMissingDefaultTask?
Should we fix the @todo in this issue?
Comment #10
pwolanin CreditAttribution: pwolanin commentedComment #11
pwolanin CreditAttribution: pwolanin commentedFixing the route and tab root values in the manager test.
Comment #13
pwolanin CreditAttribution: pwolanin commentedMissed changing some local task entries, but now getting a different error.
Comment #14
pwolanin CreditAttribution: pwolanin commentedrenaming the key to "base_route_name" per discussion w/ tim.plunkett and dawehner
Comment #16
dawehnerLet's see how much this fixes.
Comment #18
pwolanin CreditAttribution: pwolanin commentedugh, just set the wrong root route name for 2 tabs.
Comment #19
pwolanin CreditAttribution: pwolanin commentedwe should adds some unit or system tests of this behavior
Comment #20
dawehnerHere are a couple of more unit tests.
Comment #21
pwolanin CreditAttribution: pwolanin commentedHmm, 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.
Comment #22
pwolanin CreditAttribution: pwolanin commentedtrying to validate the route.
Comment #23
dawehnerMoved the test code to the actual place where it is needed as well as added one for the invalid route case.
Comment #24
alexpott#2095271: Add default tabs for routes expected by config_translation has landed so this patch could remove the default tabs that patch adds.
Comment #25
pwolanin CreditAttribution: pwolanin commented@alexpott - likely the tab titles will change if we fall back to the generic route titles - do we want to do that?
Comment #26
Gábor HojtsyI 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.
Comment #27
dawehnerThis 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?
Comment #28
Gábor HojtsyAlso 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.
Comment #30
Gábor HojtsyDiscussed the "how to provide a default tab title without a tab question with pwolanin":
Comment #31
pwolanin CreditAttribution: pwolanin commentedLet's postpone this and work on just the keys and grouping for now: #2107531: Improve DX of local task YAML definitions
Comment #32
Gábor HojtsyTagging as discussed as part of the config translation meeting in Prague.
Comment #32.0
Gábor Hojtsyrelated issue
Comment #33
pwolanin CreditAttribution: pwolanin commentedre-activating since #2107531: Improve DX of local task YAML definitions is committed.
Comment #34
pwolanin CreditAttribution: pwolanin commentedComment #35
hass CreditAttribution: hass commentedClosed #2275845: Local "Settings" tab missing if "Translate @type" tab is added as duplicate.
Comment #36
pwolanin CreditAttribution: pwolanin commentedvery little of the last patch actually applies now - probably needs a reworking by hand.