Problem/Motivation

Moving to a route-based (instead of path based) routing systems means we need to rethink the way menu links are defined. Since the route name (machine name) is now the genuine identifier, a hierarchy based on a notion of relative paths no longer makes sense, and using the path as the unique key no longer makes sense. In addition, moving that definition of menu links to a separate (and clearly defined) hook would make #2044969: Break the old router for contrib potentially much easier.

Proposed resolution

  • Define a new hook_menu_link_defaults()
  • Key each link by a machine name - initially the same as the current system path replacing "/" with "." and adding the module name in front
  • Add a column to the database table so the machine name can be saved in the database for later lookups/resets
  • Make the hierarchy explicit by having each link define its parent
  • Don't check access links via this hook except for route route-based, hence some some links to unconverted pages will be temporarily accessible to everyone
  • Carry out a full conversion in 2 phases:
    1. Copy all existing hook_menu entries to this new hook and get them working : leave the old ones in place to avoid constant conflicts with other patches. Get that committed.
    2. Remove hook_menu implementations as long as all have been converted to routes. This is a follow-up issue: #2177041: Remove all implementations of hook_menu

Something like this:

<?php
/**
 * Define some default menu links to be saved on behalf of my module.
 */
function hook_menu_link_defaults() {
 
$links = array();
 
$links['views_ui.admin.structure.views'] = array(
    
'link_title' => 'Views',
    
'route_name' => 'views_ui.list',
    
'route_parameters' => array(),
  );
 
$links['views_ui.admin.structure.views.add'] = array(
   
'link_title' => 'Add view',
   
'route_name' => 'views_ui.add',
   
'parent' => 'views_ui.admin.structure.views',
  );
  return
$links;
}
?>

Remaining tasks

Final reviews/commit

API changes

New hook, and removal of any code related to saving links from hook_menu.

Issues Unblocked by this Patch

Files: 
CommentFileSizeAuthor
#287 2047633-menu-links-287.patch139.8 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 63,278 pass(es).
[ View ]
#287 increment.txt974 bytespwolanin
#286 2047633-menu-links-286-just-reoll.patch139.58 KBpwolanin
PASSED: [[SimpleTest]]: [MySQL] 63,198 pass(es).
[ View ]

Comments

pwolanin’s picture

Title:Create a hook_menu_link() or hook_route_link() and mark hook_menu() deprecated to » Create a hook_menu_link() or hook_route_link() and mark hook_menu() deprecated or whitelist only core to use it while converting
pwolanin’s picture

better title

amateescu’s picture

I love this idea. It's the only sane way to finally decouple menu links from old router items.

We should also try to invoke this hook from the new menu_link module, and move _menu_navigation_links_rebuild() and friends to the same module while we're at it.

dawehner’s picture

I am wondering whether we should replace a hook_menu_link() (however we call it) with a .menu_link.yml file and build something similar as routes, but this is just an idea.

The advantage is that people have a more common way to build things related with routing/menu (which people can't distinct that easy as we think). If you need non-static menu links, you just go with a event subscriber and act on certain events.

pwolanin’s picture

@dawehner - I think the upgrade to a similar hook instead of a yaml file would be easier and maybe sends the message that links are quite a different part of Drupal than routing?

I also think plenty of modules will continue to want to do some dynamic link generation here, rather than e.g. using menu_link_maintain()

dawehner’s picture

At least we should put the hook_menu_link into a separate include file, so it is not loaded on every request. This is one of these decisions which are hard to make, especially because every solution as advantages/disadvantages.

pwolanin’s picture

Actually - if we have a format for exported menu link entities, maybe it would make sense to define them that way? we'd make it all more clearly exportable and deployable.

From discussion in IRC, it sounds like that format would need to be tweak a bit so we could specify the parent using a machine name rather than a not-yet-generated UUID.

Crell’s picture

We're eliminating manual include files everywhere else. Let's not re-introduce it here.

I'm flexible on moving the remnants of hook_menu() to a new hook, but that's an API change so unless it fulfills https://drupal.org/node/2045345 I doubt that we could justify it.

pwolanin’s picture

I think this crosses the "required to fix it sanely" threshold.

tim.plunkett’s picture

Assigned:Unassigned» catch
Priority:Normal» Major

Let's see. (I agree.)

dawehner’s picture

We're eliminating manual include files everywhere else. Let's not re-introduce it here.

I dont' talk about manual include files but rather about a hook_hook_info entry.

catch’s picture

Assigned:catch» Unassigned

I'm finding it hard to have an opinion on this one.

Is the main reason to rename the hook to break it for un-ported modules? Or to make it clearer that it's only defining menu links?

There's no 'exported' menu links format - they're content entities rather than config entities, and that format would need to reflect relationships between them etc. which aren't in hook_menu() atm but get added via processing or the UI, so that's not really an option I don't think - for this we should just keep more or less the same thing as we have now and revisit again for D9.

pwolanin’s picture

So, the change would make it very easy and clean to white-list core's remaining hook_menu implementations and break unported modules.

Also, this name change would make the purpose clear and reduce developer confusion.

webchick’s picture

I'm leaning towards a won't fix on this (at least as far as renaming hooks goes) unless it's really necessary for #2044969: Break the old router for contrib, but I don't think so.

The rest (deprecating the APIs for contrib) seems like it's handled in that issue, so we should probably centralize discussion over there, and close this as a duplicate.

amateescu’s picture

I'm very much in agreement with @pwolanin here, changing the hook name would make it very visible when porting a module to D8 that you need to check out the new routing system *now*, not in a few months when we'll hopefully be able to remove the old one and your code will break.

It also makes it consistent with the new module and entity type that we have in D8, which is 'menu_link', not 'menu' (that's a different entity type).

I guess the answer to @catch in #12 is: pretty much both :) Also, Peter has a plan to make the parent relationship explicit in hook_menu_link() definitions, which I also think is a nice goal. Of course, that relationship will not be taken into consideration on menu rebuilds if the menu link was customized in the UI.

To sum up, I think this is a very good change to make and we should do it as soon as possible.

webchick’s picture

There are other ways to accomplish that that don't invalidate documentation and break APIs post-API freeze, though. For example:

<?php
if (!in_array($module, $core_modules)) {
  if (
in_array($key, $list_of_deprecated_properties)) {
    throw new
Exception("Bad.");
  }
}
?>
amateescu’s picture

What exactly is 'that'? :)

pwolanin’s picture

@webchick - let's be honest. There is still substantial API change going on with all the menu and routing code. It's maybe 30% done. Let's not make the final D8 worse and the current tasks harder if changing this hook names is the right thing to do.

ParisLiakos’s picture

+1
Totally agree with amateescu and pwolanin

dawehner’s picture

Adding #2051889: Add route parameters property to menu links as a dependency to get something going. The link generator issue is also quite important: #2047619: Add a link generator service for route-based links

pwolanin’s picture

Title:Create a hook_menu_link() or hook_route_link() and mark hook_menu() deprecated or whitelist only core to use it while converting» Move definition of menu links to hook_menu_link()

Clarifying title

pwolanin’s picture

Issue summary:View changes

Updated issue summary.

pwolanin’s picture

Issue summary:View changes

Updated issue summary.

pwolanin’s picture

Issue summary:View changes

Updated issue summary.

pwolanin’s picture

Issue summary:View changes

Updated issue summary.

pwolanin’s picture

Title:Move definition of menu links to hook_menu_link()» Move definition of menu links to hook_default_menu_links()

So, we should be clear for a better DX that this hook is proving default content (which may be laster altered by the user).

Better yet would be something like hook_default_menu_links()?

geerlingguy’s picture

I like hook_default_menu_links(), since that seems to map to the end result better. I do remember, vividly, the confusion I had around all the various magic that hook_menu() contained when I started writing modules for Drupal. It took time to realize I was dealing with both the definition of menu items, menu routes, and menus themselves, in a way.

Renaming the function would make it much clearer as to what the hook is actually doing, IMO.

geerlingguy’s picture

Issue summary:View changes

updated resultion

pwolanin’s picture

Issue summary:View changes

Updated issue summary.

pwolanin’s picture

Issue summary:View changes

Updated issue summary.

pwolanin’s picture

Discussed this plan in IRC with catch and alexpott

Both were in support of the general idea of moving links to another hook. Discussed the plan in the issue summary with catch in detail of 2 phases of adding the new hook and then removing entries from the old one in a follow-up patch. This would minimize the number of conflicts and let us do a big-bang conversion. Also discussed the possibility of skipping access checks for anything not converted to a route yet to as to avoid needed to re-code things that we're removing soon.

pwolanin’s picture

StatusFileSize
new5.8 KB

for context attached (with permission) is the raw IRC chat w/ catch.

plach’s picture

For reference, this will help also with completing #1810350-50: Remove menu_* entity keys and replace them with entity links.

pwolanin’s picture

Status:Active» Needs review
StatusFileSize
new43.56 KB
FAILED: [[SimpleTest]]: [MySQL] 56,362 pass(es), 633 fail(s), and 20,015 exception(s).
[ View ]

This is still very rough, but a start on converting system module links and the underlying menu.inc code.

effulgentsia’s picture

+++ b/core/modules/system/system.module
@@ -955,6 +957,204 @@ function system_menu() {
+  $items['admin/config/regional'] = array(
+    'link_path' => 'admin/config/regional',
+    'link_title' => 'Regional and language',
+    'description' => 'Regional settings, localization and translation.',
+    'weight' => -5,
+  );
...
+++ b/core/modules/system/system.module
@@ -955,6 +957,204 @@ function system_menu() {
+  $items['admin/config/search'] = array(
+    'link_title' => 'Search and metadata',
+    'link_path' => 'admin/config/search',
+    'parent' => 'admin/config',
+    'description' => 'Local site search, metadata and SEO.',
+    'weight' => -10,
+  );

1. Why does the first not have a 'parent', but the second does?
2. Since the keys no longer have any direct relation to URLs, should we change them to use some other delimiter than "/" (the issue summary currently has "_")?
3. Should we default 'parent' to the key name up to the last instance of the chosen delimiter? If not, should we then make our key names even shorter, like 'system.search' instead of 'admin_config_search'?

effulgentsia’s picture

Title:Move definition of menu links to hook_default_menu_links()» Move definition of menu links to hook_default_menu_links(), decouple key name from path, and make 'parent' explicit
Issue tags:+API change

Oh, and adding the "API change" tag to this issue. This still needs approval from a maintainer. #25 does not look to me like actual approval. FWIW, I agree with the change, because it looks like we need to still make API changes to hook_menu() anyway, such as changing the keys (link machine names) from paths to something else, and adding an explicit 'parent' key for each link. Since these are new changes that haven't already been communicated (as opposed to the ones that have, such as removing 'page callback'), then it's a post-API-freeze BC break either way, so we might as well pick a hook name we like, and I think hook_default_menu_links() has some DX/clarity advantages over hook_menu(), per #23.

effulgentsia’s picture

Also, FWIW, I like #4's suggestion of moving this into a non-CMI YAML file, as we have for routing, instead of a hook, but #12 rejected that.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new11.29 KB
new44.17 KB
FAILED: [[SimpleTest]]: [MySQL] 57,449 pass(es), 417 fail(s), and 174 exception(s).
[ View ]

This missing parent for a couple was a bug. This patch is working a little better. Still only includes system.module

YAML vs hook I don't really care, but I think converting to a hook is a little easier given where we are starting from and for module updates.

pwolanin’s picture

StatusFileSize
new46.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,288 pass(es), 415 fail(s), and 174 exception(s).
[ View ]

Some fixes plus start adding user module.

pwolanin’s picture

StatusFileSize
new4.82 KB
Xano’s picture

One thing that we may want to think about is that we want to get rid of hooks in Drupal 9. Converting this to a different hook in D8 and then converting it to another format in D9 while keeping the actual functionality the same will hurt BC. Instead, converting it to a format that will likely stay in D9 (yml, plugins?) now will less likely break BC then. Let's not do two 'lesser' API changes in two Drupal versions instead of a big one in one Drupal version.

twistor’s picture

+1 to converting to an yaml file. This would probably also need an Event ala RouteSubscriber for dynamic menus, but not sure on that.

If that's too big of an api change, can we at least add it to hook_hook_info()? hook_menu() is often one of the largest hooks and 95% of the time it's just clutter.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new13.77 KB
new48.67 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

After discussion in IRC w/ Crell, twistor, Xano and others. Moving ahead to make it clearer that machine names are != path by replacing "/" with "."

Also adds links for contact and dblog.

pwolanin’s picture

StatusFileSize
new5.56 KB
new49.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Fix up the user and logout links and rename $items to $links

pwolanin’s picture

StatusFileSize
new4.63 KB
new52.05 KB
FAILED: [[SimpleTest]]: [MySQL] 57,367 pass(es), 395 fail(s), and 170 exception(s).
[ View ]

Trying to get the logic better for repeated rebuilds where the hook_info changes.

plus add node and comment links.

pwolanin’s picture

Issue summary:View changes

Updated issue summary.

pwolanin’s picture

Issue summary:View changes

Updated issue summary.

pwolanin’s picture

StatusFileSize
new7.1 KB
new55.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu-links-2047633-41.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

revert changes not needed in SystemController and add a couple more modules.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new55.57 KB
FAILED: [[SimpleTest]]: [MySQL] 57,406 pass(es), 394 fail(s), and 170 exception(s).
[ View ]

re-roll for conflict in filter module

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new2.57 KB
new55.83 KB
FAILED: [[SimpleTest]]: [MySQL] 45,010 pass(es), 6,863 fail(s), and 929 exception(s).
[ View ]

silly me - I re-enabled a broken update function in the patch. letting sleeping dogs lie, plus removing one DB column.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new884 bytes
new56.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links_rebuild-2047633-47.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Let's see how much this fixes.

Note: this code is just involved in {menu_router}, so running this code does not really matter as the menu_name should not be used in the {menu_router} table.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new57.53 KB
FAILED: [[SimpleTest]]: [MySQL] 58,408 pass(es), 367 fail(s), and 53 exception(s).
[ View ]

Rerolled.

pwolanin’s picture

Issue tags:+WSCCI

not sure why the testbot removed this tag...

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new59.7 KB
FAILED: [[SimpleTest]]: [MySQL] 58,311 pass(es), 345 fail(s), and 15 exception(s).
[ View ]

Just some work everywhere.

dawehner’s picture

Most or at least many of the issues are caused by menu_link_get_preferred() working just on menu links but not on things like local tasks anymore.

pwolanin’s picture

Great, well those should mostly be resolved by the breadcrumb patch?

dawehner’s picture

Well, we should have the foundations for it, but maybe it would be kind of hard to share the information between the title bit and the breadcrumb bit, but yeah the breadcrumb patch is nearly ready anyway.

pwolanin’s picture

We should do away with setting the title based on the active trail. I think that was a bad decision in 6.x

dawehner’s picture

Yeah the inheriting of options in the menu tree is just wrong. Add the title/title callback every would do it.

So we basically need on issue which converts all titles.

Xano’s picture

As discussed with @pwolanin at the extended sprints in Prague, I opened #2094481: Support default content entities to try and solve the definition and import of default content entities on a more generic level.

pwolanin’s picture

Note that we need to build in support for default translation of link titles. However, that depends on entityNG for menu_links

pwolanin’s picture

Issue summary:View changes

Updated issue summary.

jeff h’s picture

I am wondering where this issue got up to... I'd dearly like to see this get into D8.

Specifically, as a module developer I fully support:

* breaking hook_menu so people are forced to get up to speed on the D8 separation between menu items and routes... a distinction that makes a lot of sense. There are already good blog posts on this topic so D7 developers should be able to pick this up easily.

* the 'parent' key makes so much sense — an implicit structure is inherent in menu items and this is easy to comprehend.

* as it is currently, the D8 hook_menu items are keyed by the route's URL path, which I think is confusing since the items directly reference the route anyway.

Does it have approval from a core maintainer as yet?

pwolanin’s picture

@jeff h - it's got implicit approval, has just been on the back burner.

Sadly, at least 6 hunks fail to apply now - working on a re-roll.

pwolanin’s picture

Issue summary:View changes
StatusFileSize
new55.62 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links-2047633-63.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's a re-rolled patch via rebase, but almost certainly broken due to changed route names, etc. At least a basis for trying to fix things.

pwolanin’s picture

StatusFileSize
new55.51 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links-2047633-64.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new14.14 KB

Fixing some route names. Seems to be blowing up on an entity query though.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new55.61 KB
FAILED: [[SimpleTest]]: [MySQL] 58,690 pass(es), 241 fail(s), and 16 exception(s).
[ View ]
new55.61 KB

Let's first ensure that the installation works again.

dawehner’s picture

StatusFileSize
new55.97 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

This is a new version, which fixes a couple of the failures.

One thing we might should do in the meantime is to get titles on all the routes and not rely on hook_menu() anymore.

dawehner’s picture

Crell’s picture

Issue tags:+beta blocker
dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new86.06 KB
FAILED: [[SimpleTest]]: [MySQL] 57,531 pass(es), 163 fail(s), and 3,748 exception(s).
[ View ]

This now converts all the entries which just uses hook_menu() and not hook_menu_alter() ... let's first see the test failures.

Drupal installs fine, so we don't get a massive amount, especially now that the title issue is solved, yeah!

dawehner’s picture

StatusFileSize
new24.01 KB
new92.65 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Introduced an alter hook and converted the views implementation to it.

webchick’s picture

Just out of curiosity, why a hook? This seems very similar to hook_image_default_styles in D7, which we converted to config entities in D8.

webchick’s picture

Hm. I guess cos they're content entities, not config entities. Still, this same question (or slight variations of) have been asked multiple times in this thread, and it'd probably be good for the issue summary to offer more of an explanation why this is seen as the best route forward. I do think it's a bit of a DX fail to define one half of the menu link in YAML and the other half in PHP, but I guess this is what catch asked for in #12?

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new92.5 KB
FAILED: [[SimpleTest]]: [MySQL] 58,939 pass(es), 136 fail(s), and 3,894 exception(s).
[ View ]
new958 bytes

Just out of curiosity, why a hook? This seems very similar to hook_image_default_styles in D7, which we converted to config entities in D8.

I think we should switch to a really similar model like routing, so have static yml files but also allows dynamic/altered items.
The main reason why this patch haven't made that is because the hard part of the patch is getting it green and using hooks certainly requires the least amount of changes. I would suggest to get it green and then switch to a better format and discuss that in the meantime?

The failure was that you should not register menu links for non-static routes (routes with a required parameter), as you cannot generate a URL for it.

pwolanin’s picture

If we want a system for providing default content entities, that would be great, but wold seem like a significant D8 addition. We should get this in as a hook and then see if it would be appropriate to build a more generic systme.

I am not sure it would be reasonable to make such a new/big API a beta blocker.

webchick’s picture

Yeah, I guess my main issue is trying not to introduce more API changes at this point in the cycle, so trying to get the format right the first time. It sounds like though like we might just have to live with shipping with this as a hook in D8 and revisit other options in D9.

One note on the patch itself:

+++ b/core/modules/views_ui/views_ui.module
@@ -36,6 +36,31 @@ function views_ui_menu() {
+  // Top-level Views module pages (not tied to a particular View).
+  $links['admin.structure.views'] = array(
+    'link_title' => 'Views',
+    'parent' => 'admin.structure',
+    'description' => 'Manage customized lists of content.',
+    'route_name' => 'views_ui.list',
+  );
+
+  // A page in the Reports section to show usage of plugins in all views.
+  $links['admin.reports.views-plugins'] = array(
+    'link_title' => 'Views plugins',
+    'parent' => 'admin.reports',
+    'description' => 'Overview of plugins used in all views.',
+    'route_name' => 'views_ui.reports_plugins',
+  );

*Having* to specify the parent is a bit unfortunate. :\ It'd be nice if it could do something automagical based on the dot notation (e.g. "admin.structure.XXX" gets its parent set behind the scenes to "admin.structure"), but could manually override it in case a route name didn't want to conform for some reason.

xjm’s picture

larowlan’s picture

pwolanin’s picture

@webchick - at this point the machine names are totally arbitrary, and I really feel queasy about the idea of parsing them for some semantic meaning.

The data here should be optimized for readability and maintainability, not pure speed while writing it using shortcuts only accessible to experts. In that spirit, I think requiring an explicit parent declaration is 100% desirable.

dawehner’s picture

StatusFileSize
new13.05 KB
new102.48 KB
FAILED: [[SimpleTest]]: [MySQL] 59,072 pass(es), 92 fail(s), and 41 exception(s).
[ View ]

I doubt that we can require people to have hal, serialize and rest enabled just to use menu links for real.

#876580: drupal_valid_path fails for dynamic paths (e.g. user/% cannot be added to menus) kind of contains the same fixes as needed here ...
This also fixes all kind of issues all over the place.

pwolanin’s picture

A few key questions I was posing to dawhener in IRC:

  • should default content be updated if the data in the hook changes? I think perhaps not, in analogy with config files.
  • Should we continue to support the "reset" feature? I'm tempted to rip it out and see if anyone misses it.
  • Should we try to delete links if the route they point to goes away? If we are treating these are pure content, maybe it would be better not to mess with "content"?
dawehner’s picture

Should we continue to support the "reset" feature? I'm tempted to rip it out and see if anyone misses it.

As long we provide all current "default links", contrib could provide that UI for us.

Should we try to delete links if the route they point to goes away? If we are treating these are pure content, maybe it would be better not to mess with "content"?

larowlan’s picture

I doubt that we can require people to have hal, serialize and rest enabled just to use menu links for real.

agreed

pwolanin’s picture

StatusFileSize
new109.48 KB
FAILED: [[SimpleTest]]: [MySQL] 56,998 pass(es), 491 fail(s), and 955 exception(s).
[ View ]
new11.55 KB

Some work around the edges for now.

Converts the menu link to include a #type => link render array so we can naturally handle either href or route name.

Removes all localizing for now since they needs to come back later as proper translatable fields.

Removes attempts to set the page title using the active trail - that should go.

pwolanin’s picture

StatusFileSize
new113.71 KB
FAILED: [[SimpleTest]]: [MySQL] 56,590 pass(es), 362 fail(s), and 926 exception(s).
[ View ]
new6.54 KB

Tries to fix admin page rendering and adds a build() method to the MenuLink class for consistency (though feels like most of the prepare function should go someplace there too).

not sure yet why the twig rendering is blowing up all over.

amateescu’s picture

Really nice cleanups here :)

  1. +++ b/core/includes/menu.inc
    @@ -1133,21 +1078,12 @@ function menu_tree_output($tree) {
    +    $element['link'] = $data['link']->build();

    +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -638,26 +614,29 @@ public function findParent(EntityStorageControllerInterface $storage_controller,
    +  /**
    +   * Builds and returns the renderable array for this menu link.
    +   *
    +   * @return array
    +   *   A renderable array representing the content of the link.
    +   */
    +  public function build() {

    This is the job of a entity ViewBuilder, and we have an issue to implement one for menu links: #2100467: Menu links are not ready to use a view builder so remove it from the annotation for now

  2. +++ b/core/includes/menu.inc
    @@ -2350,10 +2286,10 @@ function menu_set_active_trail($new_trail = NULL) {
    +       _menu_link_translate($preferred_link);

    @@ -2380,7 +2315,7 @@ function menu_set_active_trail($new_trail = NULL) {
    +            _menu_link_translate($link);

    Isn't this _menu_link_prepare() now?

  3. +++ b/core/includes/menu.inc
    @@ -2689,116 +2603,125 @@ function menu_get_router() {
    +  // @todo - remove links based on their machine names when module are
    +  // uninstalled, or mark them disabled if the route names are missing?

    I'd vote for disabled.

dcrocks’s picture

Where does _menu_link_prepare() exist?

amateescu’s picture

In the patch.

dcrocks’s picture

In 2 places 'prepared' is used instead of 'prepare'. Shouldn't the names match the function name?

   drupal_alter('prepared_menu_link', $item);
   function hook_prepared_menu_link_alter(\Drupal\menu_link\Entity\MenuLink &$menu_link, $map) {

This needs updating in user.module.

function user_translated_menu_link_alter(MenuLink &$menu_link) {

And shouldn't \Drupal::moduleHandler()->alter('prepare_menu_link', $item) be used instead of drupal_alter since the latter is deprecated?

dawehner’s picture

StatusFileSize
new2.17 KB
new114.99 KB
FAILED: [[SimpleTest]]: [MySQL] 56,909 pass(es), 172 fail(s), and 457 exception(s).
[ View ]

No offense, but I have the feeling that these kind of unrelated changes will SO much not help to get this patch green on the longrun.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new115.03 KB
FAILED: [[SimpleTest]]: [MySQL] 57,053 pass(es), 182 fail(s), and 474 exception(s).
[ View ]

I'm with @dawehner, there is a ton of stuff here that might be a good change, but seems out of scope. This patch is big enough already, can we remove some of this?
Especially the #below/below part, no idea what's happening there.

This is a straight reroll.

Status:Needs review» Needs work

The last submitted patch, 109: hook_default_menu_links-2047633-109.patch, failed testing.

DamienMcKenna’s picture

Issue summary:View changes
dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new10.09 KB
new111.78 KB
FAILED: [[SimpleTest]]: [MySQL] 59,411 pass(es), 150 fail(s), and 3,289 exception(s).
[ View ]

Just posting some work, which removes some of the changes to move the main point of the patch forward.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new3.6 KB
new111.78 KB
FAILED: [[SimpleTest]]: [MySQL] 59,400 pass(es), 150 fail(s), and 3,289 exception(s).
[ View ]

This fixes a couple of the failures.

catch’s picture

Priority:Major» Critical

If it blocks the beta, it by definition blocks the release.

I agree with webchick that the hook isn't ideal, but I also agree that making it anything else we'd want a 'default content entities' API which is out of scope at the moment.

However, potentially we could provide a 'default entities API' in a future 8.x release, use this to create default menu links, and have the hook left in as a bc layer.

Gábor Hojtsy’s picture

@catch: given #98 above saying Removes all localizing for now since they needs to come back later as proper translatable fields., that means we should consider #1842858: [PP-1] Convert menu links to the new Entity Field API (already critical) and #1966398: [PP-1] Refactor menu link properties to multilingual (only major yet) as a critical as well. Which may make #1498720: [meta] Make the entity storage system handle changes in the entity and field schema definitions a critical given the supposed performance problems of duplicating tables for multilingual. Would be good to roll this all the way to the issues required as consequences of decisions made here.

catch’s picture

I'd be OK with those three being critical.

amateescu’s picture

FWIW, I fully agree with #116. And there's some great cleanup here that will help #1842858: [PP-1] Convert menu links to the new Entity Field API a lot!

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new7.43 KB
new112.51 KB
FAILED: [[SimpleTest]]: [MySQL] 59,326 pass(es), 118 fail(s), and 352 exception(s).
[ View ]

More fixes.

pwolanin’s picture

Was struggling to find the problem in TrailTest.

Aside from a lot of ugly path-based code that should be converted to route names, I'm not seeing how this could ever work since menu_link_get_preferred() check access on the fetch link which is always going to be false for the 403 page.

possibly this test should just be deleted? It seems ot be testing something we don't really care about now?

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new118.6 KB
FAILED: [[SimpleTest]]: [MySQL] 59,068 pass(es), 115 fail(s), and 346 exception(s).
[ View ]
new6.09 KB

Ok, this change just deletes TralTest test - I didn't have time to compare to 7.x, but it did not seem meaningful now for 8.x

pwolanin’s picture

StatusFileSize
new119.01 KB
FAILED: [[SimpleTest]]: [MySQL] 59,118 pass(es), 114 fail(s), and 30 exception(s).
[ View ]
new562 bytes

Quick fix to avoid notices - possible coming from menu_link_get_preferred()

pwolanin’s picture

The book module fails should ideally be handled by #2100577: Phase 1 - Decouple book module from menu inc so it's not worth spending time now on BookTest

jhodgdon’s picture

I took a look at the documentation in this patch. Found a couple of minor things:

a)

+/**
+ * Builds menu links for the items returned from hook_default_meanu_links().
+ */
+function menu_default_links_rebuild() {

hook has typo in name in doc block "meanu_links" instead of "menu_links".

b) Within function drupal_valid_path():

+  /** @var $route_provider \Drupal\Core\Routing\RouteProviderInterface */
+  $route_provider = \Drupal::service('router.route_provider');

We don't actually want /** @tag */ comments within function bodies, at all.

c) Shouldn't hook_menu() be going away in this patch, and all the existing hook_menu() implementations be removed? Otherwise, I wouldn't be sure the tests are valid... Oh, I see, there is a two-phase approach. Maybe a patch that also removes hook_menu() would at least be useful for testing purposes? But... the hook_menu() docs page on api.drupal.org says that menu_router_build() invokes it, and also system_get_module_admin_tasks()... I don't think that first invocation has been removed from the patch?

d) In MenuLink.php:

+    // @FIXME Decide whether we want to keep the reset functionality.

Please use @todo, which is more standard in the Drupal project.

e) Is there documentation of the new hook somewhere? I'm not seeing it in the patch.

I didn't set this to "needs work" in case that would be annoying if this is really just an "in-progress" patch looking for feedback, but I think these issues need to be addressed... and I didn't read through the 128 comments here to see what's going on, just the summary. So sorry if I am duplicating other comments. Update the summary. :)

pwolanin’s picture

@jhodgdon - thanks for the review.

Regarding /** @tag */ comments within function bodies, check with dawehner on this, but I think he's been applying them in some places like there where it's otherwise impossible for the IDE to discover the variable type.

tim.plunkett’s picture

There are more than a dozen usages of this, it is included in the proposed PSR-5, and someone should open an issue about adding it to our doc standard. But not worth discussing further in this issue.

moshe weitzman’s picture

Issue tags:+Performance

Adding Performance tag as this gets rid of a very slow code path. See #2150833: Performance regression in menu access checks. Lets try to clear the blocker #2100577: Phase 1 - Decouple book module from menu inc and proceed to a speedy commit.

jhodgdon’s picture

Yes please open a new issue if you want to update the docs standards; until then, it's not compliant. Use tag "coding-standards" and component "documentation" and start title with [policy]. And then go read #1617948: [policy for now] New standard for documenting form/render elements and properties where people complained about adding /** */ comments within function bodies, and thereby blocked otherwise quite reasonable proposals.

dawehner’s picture

dawehner’s picture

StatusFileSize
new121.03 KB
FAILED: [[SimpleTest]]: [MySQL] 59,268 pass(es), 56 fail(s), and 15 exception(s).
[ View ]
new4.41 KB

Fixed the following failures:

  • Drupal\book\Tests\BookTest
  • Drupal\language\Tests\LanguageUILanguageNegotiationTest
  • Drupal\system\Tests\System\FrontPageTest (should be already fixed)
dawehner’s picture

.

pwolanin’s picture

StatusFileSize
new125.85 KB
FAILED: [[SimpleTest]]: [MySQL] 59,269 pass(es), 55 fail(s), and 15 exception(s).
[ View ]
new4.83 KB

Fix Drupal\system\Tests\Menu\TreeOutputTest

change 'href' to 'link_path'

pwolanin’s picture

StatusFileSize
new123.91 KB
FAILED: [[SimpleTest]]: [MySQL] 59,221 pass(es), 22 fail(s), and 57 exception(s).
[ View ]
new11.11 KB

This removes a couple obsolete test cases in MenuRouterTest, but I have been seeing a real fail in that the tools menu doesn't show up in the sidebar at all when it should (shows up in a current core test run), and links that should be in that menu aren't in the menu_links table when I check that during the test.

In fact - almost nothing is saved into the tools menu, apparently due to an invalid rout parameter for the "exotic" path.

This changes fixes that so the links save - but now I see a lot of Notices and exceptions due to
Undefined property: Drupal\menu_link\Entity\MenuLink::$menu_name

pwolanin’s picture

StatusFileSize
new127.63 KB
FAILED: [[SimpleTest]]: [MySQL] 59,227 pass(es), 14 fail(s), and 1 exception(s).
[ View ]
new11.56 KB

This fixes issues with the menu links in the tests.

tim.plunkett’s picture

Parent issue:» #1889790: [Meta] Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)
pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new130.23 KB
FAILED: [[SimpleTest]]: [MySQL] 59,153 pass(es), 8 fail(s), and 1 exception(s).
[ View ]
new5.08 KB

These changes should fix ~5 more test fails, including avoiding a fatal error and restoring the reset functionality.

pwolanin’s picture

StatusFileSize
new130.79 KB
FAILED: [[SimpleTest]]: [MySQL] 59,197 pass(es), 8 fail(s), and 1 exception(s).
[ View ]
new1.03 KB

Fix one more fail by fixing a missing permission (badly written test code is a bit non-deterministic)

jhodgdon’s picture

There are still no docs for this hook, which makes the patch really hard to evaluate. Writing hook docs is a great exercise, as it makes you think about whether the hook makes any sense. :)

dawehner’s picture

StatusFileSize
new131.75 KB
FAILED: [[SimpleTest]]: [MySQL] 59,339 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
new4.33 KB

This fixes the toolbar tests as well as the frontpage tests but and fixes the exception in the MenuRouterTest which causes 5 new failure in the actual test.

pwolanin’s picture

StatusFileSize
new134.5 KB
FAILED: [[SimpleTest]]: [MySQL] 59,393 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
new2.97 KB

This patch is maybe progress - getting a different exception now:

Route "aggregator.categories" does not exist.

also makes a rough start on api docs.

Gábor Hojtsy’s picture

Aggregator categories have recently been removed from core, so that sounds like a legitimate fail :)

pwolanin’s picture

StatusFileSize
new134.37 KB
PASSED: [[SimpleTest]]: [MySQL] 59,301 pass(es).
[ View ]
new524 bytes

ok, well that should be an easy fix then

amateescu’s picture

Awesome to see this green!

  1. +++ b/core/includes/menu.inc
    @@ -759,7 +759,9 @@ function _menu_translate(&$router_item, $map, $to_arg = FALSE) {
    +   // Avoid notices until we remove this code.

    Should we add a link to an issue here? (and the indentation is off a bit)

  2. +++ b/core/includes/menu.inc
    @@ -1133,19 +1085,14 @@ function menu_tree_output($tree) {
    +    // @todo Use route name and parameters to generate the link path, unless
    +    //    it is external.
    +    $element['#href'] = $data['link']['link_path'];

    Here or followup?

  3. +++ b/core/includes/menu.inc
    @@ -2487,6 +2423,8 @@ function menu_link_get_preferred($path = NULL, $selected_menu = NULL) {
    +    // @todo How do we fetch the title of tabs and other non menu links.

    Still relevant?

  4. +++ b/core/includes/menu.inc
    @@ -2689,105 +2614,141 @@ function menu_get_router() {
    +  // Remove process links names so we can find stragglers.

    Remove processed links ...

  5. +++ b/core/includes/menu.inc
    @@ -2689,105 +2614,141 @@ function menu_get_router() {
    +      // Note, we set this as 'system', so that we can be sure to distinguish all

    "we set the 'module' field as 'system'"

  6. +++ b/core/includes/menu.inc
    @@ -2689,105 +2614,141 @@ function menu_get_router() {
    +          $link['link_title'] = 'THIS IS A BUG';

    lol :) Throwing an exception is to invasive here? Maybe it deserves at least a watchdog entry.

  7. +++ b/core/modules/config_translation/lib/Drupal/config_translation/Plugin/Derivative/ConfigTranslationContextualLinks.php
    @@ -58,7 +58,7 @@ public function getDerivativeDefinitions(array $base_plugin_definition) {
    -      /** @var \Drupal\config_translation\ConfigMapperInterface $mapper */
    +      /** @type \Drupal\config_translation\ConfigMapperInterface $mapper */

    Unrelated change.

  8. +++ b/core/modules/contact/contact.module
    @@ -85,6 +85,26 @@ function contact_menu() {
    +    'type' => MENU_SUGGESTED_ITEM,

    Since we're moving/updating all menu link definitions, how about doing away with MENU_SUGGESTED_ITEM and just use 'hidden' straight up?

  9. +++ b/core/modules/entity/entity.module
    @@ -96,6 +96,36 @@ function entity_menu() {
    +  $links['admin.structure.display-modes'] = array(

    Any reason for using hyphens instead of underscores?

  10. +++ b/core/modules/menu/lib/Drupal/menu/Form/MenuDeleteForm.php
    @@ -106,7 +106,7 @@ public function submit(array $form, array &$form_state) {
         // @todo Convert this to an EFQ once we figure out 'ORDER BY m.number_parts'.
    -    $result = $this->connection->query("SELECT mlid FROM {menu_links} ml INNER JOIN {menu_router} m ON ml.router_path = m.path WHERE ml.menu_name = :menu AND ml.module = 'system' ORDER BY m.number_parts ASC", array(':menu' => $this->entity->id()), array('fetch' => \PDO::FETCH_ASSOC))->fetchCol();
    +    $result = $this->connection->query("SELECT mlid FROM {menu_links} ml INNER JOIN {menu_router} m ON ml.link_path = m.path WHERE ml.menu_name = :menu AND ml.module = 'system' ORDER BY m.number_parts ASC", array(':menu' => $this->entity->id()), array('fetch' => \PDO::FETCH_ASSOC))->fetchCol();

    Since the purpose of this patch is completely decouple {menu_links} from {menu_router}, can we remove this JOIN?

  11. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -368,13 +375,15 @@ public function setRouteObject(Route $route) {
    +    // @FIXME Decide whether we want to keep the reset functionality.

    Do we have a decision yet? :)

  12. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -384,27 +393,23 @@ public function reset() {
    +  protected function buildFromOriginalItem(array $item) {

    How about buildFromArray()? Or we can just move this code to a EntityInterface::preCreate() implementation.

  13. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -384,27 +393,23 @@ public function reset() {
    -      ->getStorageController('menu_link')->create($item);
    +       ->getStorageController('menu_link')->create($item);

    Unneeded change.

  14. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -597,7 +594,7 @@ public static function findRouteNameParameters($link_path) {
    +  protected function setParents(MenuLink $parent) {

    We type hint with interfaces, not classes.

  15. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -638,26 +635,29 @@ public function findParent(EntityStorageControllerInterface $storage_controller,
    +  /**
    +   * Builds and returns the renderable array for this menu link.
    +   *
    +   * @return array
    +   *   A renderable array representing the content of the link.
    +   */
    +  public function build() {

    This should say that it's temporary and it's going to be moved to a proper view builder in #2100467: Menu links are not ready to use a view builder so remove it from the annotation for now.

  16. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
    @@ -100,7 +100,7 @@ public static function createInstance(ContainerInterface $container, $entity_typ
    +    $query->leftJoin('menu_router', 'm', 'base.link_path = m.path');

    How come this is still necessary?

  17. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/LinksTest.php
    @@ -183,7 +183,7 @@ function testMenuLinkReparenting($module = 'menu_test') {
    +   * Test automatic reparenting of menu links derived from hook_default_men_links.

    Typo and exceeds 80 chars.

  18. +++ b/core/modules/system/system.admin.inc
    @@ -9,6 +9,7 @@
    +use Symfony\Cmf\Component\Routing\RouteObjectInterface;

    Doesn't seem to be used.

  19. +++ b/core/modules/system/system.install
    @@ -2293,6 +2293,23 @@ function system_update_8060() {
    + * Add machine_name column to the menu_links table.

    .. and remove the 'router_path' field.

  20. +++ b/core/modules/system/tests/modules/menu_test/lib/Drupal/menu_test/Controller/MenuTestController.php
    @@ -19,6 +19,11 @@ public function menuTestCallback() {
    +  public function titleCallback(array $_title_arguments = array(), $_title = '') {

    Missing doxygen.

  21. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TaxonomyController.php
    @@ -57,6 +57,19 @@ public function termTitle(TermInterface $taxonomy_term) {
    +   *   The term label.

    The vocabulary label.

pwolanin’s picture

Thanks for the review.

some feedback:

#2 I started changing, and it was out of scope per tim/daniel.
- I'll make a follow-up to convert everything to using at least a #type => link render element.

#6 I was wondering if it was going to show up in the UI - I was seeing a broken link after install.

#8 well, suggested is maybe the right semantics here, since it's suggested and can be enabled? Or we should rename the column to disabled? Either way, seems like a follow-up discussion.

#9 machine names were manually generated from existing paths - we can certainly go to underscore if we want.

#10 yes let's take the join out.

#11 It seems to work, so maybe we should opens a follow-up for discussion?

#15 - seems potentially overkill and a performance regression to go through another class to get the render array?

#16 looks like it should be removed

jhodgdon’s picture

Also it looks like the @return for menu_link_translate() got removed in the patch, inadvertently.

pwolanin’s picture

StatusFileSize
new136.07 KB
FAILED: [[SimpleTest]]: [MySQL] 57,449 pass(es), 182 fail(s), and 11 exception(s).
[ View ]
new14.74 KB

ok, let's see if these changes break anything

amateescu’s picture

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
@@ -380,10 +380,12 @@ public function reset() {
+    static::preCreate($storage_controller, $original);

preCreate() is already invoked by the storage controller :)

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageControllerInterface.php
@@ -34,20 +34,6 @@ public function setPreventReparenting($value = FALSE);
-  public function loadUpdatedCustomized(array $router_paths);

Nice!

amateescu’s picture

#2095283: Remove non-storage logic from the storage controllers has been committed, which conflicts a bit with the storage controller changes here.

dawehner’s picture

StatusFileSize
new135.99 KB
FAILED: [[SimpleTest]]: [MySQL] 57,438 pass(es), 183 fail(s), and 11 exception(s).
[ View ]

Just a simple rerole.

dawehner’s picture

StatusFileSize
new1.71 KB
new135.8 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links-2047633_11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

How come this is still necessary?

Other queries like loadModuleAdminTasks need the join to be there, so let's try to keep the patch size as small as possible and improve later.

amateescu’s picture

Now that we know who exactly still needs that join, I'm perfectly fine with keeping it in this patch :)

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new135.69 KB
FAILED: [[SimpleTest]]: [MySQL] 57,748 pass(es), 38 fail(s), and 10 exception(s).
[ View ]

Just a rerole to see the test failures.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new136.94 KB
FAILED: [[SimpleTest]]: [MySQL] 58,819 pass(es), 2 fail(s), and 3 exception(s).
[ View ]
new4.98 KB

Let's try to revert some specific changes to get it back to green again.

dawehner’s picture

StatusFileSize
new2.67 KB
new134.63 KB
FAILED: [[SimpleTest]]: [MySQL] 59,601 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Let's try to revert some specific changes to get it back to green again.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.87 KB
new135.49 KB
PASSED: [[SimpleTest]]: [MySQL] 59,546 pass(es).
[ View ]

That is it.

jhodgdon’s picture

With all of the patches flying on this issue, and comments saying "reverting to make tests pass" etc., it is hard to know whether the patches that are posted are really meant to be reviewed as potential candidates for committing. I'd like to review the API docs in the patch, for instance, but I don't want to waste time looking at them if the patch is still being worked on.

So when you think you have a real viable patch, can you post a comment that lets potential reviewers you think it's viable? Until then I'm assuming you're still working on it and using patch posting as a collaboration tool or a tool to see what still needs to be done (which is fine, but for those not working actively on the patch it is kind of pointless to look at each patch).

Thanks!

dawehner’s picture

You know, peter from time to time just goes to far with his patches, so this patch lets the patch stay in scope.

So this patch is ready to reviewed.

jhodgdon’s picture

Status:Needs review» Needs work

OK, thanks! I took a look at the patch (primarily at the API docs, as you might expect). I found a few things to fix:

a)

* @return
  *   Returns the map of path arguments with objects loaded as defined in the
  *   $item['load_functions'].
  *   $item['access'] becomes TRUE if the item is accessible, FALSE otherwise.
- *   $item['href'] is generated from link_path, possibly by to_arg functions.
- *   $item['title'] is generated from link_title, and may be localized.
- *   $item['options'] is unserialized; it is also changed within the call here
- *   to $item['localized_options'] by _menu_item_localize().
+ *   $item['href'] is generated from link_path
+ *   $item['access'] becomes TRUE if the item is accessible, FALSE otherwise.
+ *   $item['title'] is generated from link_title.
+ *   $item['route_parameters'] is unserialized if needed.
+ *   $item['options'] is unserialized and copied to $item['localized_options'].
  */
-function _menu_link_translate(&$item, $translate = FALSE) {
+function _menu_link_translate(&$item) {

This ends up with two $item['access'] and the $item['href'] should end in . -- actually this should be formatted as a list anyway, shouldn't it?

And actually, the return statement has been removed from the end of this function body. So the @return needs to be taken out entirely. It seems like the description of what happens to $item is useful, but there is not a return value for this function.

b)

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
@@ -25,18 +25,17 @@
class MenuLinkStorageController extends DatabaseStorageController implements MenuLinkStorageControllerInterface {

   /**
-   * Indicates whether the delete operation should re-parent children items.
-   *
-   * @var bool
+   * Contains all {menu_router} fields without weight.
+   * @var array
    */
-  protected $preventReparenting = FALSE;
+  protected static $routerItemFields;

Should be a blank line between description and @var. Also if this is an array of some specific type of object, you can do @var \Name\Of\The\Class[]

c) There are still some @todo items in the code, like MENU_SUGGESTED_ITEM. Do they need to be resolved now?

d)

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/BreadcrumbTest.php
...
+   * Test automatic reparenting of menu links derived from hook_default_men_links.
    */
   function testMenuLinkRouterReparenting() {

Test -> Tests and the hook name has a typo, should be _menu_ not _men_.

e) hook_default_menu_links() and the _alter() hook both need sample function bodies. Also the base hook and alter hook should have @see references to each other.

f) In hook_default_menu_links:

+ *   An array of default menu links. Each link has a key corresponding to a
+ *   unique machine name. The corresponding array value is an
+ *   associative array that may contain the following key-value pairs:
+ *   - "link_title": Required. The untranslated title of the menu item.
+ *     with path component substitution as described above.
...

- Second sentence should say "Each link has a key that is the machine name, which must be unique."
- Do not put "" or '' or anything else around the key names in the list. And "Required" should be (required). See https://drupal.org/node/1354#lists
- I think the second line of "link_title" should go away? There is nothing about path component substitution "above"?
- "route_name" says it is an untranslated title, but I think it's the machine name of the route really?
- "route_parameters" says the same thing, and it definitely isn't?
- "link_path" doesn't really say what it is.
OK I stopped looking. This doc section is not OK. Please fix. :)

-

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new6.71 KB
new136.26 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.api.php.
[ View ]

Should be a blank line between description and @var. Also if this is an array of some specific type of object, you can do @var \Name\Of\The\Class[]

Fixed. I don't think it is really helpful for scalar datatypes like string[] or int[] or even mixed[] ?

There are still some @todo items in the code, like MENU_SUGGESTED_ITEM. Do they need to be resolved now?

Yes, they need to be resolved at some point, though I really think we should take the iterative strategy and fix small in scope changes, given that adding 50kb to a patch defers the result potentially for at least 2 weeks.

e) hook_default_menu_links() and the _alter() hook both need sample function bodies. Also the base hook and alter hook should have @see references to each other.

Added one example.

OK I stopped looking. This doc section is not OK. Please fix. :)

I hope you will like this more.

Status:Needs review» Needs work

The last submitted patch, 184: menu_links-2047633.patch, failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new136.27 KB
FAILED: [[SimpleTest]]: [MySQL] 57,866 pass(es), 1,268 fail(s), and 80,883 exception(s).
[ View ]
new513 bytes

This just fixes the bogus error in system.api.php

The last submitted patch, 186: menu_links-2047633-186.patch, failed testing.

The last submitted patch, 186: menu_links-2047633-186.patch, failed testing.

dawehner’s picture

StatusFileSize
new136.27 KB
PASSED: [[SimpleTest]]: [MySQL] 59,929 pass(es).
[ View ]

This was just stupid, ... $item might be an object.

pwolanin’s picture

189: menu_links-2047633.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 189: menu_links-2047633.patch, failed testing.

pwolanin’s picture

I don't get that fail locally - sending to retest again.

pwolanin’s picture

Status:Needs work» Needs review

189: menu_links-2047633.patch queued for re-testing.

pwolanin’s picture

StatusFileSize
new138.05 KB
PASSED: [[SimpleTest]]: [MySQL] 60,058 pass(es).
[ View ]
new8 KB

Some small changes to clean up comments and avoid repeating the code to create an entity from the default link data. Moves the menthod to the storage controller, since it's really just an extension of create()

dawehner’s picture

StatusFileSize
new1.34 KB
new138.07 KB
PASSED: [[SimpleTest]]: [MySQL] 59,771 pass(es).
[ View ]

Just two simple things.

jhodgdon’s picture

This patch is looking better since the last time I looked at it!

A few notes:

a)

+ * @param array $item
+ *   The passed in and changed menu item with the following elements:
+ *   - href: The actual path to the link, is generated from link_path
+ *   - access: Becomes TRUE if the item is accessible, FALSE otherwise.
+ *   - title: The title of the link, is generated from link_title.
+ *   - route_name: The route name of the menu link.
+ *   - route_parameters: The unserialized route parameters.
+ *   - options: Is unserialized and copied to $item['localized_options'].
  */
-function _menu_link_translate(&$item, $translate = FALSE) {
+function _menu_link_translate(&$item) {

In the href item, what is "link_path"? I don't see it in the list. Maybe this should say: "The path to the link, which comes from the defined route." -- or ... not sure what it means. In any case it should end in ".". Same applies to the title item here. ... This is all a bit confusing to me. Some of the items in the list are inputs, I think, and some are added to the array. It seems like this should be made clearer. Maybe it should be two sections -- something like:
The following elements need to be present in $item:
(list, and indicate here if they are also updated in the function)
The following elements are added to or overridden in $item:
(list)

b)

-function menu_item_route_access(Route $route, $href, &$map) {
-  $request = RequestHelper::duplicate(\Drupal::request(), '/' . $href);
-  $request->attributes->set('_system_path', $href);
+function menu_item_route_access(Route $route, $href, &$map, Request $request = NULL) {

This function has a new argument $request but I don't see a new @param added to the documentation above it.

c)

+ * Recursive helper to save menu links.
  */
+function _menu_link_save_recursive($controller, $machine_name, &$children, &$links) {

Should probably say something like "Saves menu links recursively for whatever_uses_it()." to conform to our docs standards of starting with a verb and referring to the user of the function.

d)

+/**
+ * Find all links returned from hook_default_menu_links().
+ */
+function menu_get_default_links() {

Find -> Finds

e)

+  /**
+   * {@inheritdoc}
+   *
+   * This function should only be called for link data from
+   * hook_default_menu_links().
+   */
+  public function createFromDefaultLink(array $item) {

@inheritdoc does not work in the API module if you put other stuff in the documentation. Our docs standard currently is that you either inherit the doc or you don't. You can't both inherit it and add stuff to it.

f) In the docs for hook_default_menu_links():

+ *   - link_title: (required) The untranslated title of the menu item.
+ *     with path component substitution as described above.

This is not described above. There is nothing above this.

g) Same docs block:

+ *   - route_parameters: (required) The route parameters to build the path.

I do not think this is required. Most of the implementations and the sample function body do not include it.

h) same block:

+ *   - link_path: (optional) If you have an external link use link_path instead
+ *     of providing a route_name.

Well above it says route_name is required. Here it says it isn't.

i) same block:

*   - parent": (optional) The machine name of a link to define hierarchy.

Stray " -- also this isn't clear. What hierarchy is defined? How about just saying "The machine name of the link that is this link's menu parent" or something?

j) same block:

+ *   - weight: An integer that determines the relative position of items in

Should probably say (optional)

k) same block:

+ *   - type: (optional) A bitmask of flags describing properties of the menu item.
+ *     Many shortcut bitmasks are provided as constants in menu.inc:

It seems like only the two flags of normal/suggested apply actually now?

l) same block:

+ *   - options: An array of options to be passed to l() when generating a link
+ *     from this menu item.

This should say (optional)

m)

+ *   - menu_name: (optional) Set this to a custom menu if you don't want your
+ *     item to be placed in the default Tools menu.

How about saying "The machine name of a menu to put the link in, if not the default Tools menu."

n) How come the menu links table schema is in menu_link.install and the update function for it is in system.install? That seems wrong. If menu_link is not enabled, won't that system.install update function fail?

dawehner’s picture

StatusFileSize
new137.72 KB
PASSED: [[SimpleTest]]: [MySQL] 59,344 pass(es).
[ View ]

This is not described above. There is nothing above this.

We indeed don't do that fancyness anymore.

Well above it says route_name is required. Here it says it isn't.

You are absolute right!

Stray " -- also this isn't clear. What hierarchy is defined? How about just saying "The machine name of the link that is this link's menu parent" or something?

This sounds nice!

It seems like only the two flags of normal/suggested apply actually now?

You are right, I changed the text a bit.

n) How come the menu links table schema is in menu_link.install and the update function for it is in system.install? That seems wrong. If menu_link is not enabled, won't that system.install update function fail?

Well, menu links are part of system module in d7, so if you have system in d7 you need to update the table of menu links. menu links in d7
are bound to the system schema, so that is kind of the place to update it. Additional the other update functions for menu links happen on there.

jhodgdon’s picture

interdiff?

dawehner’s picture

StatusFileSize
new4.75 KB

Here is one.

jhodgdon’s picture

Thanks for the interdiff, although it wasn't complete? Anyway, a few comments:

a)

+ *   - route_parameters: The unserialized route parameters of the men link.

typo: men -> menu

b) I think my comment in #196-a still stands, in regards to the menu_link_translate() param docs:

This is all a bit confusing to me. Some of the items in the list are inputs, I think, and some are added to the array. It seems like this should be made clearer.

c) #196 (e) still applies.

d) In the hook docs:

+ *   - route_name: (required) The route name to be used to build the path.
+ *   - route_parameters: (optional) The route parameters to build the path.
+ *   - link_path: (optional) If you have an external link use link_path instead
+ *     of providing a route_name.

The route_name (required) and docs for link_path still contradict each other. Either route_name or link_path is required, but saying route_name is required is misleading.

e) in hook docs:

+ *   - parent": (optional) The machine name of the link that is this link's menu

Remove the "

f) #196 (l) still applies.

g) I still do not understand the justification for having the update function and the schema in different modules. If the Menu Links module is not installed, its schema/database tables will not exist, and the update function will not work. Don't they need to be together? And if someone updates from 7 to 8 and does not have the menu links module enabled at the time... what happens then if they install the module and it tries to create the tables and they already exist? The whole thing I think needs a bit more thought.

dawehner’s picture

StatusFileSize
new137.77 KB
PASSED: [[SimpleTest]]: [MySQL] 59,297 pass(es).
[ View ]

a)

Fixed

b) I think my comment in #196-a still stands, in regards to the menu_link_translate() param docs:

This is all a bit confusing to me. Some of the items in the list are inputs, I think, and some are added to the array. It seems like this should be made clearer.

Splitted up the input and changed elements.

c)

Fixed

The route_name (required) and docs for link_path still contradict each other. Either route_name or link_path is required, but saying route_name is required is misleading.

Right.

Remove the "

Good catch.

This should say (optional)

Fixed.

g) I still do not understand the justification for having the update function and the schema in different modules. If the Menu Links module is not installed, its schema/database tables will not exist, and the update function will not work. Don't they need to be together? And if someone updates from 7 to 8 and does not have the menu links module enabled at the time... what happens then if they install the module and it tries to create the tables and they already exist? The whole thing I think needs a bit more thought.

If you update from d7 to d8 you don't have the menu module installed, just the system module, which provides these tables back in d7. Afaik these updates will be migrations on the longrun.

I am sorry but I failed again to provide a proper interdiff.

pwolanin’s picture

StatusFileSize
new137.77 KB
PASSED: [[SimpleTest]]: [MySQL] 59,769 pass(es).
[ View ]

re-roll for conflict in search and views modules.

amateescu’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new137.76 KB
PASSED: [[SimpleTest]]: [MySQL] 59,757 pass(es).
[ View ]
new5.99 KB

Reviewed this patch again and I understand that some parts of the review from #159 need to be handled in followups in order to keep the patch size manageable. Good thing we already have followups for most of them.

I hope @jhodgdon is happy with the latest changes and I only found a few small nitpicks, so here's a small update and a much deserved RTBC :)

chx’s picture

jhodgdon’s picture

Status:Reviewed & tested by the community» Needs work

The hook documentation now does not tell you that you need to provide either route_name or link_path. What it says in the latest patch is:

+ *   - route_name: (optional) The route name to be used to build the path.
+ *   - route_parameters: (optional) The route parameters to build the path.
+ *   - link_path: (optional) If you have an external link use link_path instead
+ *     of providing a route_name.

I think you need to have either route_name or link_path, right? The docs do not say this.

The latest patch by amateescu also introduced a new issue too. From the interdiff:

+++ b/core/modules/views/lib/Drupal/views/ViewExecutable.php
@@ -1533,19 +1533,20 @@ public function executeHookMenu($display_id = NULL, &$callbacks = array()) {
   }

   /**
-   * Called to get hook_default_menu_links() information from the view and the named display handler.
+   * Returns default menu links information from the view and the named display
+   * handler.

First line descriptions of functions must be one line of less than 80 characters.

As #201 didn't provide an interdiff, I have no idea if there were other issues introduced in that patch...

pwolanin’s picture

StatusFileSize
new138.82 KB
FAILED: [[SimpleTest]]: [MySQL] 60,022 pass(es), 0 fail(s), and 1,750 exception(s).
[ View ]
new36.82 KB

This addresses jhogdon's comments, and fixes a place in system still using link_path

also tries to normalize machine names to start with the module name, though I suspect I didn't get 100%. I think we should do this now vs later when it would be more disruptive. It also aligns these machine names more with other core patterns.

pwolanin’s picture

Status:Needs work» Needs review

doh. setting to NR so it's tested.

The last submitted patch, 206: menu_links-2047633-205.patch, failed testing.

The last submitted patch, 206: menu_links-2047633-205.patch, failed testing.

The last submitted patch, 206: menu_links-2047633-205.patch, failed testing.

dawehner’s picture

StatusFileSize
new138.83 KB
PASSED: [[SimpleTest]]: [MySQL] 59,731 pass(es).
[ View ]
new1.78 KB

I hope this should most of the failures ...

pwolanin’s picture

Issue summary:View changes
pwolanin’s picture

jhodgdon’s picture

The patch in #206 addressed my remaining concerns, and the patch in #211 didn't introduce any more, so I think from a docs perspective this patch is in good shape.

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community

ok, back to RTBC then based on jhogdon being happy and the machine names being fixed.

catch’s picture

Few questions came up, leaving RTBC since none of these are blockers to commit.

  1. +++ b/core/includes/menu.inc
    @@ -2649,105 +2580,137 @@ function menu_get_router() {
    +          // A change in hook_default_menu_links may move the link to a

    hook_default_menu_links()

  2. +++ b/core/includes/path.inc
    @@ -206,16 +222,22 @@ function drupal_valid_path($path, $dynamic_allowed = FALSE) {
    -    $route = \Drupal::service('router.route_provider')->getRouteByName($item['route_name']);
    -    $item['access'] = menu_item_route_access($route, $path, $map);
    +    $route = \Drupal::service('router.route_provider')->getRouteByName($route_name);
    +    $request = RequestHelper::duplicate(\Drupal::request(), '/' . $path);
    +    $request->attributes->set('_system_path', $path);
    +    $request->attributes->set('_menu_admin', TRUE);
    +

    This looks like something we missed before that should already have been in there?

  3. +++ b/core/modules/aggregator/aggregator.module
    @@ -128,6 +128,30 @@ function aggregator_menu() {
    /**
    + * Implements hook_default_menu_links().
    + */
    +function aggregator_default_menu_links() {
    +  $links = array();
    +  $links['aggregator.admin_overview'] = array(
    +    'link_title' => 'Feed aggregator',
    +    'description' => "Configure which content your site aggregates from other sites, how often it polls them, and how they're categorized.",
    +    'route_name' => 'aggregator.admin_overview',
    +    'weight' => 10,
    +  );
    +  $links['aggregator'] = array(
    +    'link_title' => 'Feed aggregator',
    +    'weight' => 5,
    +    'route_name' => 'aggregator.page_last',
    +  );
    +  $links['aggregator.sources'] = array(
    +    'link_title' => 'Sources',
    +    'route_name' => 'aggregator.sources',
    +  );
    +
    +  return $links;
    +}

    While the 'default menu links' hook is a bit of a one-off, this actually looks completely fine to me until we come up with something properly generic. It's easy to port over from hook_menu(), and what's left in here is right. 'parent' isn't in this hunk, but usage of that elsewhere is good and bette than current guessing. I don't understand what's going on with MENU_SUGGESTED_ITEM- fine in a follow-up but that should probably be critical too.

  4. +++ b/core/modules/book/book.module
    @@ -181,6 +181,26 @@ function book_menu() {
    +    // @TODO what to do about MENU_SUGGESTED_ITEM, maybe specify no menu_name?

    Is there an issue open for this todo?

  5. +++ b/core/modules/book/lib/Drupal/book/BookManager.php
    @@ -131,7 +131,7 @@ public function getLinkDefaults($nid) {
    +      'link_path' => 'node/%',

    Bit weird that we still use this and can't use the actual route, but more follow-up I guess.

effulgentsia’s picture

Assigned:Unassigned» effulgentsia
Status:Reviewed & tested by the community» Needs review

I brought this issue up on a call with Dries and webchick, and they had some feedback on the API of hook_default_menu_links(). I'll post that here within 24 hours.

dawehner’s picture

StatusFileSize
new1.67 KB
new138.81 KB
PASSED: [[SimpleTest]]: [MySQL] 59,745 pass(es).
[ View ]

This looks like something we missed before that should already have been in there?

Once we manage to get rid of _menu_translate() we should also try to replace menu_item_route_access() with the checkNamedRoute on the access manager.

I don't understand what's going on with MENU_SUGGESTED_ITEM- fine in a follow-up but that should probably be critical too.

A menu suggested item is the same as before. It is a menu item which is hidden by default. Maybe we should add some constant on the MenuLinkInterface to make it more clear?
Here is one: [#2176123

Bit weird that we still use this and can't use the actual route, but more follow-up I guess.

We indeed have one for that already: #2084421: Phase 2 - Decouple book module schema from menu links

I brought this issue up on a call with Dries and webchick, and they had some feedback on the API of hook_default_menu_links(). I'll post that here within 24 hours.

Hopefully this will be just a review, so I can post a patch in the meantime.

effulgentsia’s picture

Ok, here's my notes from the discussion with Dries and webchick. Note: we did not go over any of the implementation details of the patch, and instead only had time to discuss the API change of hook_menu() to hook_default_menu_links() before Dries had to attend to something else.

  1. Are we sure we want the name to be hook_default_menu_links() rather than hook_menu_links()? Do module developers need/want to be concerned with the distinction that what they're putting into that hook can be overridden / added-to via the UI? Or is there some other meaning we're trying to convery with the "default" prefix?
  2. In other issues, we've been moving some declarative things from being arrays to being objects (e.g., FieldDefinition and EntityType). Dries thinks this is a place that could benefit from that as well. So, for example, instead of
    $links['user'] = array(
        'link_title' => 'My account',
        'weight' => -10,
        'route_name' => 'user.page',
        'menu_name' => 'account',
      );

    Maybe something like:

    // Just an example. Actual implementation might end up being something
    // quite different.
    $links[] = MenuLink::create('user', 'user.page')
    ->setTitle('My account')
    ->setWeight(10)
    ->setMenu('account')

    Thoughts? Normally, I'd suggest such a thing be pursued in a follow up rather than hold up this patch, but webchick wasn't keen on that, since it would mean an API change affecting every module developer now, and then another one later, and given where we are in the cycle, that would be rude.

  3. Why do the majority of the machine names of the links not match the machine names of the corresponding routes? While there shouldn't be anything requiring them to match, especially since you may want multiple links pointing to the same route, wouldn't it be better DX for them to match in cases where there isn't a compelling reason otherwise?
  4. Related to above, could/should we make 'route_name' optional, and have it default to the machine name of the link?
  5. Why are we not removing hook_menu() implementations in this patch? I understand not having done that up until this point, to keep rerolls / patch reviews easier, but shouldn't we add that to the patch prior to commit? Otherwise, we run the risk of committing this prior to an alpha release, and the hook_menu() removal after an alpha release, which would confuse module developers chasing alphas.

@webchick: please add to / correct the above if I missed something or understood something incorrectly.

jhodgdon’s picture

+1000 on #219 item 5, which I brought up ages ago. I don't think we should have both hooks there after this patch. It will just lead to confusion. If hook_menu() for instance is still on api.drupal.org, people will try to use it because they know about it. If it's completely gone, they'll be like "Oh, this is gone, maybe I should go search for a change notice". Right?

+1 on the rest of #29 too -- seems like a good idea.

pwolanin’s picture

I think "default" is very important to have in the name - it's defining default content (links), not all the content (links).

pwolanin’s picture

re: point 5 - I strongly disagree - we are at comment #222 and I expect removing hook_menu is going to be a bit of a can of worms yet. We need to get this in so we have a clean point to start that patch- I'm willing to bet it will be a 100-200 comment thread on its own by the time we find all the edge cases.

re: point 4 - no, I think it's worse DX to sometimes use the machine name for the route. I opposed this for local tasks, and I won't sign off on it here.

re: point 3 - I was trying to make sure all had the module name as the prefix. Further cleanup/normalization is certainly possible. I'm really not sure that making them the same as the route name is actually better DX. Why would you think so?

re: point 2: no - these are default values for content, not actual entities. What would be the gain?

pwolanin’s picture

further re: suggestion 5 being a further can of worms, Views module is still using hook_menu via views_menu_alter(), and there there is code still mixed into local tasks and local actions, and still various calls like menu_get_item() that have to be dealt with.

pwolanin’s picture

pwolanin’s picture

Issue summary:View changes
effulgentsia’s picture

re: point [#219.]2: no - these are default values for content, not actual entities. What would be the gain?

The gain would be in getting rid of arrays with magic keys from Drupal's most commonly implemented hook according to https://gist.github.com/webchickenator/4409685. The benefits of objects with getter/setter methods include IDE friendliness (e.g., autocompletion), and consistency with D8's general trend of converting lots of code to OOP.

However, here's the current state of HEAD with respect to the next 10 most commonly implemented declarative hooks from that list. Note, I'm not including non-declarative hooks (like hook_uninstall()) or hooks with no D8 equivalent (like hook_views_api()).

hook_permission: PHP array
hook_theme: PHP array
hook_schema: PHP array
hook_block_info: Annotation
hook_requirements: PHP array
hook_views_default_views: YAML
hook_field_formatter_info: Annotation
hook_views_data: PHP array
hook_node_info: YAML
hook_field_info: Annotation

None of the above 10 are currently declared as PHP objects. So despite having implemented object-based declaration syntax for ContentEntityInterface::baseFieldDefinitions() and hook_entity_info(), those two use cases don't affect as many modules, so perhaps it's better DX (at least from a consistency standpoint) to leave hook_default_menu_links() as PHP array based?

On the other hand, I'd like to resurface the proposal of making it YAML. This was suggested and +1'd in several of the early comments in this issue, including webchick in #80, and from what I can tell was only rejected by catch in #12 due to wanting to introduce the smallest possible change from status quo. However, I think there's a strong benefit to making it consistent with how we declare routes (YAML + 2 events, where the events deal with objects, not arrays, and therefore might satisfy Dries's request). Also, I looked at the latest patch in this issue, and there's not a single use case at all in core of requiring the DYNAMIC event (we should still have one for contrib's benefit), and only 3 modules that would have small implementations of the ALTER event, so updating the patch to this approach if we agree to do it should be fairly easy.

@catch: what do you think?

effulgentsia’s picture

Assigned:effulgentsia» catch

Assigning to catch for feedback on #227.

effulgentsia’s picture

further re: suggestion 5 being a further can of worms, Views module is still using hook_menu via views_menu_alter(), and there there is code still mixed into local tasks and local actions, and still various calls like menu_get_item() that have to be dealt with.

Discussed this with pwolanin, and given this, my suggestion is to commit this issue's patch as early after the next alpha is released as possible (while still addressing #219 to Dries's and webchick's satisfaction), and then have a patch in #2177041: Remove all implementations of hook_menu that gets as much of the way there as can be done quickly (e.g., within a few days), and then try to get whatever's left that's tricky done before the following alpha (one month). I think that's the best way to minimize disruption for module developers while still allowing core developers working on this to operate reasonably efficiently.

effulgentsia’s picture

I was trying to make sure all had the module name as the prefix. Further cleanup/normalization is certainly possible.

I think when you started this issue, that was a real problem, because route machine names didn't all have their module prefix. Now they all do, so no longer an argument against making link machine names and route machine names match.

I'm really not sure that making them the same as the route name is actually better DX. Why would you think so?

Because to a large extent, there's a 1:1 correspondence between link and route: while it's theoretically possible to have 2 menu links to the same route, there's no example of that in core, and it's probably very rare in contrib. So why make people deal with keeping track of 2 different names for the 99+% of links that don't need that? Also, when as a module author you want to alter another module's menu link, a common workflow is that your initial piece of knowledge is the URL of that link (because it was in testing the site that you found a menu link you want to be different). So now your task is to figure out its machine name. Assuming you know the module responsible for the URL, you can open up its routing.yml file and look for a URL pattern there that matches what you're looking for. Great, now you know the route name. It would be nice if 99% of the time, you could stop there, rather than always needing to follow that up with scanning the module's hook_default_menu_links() implementation to backtrack the link name from the route name.

I think it's worse DX to sometimes use the machine name for the route. I opposed this for local tasks, and I won't sign off on it here.

Really? Even if "sometimes" = 100% of core use cases and 99% of contrib use cases?

catch’s picture

@effulgentsia, not sure if it is a good objection, but it feels weird having default entities defined in YAML. I agree it makes this 'consistent' with local tasks and etc., but menu links aren't actually consistent with those at all due to being content entities. Are you sure there won't be any dynamic menu links defined? I assume Views will have to for one.

Agreed on leaving hook_menu() removal to a separate issue, and I don't think the alpha should particularly hold this issue up. Alphas are completely arbitrary, so holding up progress on critical issues due to an arbitrary marker feels counterproductive - especially given the amount of work that's left to do here.

dawehner’s picture

We had a similar discussion (custom objects vs. pure arrays) on the route callback issue: #2145041: Allow dynamic routes to be defined via a callback
There we use route objects, but you can return a route collection/array.

On the other hand this does not fit perfectly, given that route callbacks are dynamic, while hook_menu entries are mostly static, as effulgentsia said.
If we use objects we might also have to take care about the memory overhead of keeping all of them in memory. For routes we have this partial inserting strategy, while (my guess) for pure arrays this would not matter that much.

Because to a large extent, there's a 1:1 correspondence between link and route: while it's theoretically possible to have 2 menu links to the same route, there's no example of that in core, and it's probably very rare in contrib.

We should take into account that optimizing locally (just for menu links) makes it potentially harder to understand the big picture (route names everywhere, like on local tasks/actions etc.). Explicitness is an important point, magic is what makes things hard to debug.

Really? Even if "sometimes" = 100% of core use cases and 99% of contrib use cases?

In contrasts to the other siblings (tasks/actions/routes...) I would certainly agree that default menu links are more or less in a 1to1 relationship with routes. Even in views this is the case. The UI does not allow you to create multiple menu links for a single route (view + display ID combination), but I still vote for explicitness. If you look into the patch there are many places where we don't use that 1to1 relationship. Here is just one example:

+  $links['book'] = array(
+    'link_title' => 'Books',
+    'route_name' => 'book.render',
+  );
+

Also, I looked at the latest patch in this issue, and there's not a single use case at all in core of requiring the DYNAMIC event (we should still have one for contrib's benefit), and only 3 modules that would have small implementations of the ALTER event, so updating the patch to this approach if we agree to do it should be fairly easy.

Again views ... actually using the alter hook is kind of wrong at the moment, because the code does not care about existing menu links yet.

Alphas are completely arbitrary, so holding up progress on critical issues due to an arbitrary marker feels counterproductive - especially given the amount of work that's left to do here.

Xano’s picture

Are we sure we want the name to be hook_default_menu_links() rather than hook_menu_links()? Do module developers need/want to be concerned with the distinction that what they're putting into that hook can be overridden / added-to via the UI? Or is there some other meaning we're trying to convery with the "default" prefix?

This question from #219 seems to have been overlooked. Why is the hook in the default namespace and not in menu?

pwolanin’s picture

@Xano - that is a valid criticism, but I think we used this since we are defining menu_links.

I'd be ok flipping it to be hook_menu_link_defaults() or something like that.

pwolanin’s picture

StatusFileSize
new30.59 KB
new138.78 KB
PASSED: [[SimpleTest]]: [MySQL] 59,758 pass(es).
[ View ]

pinged tim.plunkett. We couldn't think of a better name than hook_menu_link_defaults.

This changes the name of the hook and related functions.

sun’s picture

Thanks all for your hard work on this issue!

I spared some time to review the proposed changes in some more depth.

While I believe we could move forward here mostly as-is for the sake of making progress, I did run into some major concerns that make me less confident and a bit skeptical of whether moving forward here will actually mean to have moved forward in the right direction.

I'm aware that a few of the concerns I'm raising are questioning some fundamental aspects that you seemingly decided on already. But since this change proposal appears to revamp and "finalize" the ultimate architecture and resulting DX, and because this patch appears to introduce the final code (for API consumers), these preconditions forced me to raise the concerns that I'm raising below.

My primary take-away from the currently proposed API is the impression that too many internal architectural details are leaking into user-space code of extensions. The public API design leaves me with the impression as if it were designed for arbitrary PHP components/libraries to allow for tons of flexibility, as opposed to its primary consumer, simple Drupal modules.

While I can understand the desire for architectural perfectionism, and while the proposed high-level public API is seemingly simple ("Just implement this hook and return your links array!"), the actual low-level complexity of defining the required information is extremely concerning. Just getting some links in place and at the right position can't be that hard, can it?

With the currently proposed solution, I can already see myself hopping through a dozen of menu link hooks and routing.yml files, because the system forces me to manually resolve all of the layers of indirection that it (internally) established, and because it decided to (transparently) expose that to user-space extension code, without any kind of shield to ensure a sane core product experience.

I'm not convinced that it has to be that way. It is great and very awesome to see that we're finally in a position to make a clear difference between the internal concepts of routes and menu links in our subsystems/modules. But at the same time, especially after studying this patch, I do have the impression that we currently attempt to move from 0% to 200% → missing to hit the mark at 100%.

Architectural internals aside, I think we should primarily focus on core product experience. → How hard can it be to place my module's menu link next to or below /admin/structure/types? — Are you able to give advice to a newcomer without looking at the code/definitions?

Please forgive me, I don't know how to end this intro in a positive way that reads encouraging. Here's my actual review:

  1. +++ b/core/includes/menu.inc
    @@ -854,95 +857,51 @@ function menu_tail_load($arg, &$map, $index) {
    + *   - link_title: (required) The title of the menu link.
    ...
    + *   The passed in item is changed by the following keys:
    ...
    + *   - title: The title of the link. This title is generated from the
    + *     link_title of the menu link entity.

    This poor naming of property keys always annoyed me and I hoped that this patch would finally get rid of it, as it seemingly changes the definition format.

    It appears to me that the value of 'link_title' is simply copied into 'title' now. → Can we finally get rid of 'link_title'?

    Would be great to do that here, or to have a follow-up issue for doing so.

  2. +++ b/core/includes/menu.inc
    @@ -2643,105 +2574,137 @@ function menu_get_router() {
    +function _menu_link_save_recursive($controller, $machine_name, &$children, &$links) {
    +  $menu_link = $links[$machine_name];
    ...
    +    $controller->save($menu_link);
    ...
    +  if (!empty($children[$machine_name])) {
    +    foreach ($children[$machine_name] as $next_name) {

    1. The chosen construct that both $links and $children contain a set and subset of link entities looks a bit weird to me. (A) Why is $links not sufficient? (B) Or why can't we move $children into $link->children?

    2. $controller->save($link) should be $link->save()?

  3. +++ b/core/includes/menu.inc
    @@ -2643,105 +2574,137 @@ function menu_get_router() {
    +function menu_get_default_links() {
    +  $module_handler = \Drupal::moduleHandler();
    +  $all_links = $module_handler->invokeAll('default_menu_links');
    ...
    +  $module_handler->alter('default_menu_links', $all_links);

    I agree that the discussion on using YAML files and YamlDiscovery here should be moved to a follow-up issue.

    However, I also agree that the hook name should definitely be prefixed with "menu_" → either hook_menu_links() or hook_menu_links_default().

    I did not understand why "default" is part of the hook name. The returned links appear to be processed and saved, as long as they have not been customized (existing behavior). Only a preexisting link can be customized, so if at all, the original definitions would be hook_menu_links_unless_customized(). — But IMHO, anything like that is an irrelevant detail for the hook implementor.

    Instead, a much more sensible name that complies with our hook naming standards would be hook_menu_link_info(). → It's declarative, unless altered/customized. — As usual. No need to invent something new?

    In any case, the alter hook needs to be adjusted, too.

  4. +++ b/core/includes/menu.inc
    @@ -2643,105 +2574,137 @@ function menu_get_router() {
    +function menu_default_links_rebuild() {
    +  $module_handler = \Drupal::moduleHandler();
    +  if (!$module_handler->moduleExists('menu_link')) {
         // The Menu link module may not be available during install, so rebuild
         // when possible.
         return;
       }

    It looks and feels a bit icky that a central condition is buried so deeply into call chains.

    Due to that, for example, the calling code disregards that fact and proceeds to clear all kinds of caches, whereas that only makes sense if there actually is any menu link related data in the affected caches.

  5. +++ b/core/includes/menu.inc
    @@ -2643,105 +2574,137 @@ function menu_get_router() {
    +          $link['link_title'] = 'THIS IS A BUG';

    Is this obsolete debugging code? If not, can we throw an exception instead?

  6. +++ b/core/includes/menu.inc
    @@ -2643,105 +2574,137 @@ function menu_get_router() {
       // Find any item whose router path does not exist any more.
       $query = \Drupal::entityQuery('menu_link')
    -    ->condition('router_path', $paths, 'NOT IN')
    +    ->condition('machine_name', array_keys($all_links), 'NOT IN')

    $all_links can be empty, which will cause this query to blow up.

  7. +++ b/core/modules/action/action.module
    @@ -57,6 +57,20 @@ function action_menu() {
    +  $links['action.admin.actions'] = array(
    +    'link_title' => 'Actions',
    +    'description' => 'Manage the actions defined for your site.',
    +    'route_name' => 'action.admin',
    +    'parent' => 'system.admin.config.system',
    +  );

    Now, on the real gist of this patch:

    1. I think we are pushing an extraordinary amount of layers of indirection onto D8 developers. Two property values here are referencing IDs that are outside of the visible scope of the author.

    2. The menu link machine names can be arbitrary, and the route names can be arbitrary. At minimum, the retrieval/discovery code should validate that each defined machine name actually starts with "$module.". — In addition, we should have a critical beta blocker to rename all route names in core to match the link machine names (which should be covered by a test on top).

    3. At the same time, I wonder (1) whether we really need to prefix all routes and menu link names manually and (2) what the application-wide situation of such prefixing is. In particular, I wonder whether we shouldn't adopt the namespacing approach of the asset library system some more; i.e.:

    No prefixes within your own definitions. To reference a definition of something else, use a "provider/" or "provider:" prefix.

    In the concrete example here:

    <?php
    $links
    ['admin.config.system.actions'] = array(
     
    'parent' => 'system/admin.config.system',
     
    'route' => 'action/admin',
    );
    ?>

    Alternatively, why do we have module prefixes in the first place, if all of these links can be customized (and thus turned into custom links) anyway later on?

    4. Now that I've actually "touched" this code, the original machine name in this example is deeply concerning → it results in the following hierarchy:

    system.admin.config.system
    ∟ action.admin.actions

    Now let's consider 3D instead of 2D:

    system.admin.config.system
    ∟ action.admin.actions
      ∟ mymodule.actions.admin.settings
      ∟ othermodule.admin.structure.actions.control

    → Menu link machine names will be completely arbitrary and will make absolutely no sense.

    Care to remind me, why can't we use the route name as key + parent again?

    Aside from perhaps a few edge-case modules, I've the impression that diverging from our existing 1:1 route to link mapping only causes problems with no gain for the >95% of API consumers?

    Perhaps the use-cases in my entire Drupal history were not advanced enough, but I never actually felt limited by that 1:1 mapping. I think we should explore whether we cannot move the "more than one link for a single route" edge-case/use-case to a different construct; e.g., a separate hook or routing event subscriber.

    Lastly, as mentioned above, the 'link_title' key is an alien here. It should be 'title' or 'label' (→ Entity::label()).

    Likewise, it would be great to rename 'route_name' into just 'route'.

  8. +++ b/core/modules/entity/lib/Drupal/entity/Controller/EntityDisplayModeController.php
    @@ -54,7 +54,7 @@ public function viewModeTypeSelection() {
    -          'href' => 'admin/structure/display-modes/view/add/' . $entity_type,
    +          'link_path' => 'admin/structure/display-modes/view/add/' . $entity_type,

    I do not really see how 'link_path' is an improvement over 'href'...?

    Can't we introduce a temporary BC shim to (1) vastly reduce the size of this patch and (2) leave the naming decision and rename to a follow-up issue?

pwolanin’s picture

@sun - I think you cross-posted with my last patch, so #3 is addressed

I don't understand your feedback in a large fraction of your comment, esp. #7 and 8.

re: #4 and in my last bit of patching, I think we should consider having a follow-up to move all the relevant code to the menu_link module and get it out of menu.inc to resolve the need to check if a module exists.

re: #1, etc, the DB column is link_title and that's internal to the menu_link entity. I think it's out of scope for this patch to fix that entity's properties.

In my mind, the goal here is really only to get menu link data split off and out of the way so we can kill hook_menu itself. I have only focused on that very pragmatic goal, which has been hard enough to get right.

dawehner’s picture

Now, on the real gist of this patch:

1. I think we are pushing an extraordinary amount of layers of indirection onto D8 developers. Two property values here are referencing IDs that are outside of the visible scope of the author.

2. The menu link machine names can be arbitrary, and the route names can be arbitrary. At minimum, the retrieval/discovery code should validate that each defined machine name actually starts with "$module.". — In addition, we should have a critical beta blocker to rename all route names in core to match the link machine names (which should be covered by a test on top).

3. At the same time, I wonder (1) whether we really need to prefix all routes and menu link names manually and (2) what the application-wide situation of such prefixing is. In particular, I wonder whether we shouldn't adopt the namespacing approach of the asset library system some more; i.e.:

No prefixes within your own definitions. To reference a definition of something else, use a "provider/" or "provider:" prefix.

In the concrete example here:

All that discussion has to be done in another issue, really. We use route names now everywhere, not just plain old menu links. (redirects, "configure" in info files, local tasks/actions ...) If you really want to change the behavior we have, let's do it not here.

Xano’s picture

pinged tim.plunkett. We couldn't think of a better name than hook_menu_link_defaults.

This changes the name of the hook and related functions.

Wow, that was quick. Thanks!

pwolanin’s picture

Title:Move definition of menu links to hook_default_menu_links(), decouple key name from path, and make 'parent' explicit» Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit
Issue summary:View changes
Xano’s picture

StatusFileSize
new139.23 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new5.44 KB

Some minor adjustments.

Edit: Right, so I renamed all menu_links_... functions to menu_link_..., so it will be a little easier to move them to the Menu Link module in a follow-up.

The last submitted patch, 241: drupal_2047633_241.patch, failed testing.

The last submitted patch, 241: drupal_2047633_241.patch, failed testing.

dawehner’s picture

Working on the rerole.

dawehner’s picture

StatusFileSize
new2.37 KB
new139.63 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links-2047633-244.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Did anyone considered "hook_menu_default_links"? But I agree that the current way is fine, too.

It looks and feels a bit icky that a central condition is buried so deeply into call chains.

Due to that, for example, the calling code disregards that fact and proceeds to clear all kinds of caches, whereas that only makes sense if there actually is any menu link related data in the affected caches.

Yes, in a perfect world nothing in core will require menu links beside the menu_link module itself, so we can move this code over. This is clearly work for a followup.

Is this obsolete debugging code? If not, can we throw an exception instead?

It totally is, let's write something to watchdog to help developers. We could also discuss whether throwing an exception is the actual right thing to do, as developers should see this AS FAST as possible. Fail early!

$all_links can be empty, which will cause this query to blow up.

This has been a bug before, but sure, let's fix it immediately.

1. I think we are pushing an extraordinary amount of layers of indirection onto D8 developers. Two property values here are referencing IDs that are outside of the visible scope of the author.

The problem of the discoverability of route names is a major concern indeed. https://github.com/dawehner/drupal-webprofiler (don't try it at home) tries to solve this and many other problems.

Xano’s picture

Did anyone considered "hook_menu_default_links"? But I agree that the current way is fine, too.

That would not fit in the menu_link namespace that all this will eventually move to, nor will it pave the way for a hook_ENTITY_TYPE_defaults() in the future.

effulgentsia’s picture

I discussed this with Dries and he agreed with the array/object/yaml discussion, the link vs. route machine name discussion, and the hook_menu() implementation removal all being allowed API changes in follow ups, provided that we commit this initial patch after this week's alpha release. Then, whatever we don't get done by the following alpha (if anything), he'll reevaluate for whether it's still an approved API change based on comparing the benefit of the improvement vs. the downside of breaking things multiple times for module developers chasing alphas.

So once this is RTBC again and we've released the alpha, this is on the table for any core maintainer who's reviewed the full patch to commit.

effulgentsia’s picture

Alphas are completely arbitrary, so holding up progress on critical issues due to an arbitrary marker feels counterproductive

Just to clarify on this, yes, while alpha markers are arbitrary, it's also the most stable thing we have right now for early module porters. So if we're going to split a single conceptual API change (how you declare menu links) into multiple commits, it's best if we can batch them into a single alpha cycle. In this case, our next alpha is only a day or two away, so that's not holding up progress on this issue much. If a similar thing comes up elsewhere, the pros/cons calculation might be different based on those circumstances.

sun’s picture

nor will it pave the way for a hook_ENTITY_TYPE_defaults() in the future.

Hm. Please correct me where I'm wrong, but I think that goal is misleading and concerning:

From an architectural perspective, this use-case is not a regular/legal use-case of a "hook that provides default (entity) content". → Such hooks would only be invoked exactly once in their entire lifetime, and that is in the installer.

Due to that, the architectural implementation of this hook here is also not comparable with default configuration files (which are processed and installed exactly once).

In my mind, I still think that the info hook pattern of declarative information, which may or may not be overridden/altered/customized through other means, still comes closest to the proposed architecture, and hence, hook_menu_link_info() would be the most natural choice — for now.

All of that being said, an interesting question remains to be why we are even attempting to retain and apply this custom "declarative, but then again customizable, hence declarative, but not actually declarative" pattern in the first place?

What if menu links were only installed once per module? What if you had to use the (natural) facility of hook_update_N() to update your "default" links? Just like you have to do for literally everything else in the system? Why the special casing here?

The only use-case for that is development. Can we think of an easy new facility that allows developers to "erase + re-install" their default links, so that "declarative" and "defaults" actually has a meaning?


I did not have time to study the new patches/interdiffs yet. However, upon a super-quick glance, the resulting DX caused by the 3D hierarchy of machine names still exists and is still concerning:

+  $links['action.admin.actions'] = array(
+    'parent' => 'system.admin.config.system',

No need to explain architectural details; that's all well-understood. That's a tough nut to crack. ;)

Perhaps — and this is borderline silly, but then again possibly not — we should go back to mixing "default menu link" information into the routing definitions?

(→ so as to remove two layers of indirection and go with direct route name references for the >98% use-case instead; the remaining poor 2% can happily use a bad-ass complex facility in order to be able to register multiple links for a single route, unless their use-case isn't to be solved via routes in the first place.)

pwolanin’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community
Related issues:+#2178697: Move code for finding/saving default menu links from menu.inc to menu_link module

added another follow-up: #2178697: Move code for finding/saving default menu links from menu.inc to menu_link module

I think this is back to rtbc after the final feedback by Xano.

effulgentsia’s picture

Unassigning from catch, since he provided the requested feedback. Any committer can commit, though I'm guessing he'll probably be the one to grab it first; he can reassign to himself if he wants to.

Setting status to postponed until alpha8 is released.

Filed two more follow ups: #2178723: [meta-2] Finalize module-facing API of declaring menu links and #2178725: Make all core menu link machine names and the corresponding route names match.

@sun and other reviewers: please file follow ups for any of your concerns not yet addressed, or restate in another comment here what you believe should block this patch's commit.

catch’s picture

Fine with those follow-ups.

catch’s picture

Status:Postponed» Needs review

245: menu_links-2047633-244.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 245: menu_links-2047633-244.patch, failed testing.

jhodgdon’s picture

One other thing. Due to
#2168011: Remove all 7.x to 8.x update hooks and disallow updates from the previous major version
it appears that we should not be making new hook_update_N() functions, right? As far as I know, we aren't going to support 7 to 8 database updates, and as of right now anyway, I think we are not supporting 8 to 8 database updates either. So the patch hunk in system.install I think should be removed.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new890 bytes
new139.62 KB
PASSED: [[SimpleTest]]: [MySQL] 59,732 pass(es).
[ View ]
new138.75 KB
PASSED: [[SimpleTest]]: [MySQL] 59,824 pass(es).
[ View ]

I think postponing an issue has the potential to leave issues alone, which complicates things later.

catch’s picture

Status:Needs review» Reviewed & tested by the community
pwolanin’s picture

@sun - the patch still supports the "reset" functionality for default links. Also, we are just maintaining the existing behavior that changes to the hook are reflected in non-customized links.

effulgentsia’s picture

Re #258: yep, and changing that is one of the options being discussed in #2178723: [meta-2] Finalize module-facing API of declaring menu links.

catch’s picture

Title:Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit» Change notice: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit
Priority:Critical» Major
Status:Reviewed & tested by the community» Active
Issue tags:+Needs change record

OK. Committed/pushed to 8x., thanks!

This needs a change notice, but let's please reference the follow-up issues from that and make it clear things could still change again later.

effulgentsia’s picture

Title:Change notice: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit» Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit
Assigned:Unassigned» catch
Priority:Major» Critical
Status:Active» Reviewed & tested by the community

Was this pushed?

alexpott’s picture

Title:Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit» Change notice: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit
Assigned:catch» Unassigned
Priority:Critical» Major
Status:Reviewed & tested by the community» Active

Nope. Committed 531fd59 and pushed to 8.x.

tim.plunkett’s picture

This seems to have introduced some weird visual bugs: #2180375: menu_link_rebuild_defaults() is creating duplicates of a few menu links

kim.pepper’s picture

Suggested change notice

Menu links moved from hook_menu() to hook_menu_link_defaults()

In Drupal 7 you would define routes, menu links, local tasks, and local actions in hook_menu().

Drupal 8 now defines routes in a routing.yml file and default menu links in hook_menu_link_defaults(). These are keyed by a machine name instead path, and explicitly define their parent route by name, instead of it being implied by path structure.

Drupal 7:

<?php
/**
 * Implements hook_menu().
 */
function book_menu() {
 
$items['admin/content/book'] = array(
   
'title' => 'Books',
   
'description' => "Manage your site's book outlines.",
   
'page callback' => 'book_admin_overview',
   
'access arguments' => array('administer book outlines'),
   
'type' => MENU_LOCAL_TASK,
   
'file' => 'book.admin.inc',
  );
}
?>

Drupal 8:

<?php
/**
 * Implements hook_menu_link_defaults().
 */
function book_menu_link_defaults() {
 
$links['book.admin.outlines'] = array(
   
'link_title' => 'Books',
   
'description' => "Manage your site's book outlines.",
   
'parent' => 'system.admin.structure',
   
'route_name' => 'book.admin',
  );
}
?>
alexpott’s picture

Title:Change notice: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit» Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit
Priority:Major» Critical
Status:Active» Needs work

In light of #2180375: menu_link_rebuild_defaults() is creating duplicates of a few menu links and the global sprint weekend reverted this commit.

pwolanin’s picture

StatusFileSize
new9.09 KB
new139.98 KB
FAILED: [[SimpleTest]]: [MySQL] 62,990 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Pulling the changes over from #2180375: menu_link_rebuild_defaults() is creating duplicates of a few menu links

This patch fixes most of the problems, but still has a couple test fails it seems, leaves the menu bar in a different order, and at least the disabled search module link isn't showing up in the menu admin screen.

pwolanin’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 266: menu_links-2047633-266.patch, failed testing.

jhodgdon’s picture

Just a comment on the proposed change notice in #256 - this needs to be either added to the previous change notice that talked about replacing hook_menu() with routing.yml files, or the two need to be cross-linked. If it's a new notice (which is probably better), the parts of the previous notice that deal with hook_menu() still being used need to be removed and replaced with a link to this new change notice. (The previous change notice is linked in the sidebar of this issue: https://drupal.org/node/1800686)

pwolanin’s picture

Status:Needs work» Needs review

266: menu_links-2047633-266.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 266: menu_links-2047633-266.patch, failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new5.39 KB
new139.57 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/taxonomy/lib/Drupal/taxonomy/Controller/TaxonomyController.php.
[ View ]

This should fix the test fails as well as several minor differences relative to the links that appear in HEAD.

Status:Needs review» Needs work

The last submitted patch, 272: menu_links-2047633-272.patch, failed testing.

pwolanin’s picture

Status:Needs work» Needs review
StatusFileSize
new1.77 KB
new139.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links-2047633-274.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Fixes duplicate method due to bad git rebase and adds a small test case Tim wanted to see.

kim.pepper’s picture

Thanks jhodgdon for the feedback in #269. I'll wait for this to get back in before making any further changes.

dawehner’s picture

After a quick look at the existing interdiffs, I could not see any major things, but yeah this issue and its outcome is a bit intimidating.

dawehner’s picture

  1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
    @@ -298,37 +298,29 @@ public function executeHookDefaultMenuLinks(array &$existing_links) {
    +          'machine_name' => $menu_link_id,

    I wonder whether this is needed? I though we don't require that.

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/PathPluginBase.php
    @@ -298,37 +298,29 @@ public function executeHookDefaultMenuLinks(array &$existing_links) {
    -      if ($menu['type'] != 'none') {
    ...
    -      // Set the title and description if we have one.
    ...
    +      if (!empty($menu['type']) && $menu['type'] == 'normal') {

    +1

pwolanin’s picture

@dawhner - re: 276 setting the machine name in the link was absolutely needed or an additional link was saved on each cache clear.

dawehner’s picture

Issue tags:+Needs manual testing

I guess we should really add some more manual testing to be sure.

aspilicious’s picture

Issue tags:-Needs manual testing

Did some manual testing with simplytest.me. Also cleared the caches a couple of times. Everything looks ok..

dawehner’s picture

Does anyone want to rtbc this again?

pwolanin’s picture

274: menu_links-2047633-274.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 274: menu_links-2047633-274.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new139.53 KB
PASSED: [[SimpleTest]]: [MySQL] 63,272 pass(es).
[ View ]

Re-roll

pwolanin’s picture

needs work - left some cruft from HEAD in the re-roll like:

   // Do not delete entries with an empty path as this can remove menu links in
   // the process of being created.
   $paths[] = '';
pwolanin’s picture

StatusFileSize
new139.58 KB
PASSED: [[SimpleTest]]: [MySQL] 63,198 pass(es).
[ View ]

Here's my re-roll on HEAD.

pwolanin’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new974 bytes
new139.8 KB
PASSED: [[SimpleTest]]: [MySQL] 63,278 pass(es).
[ View ]

This makes a tiny change to fix a code comment and add another condition to the query that finds stale links to delete. I think this is in the spirit of the change in HEAD to exclude empty string paths.

I verified the tests we've seen fail recently pass locally: Drupal\menu\Tests\MenuTest and Drupal\standard\Tests\StandardTest

We resolved the problem with the access checker in #2181293: AccessManager::checkNamedRoute() is not passing all route defaults (or building a complete route request). That solved the problems of the disabled search module link not showing up in the menu admin screen.

Since then this patch has also been manually tested and re-reviewed. So, putting back to rtbc (on the assumption the re-test will come back clean).

kim.pepper’s picture

Confirming the diff between #284 and #286 is just the cruft in #285.

sun’s picture

I did not re-RTBC this patch, because I was missing test coverage for the regressions that were discovered in #2180375: menu_link_rebuild_defaults() is creating duplicates of a few menu links

Frankly, I did not understand why we reverted this commit, instead of fixing the regressions, including proper test coverage in the separate follow-up issue... ("tested manually" appeared a dozen of times in recent comments here, which IMO is an inappropriate way to fix bugs/regressions in a fundamental base system facility like this)

However, I agree we should move forward here now.

This (re-)commit blocks #1996238: Replace hook_library_info() by *.libraries.yml file, which was RTBC already, and which I don't want to re-roll against reverted HEAD, because a commit of that would only break this monster here.

alexpott’s picture

Title:Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit» Change notice: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit
Priority:Critical» Major
Status:Reviewed & tested by the community» Active

Committed acd3a59 and pushed to 8.x. Thanks!

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.php
@@ -78,6 +78,12 @@ function testMenu() {
+    $this->assertIdentical($before_count, $after_count, 'menu_link_rebuild_defaults() does not add more links');
...
+    // Verify that the menu links rebuild is idempotent and leaves the same
+    // number of links in the table.
+    $before_count = db_query('SELECT COUNT(*) FROM {menu_links}')->fetchField();
+    menu_link_rebuild_defaults();
+    $after_count = db_query('SELECT COUNT(*) FROM {menu_links}')->fetchField();

@sun Here is the test coverage that would have caught the problem introduced by the initial commit. The reason this was reverted was that the global sprint weekend was just about to occur and we didn't people to be distracted by duplicate links whilst working on other issues.

dawehner’s picture

Status:Active» Needs review
jhodgdon’s picture

The previous change notice for hook_menu -> routing.yml also needs an update:
https://drupal.org/node/1800686
as it is not accurate any more.

I did a little grammar/typo cleanup on the new change notice, and separated out the Drupal 7, early Drupal 8, and New Drupal 8 code for (I hope) clarity.

tstoeckler’s picture

I updated the change notice referenced in #292: https://drupal.org/node/1800686/revisions/view/6853939/6877877
It appears as though dynamic menu links (i.e. those with a % placeholder) are not converted to hook_menu_link_defaults()?! I wasn't really sure why that is, but the change notice needs to be updated for that part as well. I just left that part as is, for now.

The change notice for *this* issue looks good. I just had one additional note, which I didn't directly add because I'm not 100% sure about it:

In D7 we had title_callback support in hook_menu(), but D8 hook_menu_link_defaults() doesn't support that right, in favor of _title_callback on routes?! I think that would be a useful addition for the change notice.

Xano’s picture

This seems to have broken \Drupal\system\SystemManager::getBlockContents(), which still uses {menu_router}, which in turn uses hook_menu(), but nog hook_menu_link_defaults(). Is there an issue for that already?

Xano’s picture

mondrake’s picture

Hm. Menu links no longer get removed if you uninstall the module they are provided by.

Tested on simplytest.me:

  1. install the 'Testing' module
  2. the link to 'Testing' appears in the 'Development' section at admin/config
  3. uninstall the 'Testing' module
  4. the link to 'Testing' still appears in admin/config
  5. clicking on the link you get an exception Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "\drupal\simpletest\form\simpletesttestform"
xjm’s picture

For #296, let's file a separate issue and reference here unless the issue is severe enough to require a rollback (as @Xano did in #294-#295). Here, we just need review on the change notice updates in #291 - #293. Thanks!

xjm’s picture

Issue tags:+Missing change record

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release.

mondrake’s picture

Gábor Hojtsy’s picture

Title:Change notice: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit» Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit
Priority:Major» Critical
Status:Needs review» Fixed
Issue tags:-Needs change record, -Missing change record

Cross-linked the change record to docs and added one more notable change. I think the change record is clear and understandable.

Status:Fixed» Closed (fixed)

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

eileenmcnaughton’s picture

My understanding of this thread is that the change has been made & https://drupal.org/node/1800686 needs to be updated from

"In Drupal 8, these two concepts will be separated: hook_menu() will remain the place where menu items"

to

"In Drupal 8, these two concepts will be separated: hook_menu_defaults() replaces hook_menu and is the place where menu items names are registered for a URL path"

Can someone confirm I have understood this correctly - I think the complete edit would be to replace

=========================

Note: This next paragraph is quite possibly not the final answer! See #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit for further developments.

In Drupal 8, these two concepts will be separated: hook_menu() will remain the place where menu items are defined for the UI and where route names are registered for a URL path. The actual mappings between route names and the page and access callbacks are defined in a file called module_name.routing.yml.
Since the new routing system is still in development, things that seem complicated and redundant are subject to further changes and improvements until the launch of Drupal 8.

Note: In previous versions of Drupal 8 hook_menu() was still used to define menu links. This was changed in #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit. Refer to the corresponding change notice for more information. The following paragraphs depict the current status of Drupal 8.

=======================
WITH
=========================

In Drupal 8, these two concepts will be separated: hook_menu_defaults() replaces hook_menu() as the place where
- menu items names are registered for a URL path,the place where
- menu items are defined for the UI
- route names are registered for a URL path.

The actual mappings between route names and the page and access callbacks are defined in a file called module_name.routing.yml.

Since the new routing system is still in development, things that seem complicated and redundant are subject to further changes and improvements until the launch of Drupal 8.

Note: In previous versions of Drupal 8 hook_menu() was still used to define menu links. This was changed in #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit. Refer to the corresponding change notice for more information. The following paragraphs depict the current status of Drupal 8.
===================================

Xano’s picture

Xano’s picture

jhodgdon’s picture

Regarding #302, your edit is not quite right... I think the paragraph that is there is OK for now, and it does link to the new change notice that explains the current state of affairs for hook_menu_link_defaults().

eileenmcnaughton’s picture

OK - well if its not resolved that's fine - but being pointed to a 300+ comment thread for step 1 of getting to grips with d8 obviously has the result of putting things in the too hard basket for people just dabbling with Drupal 8 so it would be good to swap that out for the 'answer' once it is resolved.

jhodgdon’s picture

Good point. I changed the wording slightly and hopefully people will now see that they should go to the new change notice for more information, not the (as you say) 300+ comment issue.

eileenmcnaughton’s picture

OK - that is an improvement. At least it gives the reader more confidence in the page they are reading (which as far as I could tell is up to date - https://drupal.org/node/1800686)

Mixologic’s picture

  1. '<front>':
       path: '/'
    +  defaults:
    +    _title: Home
       requirements:
         _access: 'TRUE'

    This is super minor, but I noticed when reviewing all the routing.yml files that this is the only example of a title callback that is not surrounded by single quotes.. Should this be _title: 'Home'

chrisshattuck’s picture

Issue summary:View changes

Fixing spelling error that threw me off.