Problem/Motivation
#2095117: Menu system should provide a default tab if none exists would automate creating default tabs on pages, but we need immediate solutions for the 'Site settings', 'RSS settings', 'Site maintenance settings' pages, where we add translation tabs.
Proposed resolution
Add Settings tabs for now. #2095117: Menu system should provide a default tab if none exists would automate this for good and not need us to apply this everywhere. It would be painful to apply this manually, automated is the ultimate goal, but this cannot be a blocker for config_translation getting in, and the edit tab autogeneration there is considered to be a WTF.
Remaining tasks
Review :)
User interface changes
None. If there is no other tab, the Settings tab will not show.
Modules can add tab easily, like config_translation. See #36 for screenshots.
API changes
None.
Related Issues
#2095117: Menu system should provide a default tab if none exists
Comment | File | Size | Author |
---|---|---|---|
#36 | head.png | 8.16 KB | kfritsche |
#36 | before.png | 7.99 KB | kfritsche |
#36 | after.png | 11.97 KB | kfritsche |
#38 | drupal-no-modules.png | 7.77 KB | kfritsche |
#37 | both_head.png | 10.34 KB | kfritsche |
Comments
Comment #1
Gábor HojtsyComment #2
Gábor HojtsyComment #4
Gábor Hojtsy#1: settings-tabs.patch queued for re-testing.
Comment #5
Gábor HojtsySame thing in YAML so we can actually put tabs on this with the new discovery. Should do the same thing.
Comment #7
Gábor Hojtsy#5: settings-tabs-yaml.patch queued for re-testing.
Comment #9
Gábor HojtsyPer @tstoeckler no need for weight, it defaults properly.
Comment #11
Gábor HojtsyShould use route_name.
Comment #12
YesCT CreditAttribution: YesCT commentedok.
https://drupal.org/node/2044515 is the change notice Local tasks are provided by plugins implementing LocalTaskInterface instead of type MENU_LOCAL_TASK in hook_menu()
and says:
"Note that the confusing concept/type MENU_DEFAULT_LOCAL_TASK is now gone - the "root" tab simply lists it's own ID as the tab_root_id, and a second level tab can link to the same route as its parent."
So. each of these use their own tab plugin id as their tab_root_id and that is fine.
we can make up any plugin id, since it is discovery by yml
---
change notice says should place the file in the module that is responsible for the form/path
the site information page is at admin/config/system/site-information
ag "admin/config/system/site-information" core
gives:
so that tells me that the system module defines it.
maintenance settings are at
admin/config/development/maintenance
looking that up gives:
core/modules/system/system.routing.yml
path: '/admin/config/development/maintenance'
also system module.
rss feeds settings is at
admin/config/services/rss-publishing
and looking up gives:
core/modules/system/system.routing.yml
path: '/admin/config/services/rss-publishing'
so this can all go in the same yml in system.
change notice says the pattern for the filename should be "file name pattern MODULENAME.local_tasks.yml"
so system.local_tasks.yml is fine.
------
I do not see in the change notice what the pattern should be for the plugin id
the example in the change notice uses all underscores like:
user_login_tab:
but...
so should it be . or _ ?
I looked for another MODULE.local_tasks.yml
comment uses _
but the route uses .
comment.routing.yml
has
why does it work?
because in comment.module , it still has
so maybe it is not checking the comment.local_tasks.yml and that file is broken, but we dont know it because those local tasks are also in the comment hook_menu implementation.
---
but views_ui.local_tasks.yml uses all _ for the plugin id,
the route in the local_tasks.yml uses a . and that matches the route id in the routing.yml
checked views_ui.module to see if it was really using the local_tasks.yml of it was ignore it.
does not define a local task. so it *is* actually using the local_tasks.yml
-----
the change notice says the keys should be
route_name:
title:
tab_root_id:
this is ok in the patch
----------
I checked the route name key values for all three, comparing them to what is in system.routing.yml and they are fine.
------
title is Settings for all three settings pages. This follows the pattern set already in core on the account settings page, which has a Settings tab.
----
===========
so all looks good. does it work?
without the patch, node#overlay=admin/config/services/rss-publishing
does not have a tab.
with the patch,
does not have a tab.
good.
to test it, patched config_translation with #2095269: Remove add_edit_tab, core should have default tabs, enabled the module
and... uh. no tabs. :(
---
we can come back to this tomorrow.
Comment #13
YesCT CreditAttribution: YesCT commentedI may have had my env confused. retesting.
also, @tim.plunkett says that . is the separator we should use right now as the module name separator in the plugin id in the local_tasks.yml
so the change notice example should be updated. I will edit it now.
Comment #14
YesCT CreditAttribution: YesCT commentedtim and I are working on this
Comment #15
tim.plunkettWorking on this
Comment #16
YesCT CreditAttribution: YesCT commentedComment #17
tim.plunkettStill needs tests, but found some fun bugs.
Comment #18
tim.plunkettTurns out I came to the same conclusion as #2095139: Checking for the active tab should use raw variables..
Comment #19
tstoecklerCare to comment on that one? I don't think it warrants a code comment (although it maybe can't hurt?), but I'm interested :-)
Yeah I don't really know what to do about this and #2095139: Checking for the active tab should use raw variables.. Since that one has tests, it seems we should postpone this on that. But that would be bad, since this is a blocker in itself. Hmm...
Comment #20
tstoeckler(So to be clear, the "Hmm..." above means I almost set this to RTBC, but didn't quite feel audacious enough)
Comment #21
tim.plunkettThis needs tests, please don't RTBC :)
I absolutely need to prove why the !isset() must be an empty(), will do, and will add a code comment.
Comment #22
YesCT CreditAttribution: YesCT commentedneeds work for #21 and tests
also kind of postponed on #2095139: Checking for the active tab should use raw variables.
Comment #23
tstoecklerSo I was trying to add test coverage for the !isset() vs. empty() discussed above, but the only way that I could trigger a notice, was by calling
check_markup('asdsad', FALSE)
, i.e. pass FALSE as the $format_id. In that case you get the infamousI can certainly test that, but I'm not really sure that we want to support passing FALSE as something that's called $id. I guess making our functions less brittle is a good goal in general, but I haven't seen this sort of protection elsewhere. It might also be that there is a different bug, but I wasn't able to discover it. Anyway, I'd like some feedback on that before pursuing this any further.
Comment #24
tim.plunkettThis is what I had when I found the bug... This excludes fixes from the previous patch.
Comment #26
tim.plunkett#24: tabs-fail.patch queued for re-testing.
Comment #28
tim.plunkettOh, well that other issue went in.
Comment #29
tstoecklerIt seems this is a bug-fix, i.e. the local tasks shouldn't be working right now currently. Is that correct? If so, we should some test coverage. (I could do that.)
So hook_menu() had some automation for setting -10 on MENU_DEFAULT_LOCAL_TASK, but I'm relatively sure that local_tasks.yml does not have that. So I guess we should keep that in the YAML for now. Not 100% sure on this one, though.
These should match.
This looks weird. It seems it should be the same route before and after, right?
I'll see if I can cook up some tests for the actual tabs and everything.
Comment #30
Gábor HojtsyIf you gonna fix this here, although unrelated, you should remove comment_menu() entries for this, which is what made work.
Comment #31
YesCT CreditAttribution: YesCT commented#1981368: Convert all routes to 'module_name.foo_bar' naming convention makes it sound like it was done already...
Comment #32
YesCT CreditAttribution: YesCT commentedI talked to tim. I'll work on this.
Comment #33
YesCT CreditAttribution: YesCT commented#30
yes, let us not fix the route names here. They will be fixed as a followup to #1981368-33: Convert all routes to 'module_name.foo_bar' naming convention or maybe #2100213: take local tasks out of hook_menu so it uses comment.local_tasks.yml with the recommended route naming conventions
yieks this stuff is inter-related and a bit confusing.
#29 2.
comment_routing.yml has the weights, yeah, lets keep it too.
#29 3.
between #11 and #17
the root id for maintenance changed. I think by accident? checked the change notice https://drupal.org/node/2044515 Local tasks are provided by plugins... and made it match.
#29 4.
in
user.routing.yml
yeah, we are taking out list and adding list. I think this was due to a cut and paste of the filter file. :)
fixing.
----
is this still needs tests?
Comment #35
YesCT CreditAttribution: YesCT commented@dawehner let me know the indent caused the fail.
just fixes that.
Comment #36
kfritschePatch seems reasonable for me and I have nothing to complain here.
I did a test with the config_translation module:
Drupal HEAD:
Drupal HEAD + config_translation HEAD:
That it works in here is because of a workaround in config_translations, which has to be removed.
After Patch #35 for Drupal + config_translation without workaround:
After Patch #35 for Drupal + After I re-rolled #2095269: Remove add_edit_tab, core should have default tabs for config_translation:
In conclusion this patch works fine. RTBC.
Comment #37
kfritscheAttached wrong image and forgot to set RTBC.
Comment #38
kfritscheOkay and a last screenshot. Just Drupal without config_translation.
Comment #39
YesCT CreditAttribution: YesCT commentedtalked with @tim.plunkett about needing tests.
they were added in #2095139: Checking for the active tab should use raw variables.
Comment #40
tstoecklerAwesome! Thanks YesCT and kfritsche for keeping this going! RTBC++
Comment #41
alexpottCommitted e4314ec and pushed to 8.x. Thanks!
Comment #42
Gábor HojtsyYay, thanks!
Comment #43.0
(not verified) CreditAttribution: commentedAdded link to #36 in UI changes.