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.

#2095117: Menu system should provide a default tab if none exists

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

FileSize
1.58 KB
Gábor Hojtsy’s picture

Status: Active » Needs review

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint, -language-config, -blocker

The last submitted patch, settings-tabs.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
Issue tags: +D8MI, +sprint, +language-config, +blocker

#1: settings-tabs.patch queued for re-testing.

Gábor Hojtsy’s picture

FileSize
721 bytes

Same thing in YAML so we can actually put tabs on this with the new discovery. Should do the same thing.

Status: Needs review » Needs work
Issue tags: -D8MI, -sprint, -language-config, -blocker

The last submitted patch, settings-tabs-yaml.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

#5: settings-tabs-yaml.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8MI, +sprint, +language-config, +blocker

The last submitted patch, settings-tabs-yaml.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
676 bytes

Per @tstoeckler no need for weight, it defaults properly.

Status: Needs review » Needs work

The last submitted patch, settings-tabs-yaml-v2.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
691 bytes

Should use route_name.

YesCT’s picture

ok.
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:

core/modules/system/system.routing.yml
  path: '/admin/config/system/site-information'

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...

cat core/modules/user/user.routing.yml 
user.register:
  path: '/user/register'

so should it be . or _ ?

I looked for another MODULE.local_tasks.yml

comment uses _

comment_permalink_tab:
  route_name: comment_permalink
  title: 'View comment'
  tab_root_id: comment_permalink_tab

but the route uses .

comment.routing.yml
has

comment.permalink:
  path: '/comment/{comment}'
  defaults:
    _controller: '\Drupal\comment\Controller\CommentController::commentPermalink'
  requirements:
    _entity_access: 'comment.view'

why does it work?

because in comment.module , it still has

  $items['comment/%comment/view'] = array(
    'title' => 'View comment',
    'type' => MENU_DEFAULT_LOCAL_TASK,
  );
  // Every other comment path uses %, but this one loads the comment directly,
  // so we don't end up loading it twice (in the page and access callback).
  $items['comment/%comment/edit'] = array(
    'title' => 'Edit',
    'type' => MENU_LOCAL_TASK,
    'route_name' => 'comment.edit_page',
  );
  $items['comment/%comment/approve'] = array(
    'title' => 'Approve',
    'weight' => 10,
    'route_name' => 'comment.approve',
  );
  $items['comment/%comment/delete'] = array(
    'title' => 'Delete',
    'type' => MENU_LOCAL_TASK,
    'route_name' => 'comment.confirm_delete',
    'weight' => 20,
  );

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,

views_ui_settings_basic_tab:
  route_name: views_ui.settings_basic
  title: Basic
  tab_root_id: views_ui_list_tab
  tab_parent_id: views_ui_settings_tab

the route in the local_tasks.yml uses a . and that matches the route id in the routing.yml

views_ui.settings_basic:
  path: '/admin/structure/views/settings'
  defaults:
    _form: '\Drupal\views_ui\Form\BasicSettingsForm'
  requirements:
    _permission: 'administer views'

checked views_ui.module to see if it was really using the local_tasks.yml of it was ignore it.

  // when there is a route-aware breadcrumb builder.
  $items['admin/structure/views/settings'] = array(
    'title' => 'Settings',
    'route_name' => 'views_ui.settings_basic',
    'type' => MENU_VISIBLE_IN_BREADCRUMB,
  );

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.

YesCT’s picture

I 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.

YesCT’s picture

Assigned: Unassigned » YesCT

tim and I are working on this

tim.plunkett’s picture

Assigned: YesCT » tim.plunkett

Working on this

YesCT’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests
tim.plunkett’s picture

Title: Have default "Settings" tabs on three major settings screens » Add default tabs for routes expected by config_translation
Status: Needs work » Needs review
FileSize
5.32 KB

Still needs tests, but found some fun bugs.

tim.plunkett’s picture

Turns out I came to the same conclusion as #2095139: Checking for the active tab should use raw variables..

tstoeckler’s picture

  1. +++ b/core/modules/filter/filter.module
    @@ -589,7 +584,7 @@ function filter_format_allowcache($format_id) {
    -  if (!isset($format_id)) {
    +  if (empty($format_id)) {
    

    Care 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...

tstoeckler’s picture

(So to be clear, the "Hmm..." above means I almost set this to RTBC, but didn't quite feel audacious enough)

tim.plunkett’s picture

This 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.

YesCT’s picture

Status: Needs review » Needs work

needs work for #21 and tests

also kind of postponed on #2095139: Checking for the active tab should use raw variables.

tstoeckler’s picture

So 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 infamous

Warning: array_flip(): Can only flip STRING and INTEGER values! in Drupal\Core\Config\Entity\ConfigStorageController->loadMultiple() (line 136 of core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php).

I 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.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.96 KB

This is what I had when I found the bug... This excludes fixes from the previous patch.

Status: Needs review » Needs work
Issue tags: -Needs tests, -D8MI, -sprint, -language-config, -blocker

The last submitted patch, tabs-fail.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#24: tabs-fail.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +D8MI, +sprint, +language-config, +blocker

The last submitted patch, tabs-fail.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
3.26 KB

Oh, well that other issue went in.

tstoeckler’s picture

  1. +++ b/core/modules/comment/comment.local_tasks.yml
    @@ -1,14 +1,14 @@
    -  route_name: comment_permalink
    +  route_name: comment.permalink
    ...
    -  route_name: comment_edit_page
    +  route_name: comment.edit_page
    ...
    -  route_name: comment_confirm_delete
    +  route_name: comment.confirm_delete
    

    It 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.)

  2. +++ b/core/modules/filter/filter.module
    @@ -149,11 +149,6 @@ function filter_menu() {
    -    'weight' => -10,
    

    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.

  3. +++ b/core/modules/system/system.local_tasks.yml
    @@ -0,0 +1,14 @@
    +system.site_maintenance_mode_tab:
    ...
    +  tab_root_id: system.site_maintenance_mode_settings_tab
    

    These should match.

  4. +++ b/core/modules/user/user.local_tasks.yml
    @@ -0,0 +1,4 @@
    +  route_name: user.role_edit
    
    +++ b/core/modules/user/user.module
    @@ -791,12 +791,6 @@ function user_menu() {
    -    'route_name' => 'user.role_list',
    

    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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
+++ b/core/modules/comment/comment.local_tasks.yml
@@ -1,14 +1,14 @@
 comment_permalink_tab:
-  route_name: comment_permalink
+  route_name: comment.permalink
   title: 'View comment'
   tab_root_id: comment_permalink_tab
 comment_edit_page_tab:
-  route_name: comment_edit_page
+  route_name: comment.edit_page
   title: 'Edit'
   tab_root_id: comment_permalink_tab
   weight: 0
 comment_confirm_delete_tab:
-  route_name: comment_confirm_delete
+  route_name: comment.confirm_delete
   title: 'Delete'
   tab_root_id: comment_permalink_tab
   weight: 10

If you gonna fix this here, although unrelated, you should remove comment_menu() entries for this, which is what made work.

YesCT’s picture

YesCT’s picture

Assigned: tim.plunkett » YesCT

I talked to tim. I'll work on this.

YesCT’s picture

Assigned: YesCT » Unassigned
Status: Needs work » Needs review
FileSize
2.04 KB
2.56 KB

#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

user.role_list:
  path: '/admin/people/roles'
  defaults:
    _entity_list: 'user_role'
  requirements:
    _permission: 'administer permissions'

user.role_add:
  path: '/admin/people/roles/add'
  defaults:
    _entity_form: user_role.default
  requirements:
    _permission: 'administer permissions'

user.role_edit:
  path: '/admin/people/roles/manage/{user_role}'
  defaults:
    _entity_form: user_role.default
  requirements:
    _entity_access: user_role.update

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?

Status: Needs review » Needs work

The last submitted patch, tabs-2095271-33.patch, failed testing.

YesCT’s picture

Status: Needs work » Needs review
FileSize
396 bytes
2.56 KB

@dawehner let me know the indent caused the fail.

just fixes that.

kfritsche’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
11.97 KB
7.99 KB
8.16 KB

Patch seems reasonable for me and I have nothing to complain here.

I did a test with the config_translation module:

Drupal HEAD:

drupal-no-modules.png

Drupal HEAD + config_translation HEAD:

both_head.png
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:

head.png

After Patch #35 for Drupal + After I re-rolled #2095269: Remove add_edit_tab, core should have default tabs for config_translation:

after.png

In conclusion this patch works fine. RTBC.

kfritsche’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
10.34 KB

Attached wrong image and forgot to set RTBC.

kfritsche’s picture

FileSize
7.77 KB

Okay and a last screenshot. Just Drupal without config_translation.

YesCT’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

talked with @tim.plunkett about needing tests.

they were added in #2095139: Checking for the active tab should use raw variables.

tstoeckler’s picture

Awesome! Thanks YesCT and kfritsche for keeping this going! RTBC++

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed e4314ec and pushed to 8.x. Thanks!

Gábor Hojtsy’s picture

Issue tags: -sprint

Yay, thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Added link to #36 in UI changes.