Updated: Comment 13

Problem/Motivation

We have a basic consensus on moving menu links to be plugins.

default (module-provided) menu links should use YAML discover link local tasks and local actions. We want to avoid too many more changes to the DX, so let's get them into YAML files even before the plugin conversion.

Proposed resolution

USe a simple YAML discovery mechanism so the format of declaring module default menu links doesn't change, even if we radically change the underlying API.

Remaining tasks

  • Commit

User interface changes

API changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
75.36 KB

First bit.

Status: Needs review » Needs work

The last submitted patch, 1: menu_links-2226903-1.patch, failed testing.

webchick’s picture

Issue tags: +beta blocker

Since this very blatantly affects a very module developer-facing API, tagging as beta blocker. Thanks so much for taking this on!

dawehner’s picture

Status: Needs work » Needs review
FileSize
78.67 KB
4.52 KB

This could be it.

Status: Needs review » Needs work

The last submitted patch, 4: menu_links-2226903-4.patch, failed testing.

xjm’s picture

dawehner’s picture

Status: Needs work » Needs review
FileSize
86.18 KB
9.75 KB

A couple of process

Status: Needs review » Needs work

The last submitted patch, 7: menu-2226903-7.patch, failed testing.

pwolanin’s picture

Assigned: Unassigned » pwolanin

re-rollling and looking at test fails

pwolanin’s picture

1 line test fix was all, plus some minor cleanup.

pwolanin’s picture

Status: Needs work » Needs review

NR

pwolanin’s picture

FileSize
4.04 KB
87.43 KB

bah, wrong patch

pwolanin’s picture

The last submitted patch, 10: internal-url-2054011-100.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 12: menu-2226903-10.patch, failed testing.

jibran’s picture

Great work we only have two fails now.

dawehner’s picture

Status: Needs work » Needs review
FileSize
87.44 KB
745 bytes

This should be it.

pwolanin’s picture

Assigned: pwolanin » Unassigned
Issue tags: +Avoid commit conflicts

Looking good.

2 questions:

sure we use "title" instead of "link_title". I'm not sure if consistency with the local tasks and actions is good or confuding?

Also, we should probably support defining an options array in the YAML

jessebeach’s picture

FileSize
86.95 KB
28.13 KB

sure we use "title" instead of "link_title". I'm not sure if consistency with the local tasks and actions is good or confuding?

I prefer consistency. Let's please use title. I've updated the patch to the following pattern in all the YAML files.

config.sync:
  title: 'Configuration management'
  description: 'Import, export, or synchronize your site configuration.'
  route_name: config.sync
  parent: system.admin_config_development

And yes, we do need options support to do stuff like this:

views_ui.list:
  title: Views
  parent: system.admin_structure
  description: 'Manage customized lists of content.'
  route_name: views_ui.list
  options: 
    attributes:
      data_some_thing: 'sfsdfsf'
      class:
        - 'a-class-for-this-menu-item'

Question: is there a way in YAML to write the identifier data_some_thing as data-some-thing so that we don't need to convert underscores to dashes for the attribute name?

Status: Needs review » Needs work

The last submitted patch, 19: menu-2226903-19.patch, failed testing.

pwolanin’s picture

Assigned: Unassigned » pwolanin

working on fixing this - missed the alter hooks and Views

dawehner’s picture

Assigned: pwolanin » Unassigned
Issue tags: -Avoid commit conflicts

Here is a change notice: https://drupal.org/node/2228089

pwolanin’s picture

Status: Needs work » Needs review
FileSize
90.2 KB
4.63 KB

Jesse didn't change the alter hooks or Views implementation.

Also need to fix the API docs.

klausi’s picture

Issue summary: View changes
  1. +++ b/core/includes/menu.inc
    @@ -1004,14 +923,6 @@ function _menu_link_save_recursive($controller, $machine_name, &$children, &$lin
     function menu_link_get_defaults() {
    

    This function is now empty, why? Please add a comment. @todo remove it?

  2. +++ b/core/modules/menu_link/lib/Drupal/menu_link/StaticMenuLinks.php
    @@ -0,0 +1,71 @@
    + * Provides you a service which are defined in yml files and alter them.
    

    Wat? Obscure comment radar alarm!

  3. +++ b/core/modules/system/system.module
    @@ -1696,37 +1493,39 @@ function system_get_module_admin_tasks($module, $info) {
    +      // The link description, either derived from 'description' in
    +      // hook_menu() or customized via menu module is used as title attribute.
    

    hook_menu() is gone, stop lying to me.

Otherwise this looks good, the approach makes sense and I like it that the menu constants are now gone.

Status: Needs review » Needs work

The last submitted patch, 23: menu-2226903-23.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
10.97 KB
98.22 KB

Fixes the reset() functionality, menu delete form, and code comments indicated by klausi.

dawehner’s picture

  1. +++ b/core/modules/menu/lib/Drupal/menu/Form/MenuDeleteForm.php
    @@ -104,10 +104,11 @@ public function submit(array $form, array &$form_state) {
    +      ->condition('module', '', '>')
    +      ->condition('machine_name', '', '>')
    

    Just realized we can just use ->exists('module')

  2. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -369,6 +369,21 @@ public function reset() {
    +        ->execute()->fetchObject();
    ...
    +        $existing_parent = $storage->create(get_object_vars($existing_parent));
    

    Can't we fetch it directly into an array?

dawehner’s picture

Here is also a followup: #2228207: Test reset menu links

pwolanin’s picture

Documentation for exists() is unclear - only excludes NULL it seems.

pwolanin’s picture

FileSize
947 bytes
98.21 KB

here's a change to fetch as array.

klausi’s picture

Status: Needs review » Needs work
+++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
@@ -358,8 +358,8 @@ public function setRouteObject(Route $route) {
-    // definition from hook_menu_link_defaults(). Otherwise, for example, the
-    // link's menu would not be reset, because properties like the original
+    // definition from the menu_link.static service.. Otherwise, for example,
+    // the link's menu would not be reset, because properties like the original

Two ".." in here.

  1. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -369,6 +369,21 @@ public function reset() {
    +      if ($existing_parent) {
    +        /** @var \Drupal\menu_link\MenuLinkInterface $existing_item */
    +        $existing_parent = $storage->create(get_object_vars($existing_parent));
    +        $new_link->menu_name = $existing_parent->menu_name;
    +        $new_link->plid = $existing_parent->id();
    +      }
    

    That comment does not make sense, there is no $existing_item here?

  2. +++ b/core/modules/menu_link/lib/Drupal/menu_link/StaticMenuLinks.php
    @@ -10,7 +10,7 @@
    + * Provides a service which finds default menu links in yml files and alters them.
    

    Comment over 80 characters.

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/LinksTest.php
    @@ -182,7 +182,7 @@ function testMenuLinkReparenting($module = 'menu_test') {
    +   * Tests automatic reparenting of menu links defined by the menu_link.static service.
    

    same here, comment over 80 chars.

martin107’s picture

Status: Needs work » Needs review
FileSize
2.62 KB
98.22 KB

fixed issues listed above

pwolanin’s picture

Status: Needs review » Needs work

1st line comment should not wrap. tvn is working with me to clean up docs.

tvn’s picture

Assigned: Unassigned » tvn
tvn’s picture

Status: Needs work » Needs review
FileSize
98.23 KB
1.29 KB
skipyT’s picture

Status: Needs review » Reviewed & tested by the community

this looks ok.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, awesome. I enabled a bunch of modules and clicked around on the off-chance I could spot a typo somewhere that lead to a new 404 / 403, but things worked out. :) (Well, apart from I get ( ! ) Fatal error: Maximum function nesting level of '100' reached, aborting! in /Users/webchick/Sites/8.x/core/vendor/composer/ClassLoader.php on line 133
when trying to enable multiple modules at once, but this happens in HEAD, too.)

Reviewing the diff, the vast majority is doing exacty what this issue says, tho there's a bunch of bitmask removal stuff as well that now move to a new StaticMenuLinks service, and a handful of other things. I didn't see anything immediately "red flaggy," and TBH I'd prefer to get this in sooner than later and handle any problems in follow-up issues since this patch is a (*&@# and a half to re-roll and review and a *tremendous* DX improvement.

Ergo...

Committed and pushed to 8.x. YEAH!! Thanks so much for everyone's hard work on this!

  • Commit 4ac79a1 on 8.x by webchick:
    Issue #2226903 by pwolanin, dawehner, tvn, martin107, jessebeach: Step 1...
jessebeach’s picture

From my comment in #19:

And yes, we do need options support to do stuff like this:

So I spent a bit of time yesterday exploring what it would mean to include options for a link in the yml file and ultimately, I convinced myself against it. We don't want display and application level datapoints in the link declaration. These can be added on the theming layer in alter and preprocess functions.

Conclusion: I agree with this commit. It gets us unblocked for further refactoring.

dawehner’s picture

@jessebeach
I am convinced that adding attributes would actually work quite well at the moment, because all things are just
passed along to the content menu links. We can certainly discuss that in the future.

For all the other people: the approach using plugins could indeed actually allow us to let the sitebuilder choose
whether he wants menu links as content or configuration, of which the later one can be deployed. This also solves
a huge problem you had with features in the past, as this was always a pain.

Kartagis’s picture

Hi,

Even after looking at the change record, I am stumped. I even read a core module's .menu_links.yml and .routing.yml, but I couldn't get it to work. I then had to post to forum. . Here is the link to it Please advise what to do.

Regards,
K.

Edit: The issue in my queue is #2233369: Update .menu_links.yml and .routing.yml to make settings show in /admin/config/system

Berdir’s picture

Looks like we forgot to update the main routing change record: #2226903: Step 1: Move static menu links to yml files.

xjm’s picture

Title: Step 1: Move static menu links to yml files » Change record update: Step 1: Move static menu links to yml files
Status: Fixed » Active
Issue tags: +Missing change record

:)

xjm’s picture

From @berdir:

we now have three change notices, an original, an update, and then an update for the update invalidates the previous one but is written as a change to that
kay_v’s picture

getting a handle on the remaining items so e2tha-e is set up to complete things; pasting from IRC chat with @xjm

1:31:18 PM kay_v: xjm: correct that the change record work for e2tha-e on https://drupal.org/node/2226903 would be to consolidate the existing change records?
1:35:19 PM xjm: kay_v: so https://drupal.org/node/2184797 should be unpublished, https://drupal.org/node/2228089 should be updated to indicate that hook_menu_default_links() was an interim name in D8, and we should also maybe work it into the main routing change record
1:35:26 PM xjm: kay_v: sorry for the delay; had that tell sitting awhile
1:35:30 PM xjm: kay_v: let me get the other link
1:38:18 PM xjm: kay_v: https://drupal.org/node/1800686 found it finally. so that one should be updated/integrated
1:38:32 PM xjm: kay_v: and a reference to this issue added to it so they show up on each other
1:38:54 PM kay_v: xjm: thanks - looks like e2tha-e is all over it :)
1:39:00 PM xjm: kay_v: awesome :)
1:39:47 PM xjm: kay_v: e2tha-e: for now put an [OBSOLETE] in the title of https://drupal.org/node/2184797 and I can get rid of it once we're done with the rest :)

e2tha-e’s picture

Here's what I did:

kay_v’s picture

Assigned: tvn » Unassigned
Status: Active » Needs review

This appears to cover the changes that need to be made from what @kay_v and @xjm know (based on a quick chat). Can one of the folks who contributed work to it also take a look at the above change records to confirm?

xjm’s picture

Thanks @e2tha-e, great work!

pwolanin’s picture

https://drupal.org/node/1800686 seems incomplete or should link to examples for local tasks, local actions, contextual links, menu links, and theme negotiation

xjm’s picture

@pwolanin, yes, but it seems like we should file a separate issue for that, as it's out of scope for this one, aside from the static links which should be added?

jibran’s picture

+++ b/core/modules/comment/comment.menu_links.yml
@@ -0,0 +1,10 @@
+  parent: system.admin

+++ b/core/modules/comment/comment.module
@@ -164,26 +164,6 @@ function comment_theme() {
-    'parent' => \Drupal::moduleHandler()->moduleExists('node') ? 'node.content_overview' : 'system.admin',

Do we have any follow up for this? We have possible use case for contrib at http://drupalcode.org/project/devel.git/blob/refs/heads/8.x-1.x:/devel_g....
Also can we mention it in change record.

jibran’s picture

Sorry I missed this part

 function comment_menu_link_defaults_alter(&$links) {
@@ -191,6 +171,9 @@ function comment_menu_link_defaults_alter(&$links) {
     // Add comments to the description for admin/content if any.
     $links['node.content_overview']['description'] = 'Administer content and comments.';
   }
+  if (\Drupal::moduleHandler()->moduleExists('node')) {
+    $links['comment.admin']['parent'] = 'node.content_overview';
+  }
 }
dawehner’s picture

Status: Needs review » Fixed

The changes in #2226903-46: Step 1: Move static menu links to yml files are nice! It is certainly a good idea to link together the different change notices.

pwolanin’s picture

Title: Change record update: Step 1: Move static menu links to yml files » Step 1: Move static menu links to yml files

fixing title

pwolanin’s picture

Issue tags: -Missing change record

and tag...

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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