We have several somewhat overlapping places where various bits of the menu/routing system are documented:
https://drupal.org/node/2122223
https://drupal.org/developing/api/8/routing
https://drupal.org/node/2046371
http://drupal.stackexchange.com/questions/89906/why-are-routing-files-fi...
https://api.drupal.org/api/drupal/core!includes!menu.inc/group/menu/8
https://drupal.org/node/1800686

We need three kinds of documentation:

  1. Change records to document the before-and-after for hook_menu() and its ilk from D7 to D8. Already partially done in the appropriate issues, but #2244777: Document all the menu changes (tasks, actions, contextual links, menu links) from 7 to 8 in the WSCCI change record exists to cover the gaps.
  2. API documentation in the codebase for how routes are declared, especially for Drupalisms like our various route parameters. Slightly trickier because these are not something we can just stick a docblock on like we can a function, class, or class member. See #2238935: [meta] Complete missing documentation for special strings and metadata like annotation keys, routing parameters, tagged services, etc.. This should take the form of an overhaul of the routing and menu group as shown in https://api.drupal.org/api/drupal/core!includes!menu.inc/group/menu/8.
  3. A comprehensive overview in the handbook that explains not only the API itself but how to develop with it. We have a good start at this already at https://drupal.org/developing/api/8/routing and in its sub-pages, but there's a lot more to be filled in.

Original report by chx

If #2044969: Break the old router for contrib is a go then it is now critical to document the router. There is zero documentation as far as I am concerned, the rough overview over at Symfony is inadequate even for them and doesn't cover Drupal at all. We need flowcharts, internals (so that one can debug it), explanations (why do we use _controller / _form instead of just form? etc) and examples.

For posterity, the documentation or some semblance of it now can be found at https://drupal.org/node/2122071 . It didn't even take a year to get some documentation up. And that, of course, was not written by anyone in the WSCCI team because they are exempt from such small details like producing developer friendly, performant, documented code.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Lets do it on drupal.org: https://drupal.org/node/2046371

Note: this was just a start and needs flowcharts etc.

willyk’s picture

Daniel - I can start working on helping with this issue by working from your draft and starting on some flowchart/summaries. Any guess on how much work/time is needed to address this issue? As it might help to set a target timeline timeline/overall scope for what's involved (and if needed actively recruit some others to pitch in). I'll be at MWDDS this w/end if you want to discuss.

- Willy

dawehner’s picture

It is for sure hard to tell how much time it will need to document the router, especially as this depends on your knowledge of the router system.

I won't by at MWDDS but you can always contact me via IRC

willyk’s picture

I have a solid understanding of the D7 router system and experience implementing in custom modules. I've been learning the D8 router system and have been learning the Symfony routing system and understand/follow what's outlined here: https://drupal.org/node/1800686. So just wondering if you think I could pitch on this issue (at least with initial drafts) or defer to someone with more expertise? As mentioned, I could revised the draft that you've prepares and take a shot at some flowcharts.

tim.plunkett’s picture

Please go for it!

webchick’s picture

Agreed, that's fantastic! And it's always easier for people to review and correct documentation that exists than make it up to begin with. :)

Thanks for taking a stab, willyk!

webchick’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

xjm’s picture

Component: routing system » documentation
Issue tags: +WSCCI

We also need actual API documentation in the codebase, ideally a @defgroup that includes various controller-related classes and describes the different _parameterthingies and such.

Moving to the correct component to get @jhodgdon's review once we have a patch.

jhodgdon’s picture

Title: Document the router » Menu routing docs do not match what the code is doing
Category: task » bug

I ran into this last week when I was trying to update some code to Drupal 8. The API docs on the router at
https://api.drupal.org/api/drupal/core!includes!menu.inc/group/menu/8
have not been updated to 8, and hook_menu() docs do not mention the new routing.yml stuff at all. This really needs to be fixed and is really a docs bug that they weren't updated when the menu routing system was overhauled.

dawehner’s picture

Well at least menu.inc/the group menu is certainly the wrong place as it is all about old code. We probably want a group called "Routing"

catch’s picture

Category: bug » task
catch’s picture

Category: task » bug
Priority: Critical » Major

xjm pointed out in another issue that bad docs are bugs, so compromising at major bug - there's no fatal error, data loss or security issue so not critical.

webchick’s picture

By that definition, should missing change notices be demoted to major tasks, then?

catch’s picture

Well it depends on whether it should block the release or not, we (as in possibly you and me personally in irc) had the idea of making change notices critical so they'd block the release and all get done in time. Probably they should still do that so they ought to be critical.

I don't mind if major docs omissions are major bugs or critical tasks, but the critical bugs queue needs to reflect actual, functional bugs otherwise it's impossible to have an overview of what's there. I've personally moved about 8 issues out of it today that were in the wrong place.

Critical tasks is 'stuff we have to do by release' which more open to interpretation.

I guess it comes down to if the last 10 critical tasks were all change notices, would we release RC1, or wait for them to be done?

webchick’s picture

Good question. We should probably split this off into its own discussion. Did so here: #2078085: Determine if change notices should block an 8.0 RC?

joachim’s picture

I just spent a couple of hours head-desking because {params} in routing.yml that happen to be the name of an entity type get turned into entity loaders.

> Well at least menu.inc/the group menu is certainly the wrong place as it is all about old code. We probably want a group called "Routing"

That, or find a suitable entry point to the routing system for the bulk of the docs to live on. Eg, which class processes routing.yml files?

Either way, we'd need a @see link from hook_menu().

As for whether this should block release -- do we want developers to be able to start porting their D7 modules? If so, docs are an essential requirement.

joachim’s picture

Might help to cover this in small chunks.

Here's one: #2087107: document routing.yml parameter converters.

Does the routing system have a docs entry point yet?

I guess there's https://api.drupal.org/api/drupal/core!includes!menu.inc/group/menu/8 but that is looking fairly out of date. I suggest we rewrite that, and add a new 'routing' topic.

clemens.tolboom’s picture

xjm’s picture

I mentioned this in #2106709: Remove legacy router backward compatibility layer but can we please pretty please put in a stopgap in the hook_menu() docblock that says "Hey you! Hands off that page callback! Use the routing system!" We can even reference the change notice if need be. Of course the sooner the in-code API docs are complete and correct the better, but I'd at least like to give people a clue.

amateescu’s picture

FileSize
717 bytes

Something like this? :)

jhodgdon’s picture

That patch and the previous ones should never have gone in without a docs update in hook_menu() in the first place. sigh.

effulgentsia’s picture

There are some good docs fixes in #2044969-36: Break the old router for contrib that we should bring into this issue, now that that issue is closed as a duplicate.

webchick’s picture

Priority: Major » Critical
Issue tags: +8.x-alpha4

This is a release blocker. In fact, I'd love to get this in for alpha4.

catch’s picture

Agreed with this blocking the release, but it's not a functional bug. Restoring state to the same as #11.

larowlan’s picture

Category: bug » task
Status: Active » Needs review
FileSize
9.48 KB

I think @catch meant to make it back to task as per #11
So here's the salient parts from #2044969: Break the old router for contrib

dawehner’s picture

Just in general we should get that one in as fast as possible as it will conflict with basically all other menu_router related patches.

+++ b/core/modules/system/system.api.php
@@ -668,15 +620,11 @@ function hook_menu_get_item_alter(&$router_item, $path, $original_map) {
   $items['example/feed'] = array(
     'title' => 'Example RSS feed',
-    'page callback' => 'example_feed',
-    'access arguments' => array('access content'),
-    'type' => MENU_CALLBACK,
+    'route_name' => 'example.feed',
   );
 

If you are hard that this entry can be removed as it is.

hass’s picture

Why are titles not removed from hook_menu if they are now in the route.yml?

dawehner’s picture

Please add a support question instead ... hook_menu() still contains menu links, which do have a title.

jhodgdon’s picture

Status: Needs review » Needs work

I started from scratch and reviewed the patch in #25.

Some things I think should be fixed: some small, some larger:

a) We use serial commas in Drupal docs. So,

+ * This hook enables modules to register links to be displayed in menus,
+ * as breadcrumbs and as local tasks.

should be a comma before "and".

b) The first line says this hook is for defining menu links and local tasks, but the text in (a) also mentions breadcrumbs. How about if we add breadcrumbs to the first line and get rid of the text in (a) completely? They would then be redundant. The first line could be "Define links for menus, local tasks, and breadcrumbs.".

c) The paragraph after this is incorrect in D8:

  *
  * hook_menu() implementations return an associative array whose keys define
  * paths and whose values are an associative array of properties for each
  * path. (The complete list of properties is in the return value section below.)

The keys do not define paths. They might *refer* to paths, but they don't define them.

d) Our API docs for Drupal 8 should not refer back to previous versions. So this paragraph should just be removed:

 * @section sec_callback_funcs Callback Functions
+ * The definition for each path may no longer include a page callback function.
+ * Registering page callbacks with hook_menu is not supported in Drupal 8.
+ * Instead you must create a module.routing.yml file which registers controllers
+ * for your module paths. See @link https://drupal.org/node/1800686 @endlink for
+ * further details.

And probably the next one too -- it's really not relevant for this hook to describe what used to be a function of hook_menu() in previous versions of Drupal. We need a separate topic for this, but it shouldn't be part of the hook_menu() docs.

+ * @section registering_menu_links Registering menu links
+ * For example your module.routing.yml could register path 'abc/def' as abc_def,
+ * to create a menu link and provide a page or breadcrumb title for this link
+ * you would add the following to your hook_menu() definition.
+ * @code
+ *   function mymodule_menu() {
+ *     $items['abc/def'] = array(
+ *       'title' => 'View abc for def',
+ *       'route_name' => 'abc_def',
+ *     );
+ *     return $items;
+ *   }
+ * @endcode
+ *

e) Actually, since the title of this issue is about menu routing docs, and not about hook_menu() we should probably add the separate topic about routing here and then we can put an @see into hook_menu(). Then this can be added to the meta-issue about making sure we have topics for new stuff in 8:
#2106873: [meta][policy, no patch] Make Drupal 8 developer docs more discoverable

f) The stuff about path component substitution seems to not exist in hook_menu() in D8 right now. However, some of the lines left in the hook docs refer to it. These need to be rewritten.

webchick’s picture

Just throwing this out there. We have an alpha release happening on Friday. Is it possible we could put the bulk of this in HEAD before then to address the critical issue of "this stuff is just flat-out wrong," but leave it open as a "major" to address further clean-up?

jhodgdon’s picture

I'll make a new patch. I still think the current patch has too much incorrect stuff in it.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
12.36 KB

How about this as a quick stopgap? I added a couple of @todos... also started modifying the Menu topic that was also totally wrong.

jhodgdon’s picture

FileSize
5.57 KB

forgot interdiff

juanolalla’s picture

Nice work!

@@ -19,17 +19,36 @@
+ * Once you have a route defined, you can use hook_menu() to define links
+ * and breadcrumbs for your module's paths in the main Navigation menu or other
+ * menus. See the hook_menu() documentation for more details.

I think we shouldn't suggest using hook_menu() to define links but the substitutions in development, like hook_default_menu_links() and BreadcrumbBuilderInterface implementation.

jhodgdon’s picture

I don't think we can suggest using hooks that will not be in the alpha. We could put a @todo there saying that hook_menu() will most likely be replaced, and clue people in that it's still to early to port modules.

jhodgdon’s picture

FileSize
546 bytes
12.51 KB

Here, with a @todo alert about hook_menu().

juanolalla’s picture

I'm agree jhodgon, and the @todo is a good point to avoid confusion with hook_menu()

dawehner’s picture

  1. +++ b/core/includes/menu.inc
    @@ -19,17 +19,38 @@
    + * Once you have a route defined, you can use hook_menu() to define links
    + * and breadcrumbs for your module's paths in the main Navigation menu or other
    + * menus. See the hook_menu() documentation for more details.
    

    The bit about breadcrumbs is not true anymore. Breadcrumbs are handled 100% via the routing system and paths.

  2. +++ b/core/modules/system/system.api.php
    @@ -491,21 +490,10 @@ function hook_menu_get_item_alter(&$router_item, $path, $original_map) {
    + * Define links for menus, breadcrumbs, and local tasks.
    

    Let's keep it to only define menu links, every other subsystem is thrown out/ will be thrown out.

  3. +++ b/core/modules/system/system.api.php
    @@ -517,25 +505,27 @@ function hook_menu_get_item_alter(&$router_item, $path, $original_map) {
      * $items['admin/config/system/foo/tab1'] = array(
      *   'title' => 'Tab 1',
      *   'type' => MENU_DEFAULT_LOCAL_TASK,
    - *   // Access callback, page callback, and theme callback will be inherited
    - *   // from 'admin/config/system/foo', if not specified here to override.
    + *   // Route name would be inherited from 'admin/config/system/foo', if not
    + *   // specified here to override.
      * );
      * // Make an additional tab called "Tab 2" on "Foo settings"
      * $items['admin/config/system/foo/tab2'] = array(
      *   'title' => 'Tab 2',
      *   'type' => MENU_LOCAL_TASK,
    - *   // Page callback and theme callback will be inherited from
    - *   // 'admin/config/system/foo', if not specified here to override.
    - *   // Need to add access callback or access arguments.
    + *   'route_name' => 'foo2',
      * );
    

    Let's skip those.

  4. +++ b/core/modules/system/system.api.php
    @@ -645,8 +574,6 @@ function hook_menu_get_item_alter(&$router_item, $path, $original_map) {
      *     - MENU_LOCAL_ACTION: Local actions are menu items that describe actions
    

    all the other items can be removed as well.

jhodgdon’s picture

Status: Needs review » Needs work

dawehner or someone else: Can you make a new patch? I don't have time today and also I don't really know the menu/routing system as well as you do, obviously. I was just basing this on the previous patches and guesswork. Status based on last comment.

juanolalla’s picture

Status: Needs work » Needs review
FileSize
5.26 KB
14.71 KB

Some improvements with the previous suggestions.

jhodgdon’s picture

I think this version is fine. Any other thoughts?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/includes/menu.inc
@@ -19,17 +19,38 @@
+ * @todo The rest of this topic has not been reviewed or updated for Drupal 8.x
+ *   and is not correct!

:)

---

All of the added docs are 100% correct, and the removals are accurate as well.

webchick’s picture

Priority: Critical » Major
Status: Reviewed & tested by the community » Active

Ok, excellent. Thank you SO MUCH all for all your help on this, everyone!

I'm going to go ahead and commit this, since it is obviously light years better than what is currently there. It sounds like we need to keep this open a bit longer though to resolve that @todo?

Committed and pushed to 8.x. Thanks!

Moving back to active and major. Or, feel free to close and re-open a separate, more focused issue that's not 50 replies long. :)

jhodgdon’s picture

Status: Active » Postponed
Issue tags: +revisit before beta

It sounds like we should actually postpone this until the hook that is going to be used for menu links is stabilized? There is little point in writing docs that will just have to be rewritten.

webchick’s picture

Good call!

This made it into alpha4, so untagging. :)

webchick’s picture

Issue tags: +revisit before beta

Uh. Not that.

webchick’s picture

Issue summary: View changes

Remove unhelpful sniping.

chx’s picture

Title: Menu routing docs do not match what the code is doing » The router is still undocumented
Priority: Major » Critical
Issue summary: View changes
Status: Postponed » Active
chx’s picture

Issue summary: View changes
Status: Active » Fixed
xjm’s picture

Based on #44 this should still be postponed, not fixed?

jhodgdon’s picture

Priority: Critical » Major
Status: Fixed » Postponed

Right, there is still the matter of whether hook_menu()'s remaining parts are going to be replaced by a different hook. Can we just leave this as postponed / revisit before release so we make sure the hooks got properly documented when they're stabilized? It's kind of important and the Docs Gate is not sufficient to make sure this happens.

webchick’s picture

Actually, I think with https://drupal.org/node/2122071 this is officially fixed. This includes not only a detailed listing of route properties at https://drupal.org/node/2092643 but also information on access-checking https://drupal.org/node/2122195 parameter up-casting https://drupal.org/node/2122223 and how to deal with dynamic routes https://drupal.org/node/2122201

It might be that there are small holes in what's here, but overall I think the "spirit" of the original post has been fulfilled.

I wonder if we want to open a less "aggro" issue, postponed and tagged "revisit before release" to do the final once-over on hook_menu() docs before shipping.

jhodgdon’s picture

The point in #44 was that the non-routing parts of hook_menu() were being discussed to be moved to a completely different hook name -- the parts that in D7 allowed you to also make it an Administration or Navigation menu entry. Has that happened? Is it happening? I realize it's not part of the Routing system, but the original issue title here was about how hook_menu() was not documented to reflect what it actually did, so I think it's in scope before chx changed the issue title a few comments up.

chx’s picture

Title: The router is still undocumented » Document the internals of the router
Priority: Major » Critical
Status: Postponed » Active

I have no idea how this issue got sidetracked into relatively small changes in doxygen. Please read the issue summary. Thanks!

chx’s picture

Because webchick asked in her AMA (even without mentioning names, I can read it well enough) I am willing to put up with the status quo if the internals gets documented and the DX gets fixed:

  1. #2109035: Make access checkers (much) easier to find
  2. #2092529: [meta] Improve DX for defining custom routes
  3. #2046367: Document the internals of the router
chx’s picture

Here are some questions. First questions on the new handbook pages:

  1. https://drupal.org/node/2122223 First, upcast is not a word I am familiar with. I'd expect it to be unknown to others too. A little explanation would go far. I get the impression that any {entity_type} is upcast but this is not made explicitly. It does state "all content and configuration entities automatically get upcast" but it's not clear that happens by specifying {entity_type}
  2. Same page, it's entirely unclear how we get from {node} to node_load? entity_load('node') ? In other words, if {foo} doesn't work where should I look?
  3. why the beginning underscores? Are there things without underscores? If there are, what are they? How could I provide my own (a _foo like _content or _entity_view)?
  4. https://drupal.org/node/2092643 Additional options on how the route should interact. Common options are _access_mode. Again, what else is there? How to provide, how to receive?

Now, what is completely missing is the "what happens internally" part. There's a box on https://drupal.org/node/2122071 which says [Routing]. How do we route? I have a vague idea that there is some initial matching and then there is some enhancing but the what does what and how can I extend any of that is completely missing. A URL comes in and a controller object comes out, right? There's a beginning on https://drupal.org/node/2046371 but that's mostly a @todo. Again, the reason for asking this is to be able to look at something when things go pear shaped. This is why I asked for a flowchart, like an Ariadne's thread.

joachim’s picture

> Same page, it's entirely unclear how we get from {node} to node_load? entity_load('node') ? In other words, if {foo} doesn't work where should I look?

Hell yes. I was was totally baffled by this and didn't manage to figure it out.

Berdir’s picture

http://slid.es/saschagrossenbacher/drupal-8-routing#/5 has a few notes about about upcasting (go down), look for the EntityResolver class for the implementation for entities.

There is a *great* post from dawehner about the underlines and routing in general on Drupal Answers.

dawehner’s picture

I tried to explain a bit question one and two, see https://drupal.org/node/2122223

why the beginning underscores? Are there things without underscores? If there are, what are they? How could I provide my own (a _foo like _content or _entity_view)?

I tried to explain that, as berdir said, in my stackoverflow page.

Regarding question 4: https://drupal.org/node/2092643/revisions/view/6698717/6722443

webchick’s picture

Can we please get stuff off of stack overflow and onto Drupal.org where people will actually be looking for it? :)

dawehner’s picture

It kind of answers more than the underscore question itself, but I am too lazy today to copy that into some place in the existing documentation, sorry: http://drupal.stackexchange.com/questions/89906/why-are-routing-files-fi...

Berdir’s picture

@webchick: Of course, that's why I mentioned it, it should be a nice base to document that stuff better. We can't and shouldn't get it *off* DA, though.

DA is a great platform to actually find out what we need to document and what people want to know and then figure out the best/correct answer together, after which we can also put it into d.o documentation/extend it. Encouraging people to ask questions there is IMHO one of the best things we can do, various people are trying to answer any d8 question that comes up there, see @chx blogpost about that and we're doing that quite successfully IMHO. Just have a look at the answered and unanswered questions at http://drupal.stackexchange.com/questions/tagged/8 :)

catch’s picture

Issue tags: -revisit before beta

Since this is already critical, removing 'revisit' tag.

xjm’s picture

Issue tags: +beta target
xjm’s picture

We have several somewhat overlapping places where various bits are documented:
https://drupal.org/node/2122223
https://drupal.org/developing/api/8/routing
https://drupal.org/node/2046371
http://drupal.stackexchange.com/questions/89906/why-are-routing-files-fi...
https://api.drupal.org/api/drupal/core!includes!menu.inc/group/menu/8
https://drupal.org/node/1800686

We need three kinds of documentation:

  1. Change records to document the before-and-after for hook_menu() and its ilk from D7 to D8. Already partially done in the appropriate issues, but #2244777: Document all the menu changes (tasks, actions, contextual links, menu links) from 7 to 8 in the WSCCI change record exists to cover the gaps.
  2. API documentation in the codebase for how routes are declared, especially for Drupalisms like our various route parameters. Slightly trickier because these are not something we can just stick a docblock on like we can a function, class, or class member. See #2238935: [meta] Complete missing documentation for special strings and metadata like annotation keys, routing parameters, tagged services, etc.. This should take the form of an overhaul of the routing and menu group as shown in https://api.drupal.org/api/drupal/core!includes!menu.inc/group/menu/8.
  3. A comprehensive overview in the handbook that explains not only the API itself but how to develop with it. We have a good start at this already at https://drupal.org/developing/api/8/routing and in its sub-pages, but there's a lot more to be filled in.
xjm’s picture

jhodgdon’s picture

Added #64 to issue summary, and due to item 2 in that, marking parent issue that is about updating the api.d.o landing page and the topics it links to.

webchick’s picture

Priority: Critical » Major
Status: Active » Fixed

Talked this over with the core maintainers, as well as jhodgdon.

1) We haven't thus far made the documentation of the internals of any other subsystem a release blocker (it's not part of the docs gate), even in the case of new APIs added to Drupal 8 (for example, the new configuration system). What is critical is module developer-facing documentation, which in contrast to documentation which affects only core contributors to a particular subsystem, affects bazillions of people. Keeping this issue critical would be a departure from policy, and that doesn't seem like a wise thing to do, especially given...

2) There is already internals documentation for the new routing system in the handbook under https://drupal.org/node/2122071. Is it a glorified shining example of internals documentation we hope it to be? Maybe, maybe not. (Let's be honest; probably not. ;)) However, it exists, and it should continue to be refined the same as any other documentation in the handbook. There's an Edit tab. If you find the docs unclear or insufficient, then use it.

3) For module developer-facing documentation, there are definitely some holes, but these are already captured in critical tasks #2244777: Document all the menu changes (tasks, actions, contextual links, menu links) from 7 to 8 in the WSCCI change record and #2238935: [meta] Complete missing documentation for special strings and metadata like annotation keys, routing parameters, tagged services, etc., and those will block release.

Therefore, I'm marking this issue both "major" and "fixed." If there's something specifically actionable to improve the documentation, and if you want to try and make the case that the existing docs are so bad we should actually hold up the entire release of Drupal 8 for another N months over it, then please open a separate, distinct, actionable issue and cross-link it from here.

Status: Fixed » Closed (fixed)

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

jhodgdon’s picture

Follow-up issue:
#2290129: Menu/routing topic needs an overhaul
is about the overview/landing page for menu/router stuff on api.drupal.org. If you're still following this issue... a review would be appreciated!