The menu/routing topic on api.drupal.org is woefully lame. It is filled with @todo instead of the information that should be there.
https://api.drupal.org/api/drupal/core!includes!menu.inc/group/menu/8

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Time to write some more docs...

jhodgdon’s picture

Status: Active » Needs review
Issue tags: +revisit before release
FileSize
17.24 KB

Here's an initial patch. Hopefully it's at least better than the horrible thing that is on that page now (which is a bunch of left-over D7 docs mostly), and hopefully reviewers will find and gently point out the typographical and factual errors.

I've left a few To Dos in the file. I was asking pwolanin and timplunkett in IRC about the hooks and derivatives, and they said it's still in flux. So I'm adding the "revisit before release" tag to this issue (or else we should plan to open a separate issue after this one is done for now; I'm not sure what the usual policy is on that?).

joachim’s picture

Status: Needs review » Needs work

Looks very good! (I wish there'd been overviews like this a year ago!!!)

Just a few things:

  1. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + *     _permission: 'access site reports'  * block.admin_display:
    

    Is that line meant to look like that?

  2. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * In the path, '{block}' is a wildcard - it will be replaced by the
    + * ID of the block that is being configured. The entity system takes care of
    + * reading the ID, instantiating the block entity, and passing it to the
    + * entity form controller so that the editing form can be built and displayed.
    

    Should we mention that this works because 'block' is the name of an entity type? Also, how does the entity system register that it can deal with wildcards?

  3. I gather that the menu links system is probably about to change substantially, but best to go ahead and commit this so what's currently there is documented, and update it later.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
20.09 KB
4 KB

Thanks for the review!

(1) was a copy/paste error, whoops.

(2) good point, but I don't think I want to get into it in this topic... so took out even more of the text and pointed to the Entity API docs. I also added a new section to those docs about routing.

Crell’s picture

  1. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * - The 'requirements' section gives access permission instructions. Most
    + *   routes have a simple permission-based access scheme, as shown in this
    + *   example. See the @link user_api Permission system topic @endlink for
    + *   more information about permissions.
    

    Requirements are primarily for adding regexes to restrict the route parameters. We've usurped it for other stuff like permissions. That should be mentioned here too.

  2. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * @section sec_wildcards Defining routes with wildcards
    + * Some routes have wildcards in them, and these can also be defined in a
    + * module_name.routing.yml file, as in this example from the Block module:
    

    Nowhere else do we call these wildcards, because that's not really the right term. "Placeholder" is what I've been using most.

  3. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * only requirement is that the method specified in your *.routing.yml file
    + * return a render array (see the
    + * @link theme_render Theme and render topic @endlink for more information).
    + * As a note, if your module registers multiple simple routes, it is usual
    + * (and usually easiest) to put all of their methods on one controller class.
    

    Returning an HtmlFragment object is also supported. Actually so is an HtmlPage object, or a Response.

  4. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * - weight: Lower (negative) numbers come before higher (positive) numbers,
    + *   for menu items with the same parent.
    

    Note that if no weight is specified or 2 routes have the same weight they're then sorted alphabetically by...? (I'm not actually sure what it is right now.)

Gábor Hojtsy’s picture

Generally a good one, some points:

  1. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * responders. If a match is made, Drupal will then instantiate the required
    + * classes, gather the data, format it, and send it back to the web browser.
    + * Otherwise, Drupal will return a 404.
    

    May also return a 403 if you want to mix in that the routes check access too :)

  2. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * The menu system is based on the routing system; it is used for navigation
    

    I think it may be a mis-statement that menu is based on the routing system, the menu system uses routes but not really "based" on it I guess. Not in a sense of how the routes are based on Symfony's routing at least.

  3. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + *     _permission: 'access site reports'  * block.admin_display:
    

    Paste error?

  4. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + *   - _content: A method on a page controller class (see @ref sec_controller
    + *     below).
    + *   - _form: A form controller class. See the
    + *     @link form_api Form API topic @endlink for more information about
    + *     form controllers.
    + *   - _entity_form: A form for editing an entity. See the
    + *     @link entity_api Entity API topic @endlink for more information.
    

    If you want to list these, then listing _controller would be also key.

  5. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * Local tasks need to be defined as a group, which includes the base
    + * route as the main tab, and additional routes as other tabs. Static local
    

    Not sure explaining it like a group is a good idea. Contextual links require a group defined from PHP. It is true local tasks are not shown if there is not at least 2 on the same level to be shown. Not sure documenting it as a 'group' is a good idea. It sounds confusing vs. contextual links.

  6. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * - base_route: The machine name of the main task (tab) in your group of local
    + *   tasks.
    

    Worth mentioning that if the base_route == route_name then its the default tab to be shown on the route on the given level.

  7. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * hook_menu_local_tasks_alter().
    

    Ha, I did not know. Updated https://www.drupal.org/node/2122253/revisions/view/7366961/7383453 :)

  8. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * hook_menu_local_actions_alter().
    

    Good point, documented at https://www.drupal.org/node/2133247/revisions/view/7366953/7383463

  9. +++ b/core/includes/menu.inc
    @@ -14,89 +14,234 @@
    + * hook_menu_contextual_links_alter().
    

    This does not actually exist at least in the docs.

jhodgdon’s picture

FileSize
21.39 KB
8.01 KB

Thanks for the reviews! I think I've fixed most/all of the points from #5 and #6. A few notes:

#5 - point 4 - I don't know what the sorting is either and it is most likely in flux, so I'd prefer not to even mention it for now.

#6 - point 3 - was fixed in previous patch. I think you reviewed #2?

#6 - point 9 - typo on my part, this should have said hook_contextual_links_alter().

jhodgdon’s picture

Oh weird. The patch I uploaded in #4 didn't show up at the top of this issue in Files. ?!? Strange. It didn't get run through the testbot either. No wonder you might have reviewed the patch in #2 instead! Anyway. The latest interdiff is from the patch in #4, and comment #4 has an interdiff to the patch in #2 in case that is what you reviewed.

webchick’s picture

Status: Needs review » Fixed

This looks great to me, and seems like all of Gábor's feedback was addressed. Going to err on the side of getting this in sooner than later, given it's one of the primary APIs that module developers use and the existing info is "woefully lame." ;)

Committed and pushed to 8.x. Thanks!

  • webchick committed 21fc02c on 8.x
    Issue #2290129 by jhodgdon, Gábor Hojtsy, Crell, joachim: Menu/routing...

Status: Fixed » Closed (fixed)

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

ianthomas_uk’s picture

Issue tags: -revisit before release +revisit before release candidate

revisit before release candidate is a more appropriate tag. The tag is needed because the system wasn't finalised when the documentation was written.

webchick’s picture

Issue tags: -revisit before release candidate

This ended up getting completely revisited and re-written at https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Menu%21me... so I believe we can remove this tag now.