Split routing documentation out of menu.api.php

It would probably be best if we don't change the @defgroup machine names of the topics, but other than that I have no objection. Also, a number of the .api.php files got moved into appropriate sections of core/lib, so rather than putting this one in core/modules/system, we might consider moving it elsewhere (routing really isn't part of the system module).

Hi,

I was looking for an easy issue that I could help with, so I decided to give it a try.
So here is my first attempt to a patch... I was kinda clueless about what to move and what not, so I could be totally wrong.

1. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -405,30 +182,30 @@ function hook_menu_links_discovered_alter(&$links) {
   -      '#theme' => 'menu_local_action',
   -      '#link' => array(
   +    '#theme' => 'menu_local_action',
   +    '#link' => array(
   +      'title' => t('Add content'),
   +      'url' => Url::fromRoute('node.add_page'),
   +      'localized_options' => array(
   +        'attributes' => array(
              'title' => t('Add content'),
   -          'url' => Url::fromRoute('node.add_page'),
   -          'localized_options' => array(
   -              'attributes' => array(
   -                  'title' => t('Add content'),
   -              ),
   -          ),
   +        ),
   ...
   +    ),
   

   You took lines only to put them back

2. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -405,30 +182,30 @@ function hook_menu_links_discovered_alter(&$links) {
   -      '#theme' => 'menu_local_task',
   -      '#link' => array(
   -          'title' => t('Example tab'),
   -          'url' => Url::fromRoute('node.add_page'),
   -          'localized_options' => array(
   -              'attributes' => array(
   -                  'title' => t('Add content'),
   -              ),
   -          ),
   +    '#theme' => 'menu_local_task',
   +    '#link' => array(
   +      'title' => t('Example tab'),
   +      'url' => Url::fromRoute('node.add_page'),
   +      'localized_options' => array(
   +        'attributes' => array(
   +          'title' => t('Add content'),
   +        ),
   ...
   +    ),
   

   same here

Oh ok, let me try again:

Thanks for the patch(es)!

First off... It appears that something about the way you made the patch is wrong. The routing.api.php file is new, but it's showing up in the patch as being a patch of an existing file (which doesn't exist, hence the "Patch does not apply" error in the test). So, that needs to be fixed so that we can apply the patch.

That aside... looking at the patch, the contents need a bit of attention too, but it's pretty close:

1. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -11,17 +11,6 @@
     * Define the navigation menus, and route page requests to code based on URLs.
   

   We'll need to change this line so that it doesn't mention "route page requests". Maybe something like:

   Define navigation menus, local actions, local tasks, and contextual links.

2. A few lines up (not touched by this patch), the title "Menu and routing system" for this topic also needs to be updated.

3. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -11,17 +11,6 @@
   - * The Drupal routing system defines how Drupal responds to URL requests that
   - * the web server passes on to Drupal. The routing system is based on the
   - * @link http://symfony.com Symfony framework. @endlink The central idea is
   - * that Drupal subsystems and modules can register routes (basically, URL
   

   I don't think we can just leave this paragraph out entirely, because the next paragraph starts with "The menu system uses routes" and doesn't tell what routes are.

   Actually, I think we can fix it by adding a link in that next paragraph to the Routing API topic. So it can say something like "The menu system uses routes; see the @link routing Routing API topic @endlink for more information". (where "routing" is the machine name of the new routing topic; see below...)

4. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -38,238 +27,10 @@
   - * @section sec_links Defining menu links for the administrative menu
   

   This section is part of the Menu system, not Routing, so it should stay in menu.api.php

5. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -38,238 +27,10 @@
   - * @section sec_tasks Defining groups of local tasks (tabs)
   

   This section is part of the Menu system, not Routing, so it should stay in menu.api.php

6. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -38,238 +27,10 @@
   - * @section sec_actions Defining local actions for routes
   

   This section is part of the Menu system, not Routing, so it should stay in menu.api.php

7. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -38,238 +27,10 @@
   - * @section sec_contextual Defining contextual links
   

   This section is part of the Menu system, not Routing, so it should stay in menu.api.php

8. +++ b/core/lib/Drupal/Core/Routing/routing.api.php
   @@ -0,0 +1,257 @@
   + * @defgroup menu Menu and routing system
   

   The title of this new topic shouldn't say "Menu". Probably it should be "Routing API".

   Also, since it is a new @defgroup topic, it needs a new machine name identifier (not "menu"). Probably "routing" would be good.

   See
   https://www.drupal.org/node/1354#defgroup
   for more about @defgroup if this is confusing.

9. In the file core/core.api.php we have a link to the pre-existing "menu" topic that looks like this:
   

    * - @link menu Routing, page controllers, and menu entries @endlink
   

   That needs to be split into two links -- one about Menus to the topic "menu", and one about Routing to the new topic machine name, whatever it is (probably "routing", see item above).

10. +++ b/core/lib/Drupal/Core/Routing/routing.api.php
    @@ -0,0 +1,257 @@
    + * Define the navigation menus, and route page requests to code based on URLs.
    

    This also needs to not mention menus.

11. +++ b/core/lib/Drupal/Core/Routing/routing.api.php
    @@ -0,0 +1,257 @@
    + * @section sec_links Defining menu links for the administrative menu
    

    The topic should stop here -- as noted above, the rest of the sections should stay in the Menu topic and not move to the new Routing topic.

Thanks!

Thanks @screon, @jhodgdon. This addresses all comments in #12.

Great!

Yeah I don't know what I did wrong with the patch...Anyways, glad I could be of some help :)

@screon: Thanks for getting this started. I am not sure what you might have done but I can try guessing. It seems that you added the new routing.api.php file to index and then added all it's contents. You took a normal diff (git diff) after that. Since the file was already indexed, the patch shows that the file was modified rather than being created. This especially happens if you are using an IDE. I know PHPStorm immediately prompts you to add a file to git index as soon as you create it.

This may or may not be what happened in your case but the symptoms indicate so. I hope this helps.

Yup, sounds like that is exactly what happened. I normally don't allow PHPStorm to add new files to git when I get the prompt, but I guess I accidentally added the file anyways.

Thanks for your help!

Thanks -- this is a pretty good patch, but it actually needs a few small fixes:

1. +++ b/core/core.api.php
   @@ -24,7 +24,8 @@
   + * - @link menu Page controllers and menu entries @endlink
   + * - @link routing Routing API @endlink
   

   Page controllers are part of Routing, not Menu

2. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -2,27 +2,17 @@
   + * Hooks and documentation related to the menu system, and links.
   

   Should not be a , here

3. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -2,27 +2,17 @@
   + * The menu system uses routes; see the @link routing Routing API topic @endlink
   

   This line goes over 80 characters. Shouldn't.

   Note that you need to keep @link ... @endlink on one line together, so you'll have to move the whole thing down to the next line, and then rewrap the rest of the paragraph.

4. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -2,27 +2,17 @@
   + * for more information. it is used for navigation menus, local tasks,
   

   "it" needs to be "It".

Oh, one other thing I thought of.

On the current "menu" topic page:
https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Menu!menu.api.php...

There is a list at the bottom of functions and classes that have @ingroup menu in them.

At least one of them needs to be changed to "@ingroup routing" now (the class ControllerBase). I don't see anything else that needs to be changed there, I guess...

Actually it would be nice if, as a bonus, any non-vendor classes mentioned in the Routing topic had
@ingroup routing
added to their class doc headers... could be slightly out of scope for this issue but I think it would only add about 1-5 lines to the patch.

Re-rolled patch according to #18

Thanks for the new patch! However, it's still not ready to go:

1. +++ b/core/core.api.php
   @@ -24,7 +24,8 @@
   - * - @link menu Routing, page controllers, and menu entries @endlink
   + * - @link routing Page controllers and routing entries @endlink
   + * - @link routing Routing API @endlink
     * - @link form_api Forms @endlink
   

   Um. This is not right. We've lost the link to the menu topic, and instead have two links to the routing topic.

2. +++ b/core/lib/Drupal/Core/Menu/menu.api.php
   @@ -2,28 +2,18 @@
   + * The menu system uses routes; see the @link routing Routing API
   + * topic @endlink for more information. It is used for navigation menus,
   + * local tasks, local actions, and contextual links:
   

   @link ... @endlink all needs to stay on one line. So start @link on the next line.

3. Suggestions above in comments #19/#20 were not addressed.

Addressed #22 .

Thanks!

But... this patch doesn't look good at all actually. We lost routing.api.php entirely, and #19 and #20 are still not addressed.

Also:

+++ b/core/core.api.php
@@ -24,7 +24,9 @@
  * @section interface User interface
  *
- * - @link menu Routing, page controllers, and menu entries @endlink
+ * - @link routing Page controllers and routing entries @endlink
+ * - @link routing Routing API @endlink
+ * - @link menu entries @endlink
  * - @link form_api Forms @endlink
  * - @link block_api Blocks @endlink
  * - @link ajax Ajax @endlink
diff --git a/core/lib/Drupal/Core/Menu/menu.api.php b/core/lib/Drupal/Core/Menu/menu.api.php

This is still not right.

We need:

a) One entry with @link routing
b) One entry with @link menu
c) Both of them to have good link titles.

Right now we have two lines for routing and one for menu, and the menu one doesn't have a good link title. See
https://www.drupal.org/node/1354#link
for syntax if confused.

As part of Drupal Camp Baltics I created a patch on issues mentioned on comment #22. Hopefully this moves to right direction, this is my first time creating core patches :)

Added interdiff for @teemuaro's patch against the one it's done according to its headers.

The interdiff in #28 reveals that the patch in #27 is a good starting point. It just needs additions: namely the changes requested by @jhodgdon in #19 and #20.

I'll try to work on #19 and #20 as noted in the comment above.

Only adding patch fixing comment #19 now. Comment #20 includes a mention "non-vendor classes" that I'm not sure how to interpret, so this is gonna be left pending the answer by @jhodgdon (I tried calling after her on #drupal-docs in IRC but she wasn't around at the time).

Great, we're almost there!

So. "non-vendor classes" means "don't include classes in core/vendor" -- we only want to add "@ingroup routing" to classes that belong to us (things in core/vendor come from outside projects and we don't want to edit their documentation blocks). So, setting this back to Needs Work for #20.

I also took a look at the current patch. It looks good! One minor thing to update:

+++ b/core/core.api.php
@@ -24,7 +24,8 @@
+ * - @link menu Menu entries @endlink

Can we give this a longer link title, maybe:

Menu entries, local tasks, and other links

@Crell - I do NOT think we should tag all of those @ingroup. If you look at comment #20, it suggests that any class *mentioned in the @defgroup docs* should be tagged.

Taking another look into the ingroup additions!

New try at addressing #20. I seem only to have found 2 mentions of non-vendor "classes", one of which is an array and the other being used as an example of a BaseController extension.

Also took care of #32.

Classes/interfaces that I see in the routing topic that I think should have @ingroup routing in them:
\Drupal\Core\Controller\ControllerBase
\Drupal\Core\Routing\RouteMatchInterface
Even though the dblog class is mentioned as an example, I don't really think it should have @ingroup routing. Part of the reason is that people tend to copy/paste existing classes when they create new controllers, and we really don't need to have additional examples listed.

So can we get @ingroup routing added to those two classes, and removed from the dblog class? Thanks! The rest looks good to me.

Oh, I guess it is already in RouteMatchInterface. And ControllerBase. So let's just remove dblog... OK I can do that quickly by just dropping that section out of the patch file, and then set it to RTBC. Thanks!!!!

This seems like the failure affecting several issues right now. Retesting and tentatively marking RTBC as per #37.

What is with all the random test failures lately???

Random failure again. :)

Committed and pushed to 8.0.x. Thanks!