Updated: Comment #0

Problem/Motivation

We have several legacy services in place to support using hook_menu() for routing. We're just about to not have any more need for that, so let's start removing it.

Proposed resolution

Remove legacy_router
Rename router.dynamic to router, remove the existing ChainRouter
Attempt to remove legacy_access_subscriber, legacy_url_matcher, legacy_generator
Ensure _drupal_menu_item and USES_ROUTE no longer appear

Remaining tasks

Wait for the last 2 page callbacks to die.

User interface changes

N/A

API changes

Using hook_menu() for routing will no longer work.

#1971384: [META] Convert page callbacks to controllers

Files: 
CommentFileSizeAuthor
#14 remove-legacy-router-2106709-14.patch26.07 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,788 pass(es).
[ View ]
#14 interdiff.txt8.45 KBtim.plunkett
#8 remove-legacy-router-2106709-8.patch17.61 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,687 pass(es).
[ View ]
#7 remove-chain-router.patch18.18 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,799 pass(es).
[ View ]
#5 KILL_LEGACY_ROUTER-2106709-5.patch18.05 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#2 KILL_LEGACY_ROUTER-2106709-2.patch17.83 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

I'm inclined to leave the ChainRouter layer in place for now. It's overhead for only one router is minimal, and we've discussed using it for an optimized router for common routes (which its, actually, its primary use case).

Otherwise, KILL!!!

Status:Postponed» Needs review
StatusFileSize
new17.83 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Let er rip

Status:Needs review» Needs work

Missing some bits in common.inc. Search the docblocks as well.

Status:Needs work» Needs review
StatusFileSize
new18.05 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

I don't care about removing the string "page callback" from everywhere yet. This is a functional issue.

I added back too many a hunk when I was restoring ChainRouter.

StatusFileSize
new18.18 KB
PASSED: [[SimpleTest]]: [MySQL] 58,799 pass(es).
[ View ]

I'm going to debug this locally, but in the meantime here is the patch I originally wrote, before reading #1.

StatusFileSize
new17.61 KB
PASSED: [[SimpleTest]]: [MySQL] 58,687 pass(es).
[ View ]

Uploading this one as well, let both finish please.

Both should pass, but we can decide which to use.

Removing the actual concept of a 'page callback' and all of its code is for another issue, since we still have legacy local tasks, actions, and contextual links to finish.

Issue tags:+8.x-alpha4

This is a patch I'd love to get in before alpha4.

Per comment #1, I favor patch #8. That is, keep ChainRouter in place for easier flexibility and extensibility. Even if we find later that it has an unwanted performance overhead (which I highly doubt), it could be taken out of the container configuration trivially with no API changes.

Thus, I am RTBCing #8.

TIM, YOU'RE AWESOME!

Status:Needs review» Reviewed & tested by the community

I DID change the status, I swear I did...

is getting the docblocks right a gate? Because this patch removes the old stuff, but I'm not sure if all the docblocks have been updated for this change.

is getting the docblocks right a gate? Because this patch removes the old stuff, but I'm not sure if all the docblocks have been updated for this change.

StatusFileSize
new8.45 KB
new26.07 KB
PASSED: [[SimpleTest]]: [MySQL] 58,788 pass(es).
[ View ]

Sorry about OTC, but where could I dowload 8.x-alpha4?

You can't yet. :) It comes out end of next week, hopefully.

Thanks, I hope also! :-)

If we do want to commit this like this, then we should at least have a follow-up issue to clean up the documentation, there are still 59 (not counting the one in CHANGELOG.txt) mentiones of "page callback" left. Lot's of those are pages.inc @file docblocks, but still.

Also, this still seems to exist:

<?php
   
// If the menu item references a route, normalize the route information
    // into the old structure. Note that routes are keyed by name, not path,
    // so the path of the route takes precedence.
   
if (isset($router_item['route_name'])) {
     
$router_item['page callback'] = 'USES_ROUTE';
     
$router_item['access callback'] = TRUE;
     
$new_path = _menu_router_translate_route($router_item['route_name']);
      unset(
$callbacks[$path]);
     
$callbacks[$new_path] = $router_item;
    }
?>

But that constant shouldn't exist anymore according to the issue summary?

I'm working through the menu_router_build() stuff in #2107039: Make remaining hook_menu declarations match route paths.
There are isset($item['page callback']) checks still needed for local tasks, local actions, contextual links, and theme callback. Yet the value is no longer used.

So as I understand it, we have both docs references to page callback, and things like theme callback that rely on it being set, but not actually the value.

In that case I'm OK committing this as is to remove it, and another, critical follow-up to update docs once the code references are gone. Does that sound OK?

While this means the old router won't be consulted, we still have a tonne of code writing to it etc. don't we? i.e. I don't see anything removing the table from schema here.

Title:Remove legacy routerRemove legacy router backwards compatibility layer
Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

This could probably use a g.d.o/core announcement since it's finally going to break hook_menu() for routes.

Title:Remove legacy router backwards compatibility layerChange notice: Remove legacy router backwards compatibility layer
Priority:Critical» Major
Status:Fixed» Active
Issue tags:+Needs change record

We probably need to do a few things change notice-wise here:

  1. Add a new change notification for this change that we will update for #2107533: Remove {menu_router} as well. Could be as simple as "Foo removed from hook_menu(). Use the new routing system instead."
  2. Check to see if the main routing change notice or any others need an update. Update https://drupal.org/node/1800686 if needed.
  3. Make sure this issue is referenced on all the appropriate change notifications.
  4. Restore this issue's status/priority/tags once that's done. :P

Oh, and I said this in IRC, but I'll make sure this goes in tomorrow's "This week in D8". Yay! :)

Issue tags:+Needs followup

@catch retitled https://drupal.org/node/1800686 sensibly so I think that covers #1 for now. Can someone check for #2 and then we can close this? Edit: This at least is a lie, so please read closely:

Also note that for now, the old routing system remains completely functional, until full conversion of routes to the new routing system has been finished.

Also, the hook_menu() API documentation is egregiously misleading. While I look forward to the fairies and ponies from #2046367: Document the internals of the router and #2107533: Remove {menu_router}, can we please file a small docs patch that adds a short paragraph indicating that much of the hook is deprecated and use the new routing system? It doesn't need to be perfect; it just needs to point out that most of the wall of text following it is a lie.

I had a very bad DX last week when the settings menu item has disappeared in my module and than I found later that I was out of sync with hook_menu. Once I got them in sync with the route.yml the menu item re-appeared. https://drupal.org/node/1800686 does not list the cases for 'description' and the other open @todo's.

Can someone fill this article with the missing issue links, please? I'm fairly lost in finding them myself.

Did the patch for this issue not update the hook_menu docs? Wouldn't that be part of the docs gate for a patch that totally changes what hook_menu() is for, or better yet several iterations ago when you had to start using routing.yml files in the first place? This practice of ripping things in/out of core without updating the docs is not helpful. Just saying... :((((((

or better yet several iterations ago when you had to start using routing.yml files in the first place?

That's mainly why I didn't hold the patch up. This doesn't completely remove hook_menu() or change what's required there, nor does it completely remove the router - it just removes one part of the backwards compatibility layer for it. That the docs are out of sync is bad, but it's not this issue's fault.

#2044969-36: Break the old router for contrib contains some updates to hook_menu() docs. Perhaps we can start by extracting that into a docs-only patch? Should we open a new issue for that?

There is already another issue:
#2046367: Document the internals of the router

Shouldn't this issue get properly reclassified as a meta issue, with appropriate title? If not, we need a new meta it sounds like...

Assigned:tim.plunkett» Unassigned
Issue summary:View changes

Issue tags:+API clean-up, +@deprecated

Issue tags:+Missing change record

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

Title:Change notice: Remove legacy router backwards compatibility layerChange notice: Remove legacy router backward compatibility layer

Status:Active» Fixed

#2107533: Remove {menu_router} together with [#1800686] should really cover it properly.

Title:Change notice: Remove legacy router backward compatibility layerRemove legacy router backward compatibility layer
Priority:Major» Critical

Restoring title/priority

Issue tags:-Missing change record

Issue tags:-Needs change record

This issue just doesn't want to give up the needs change record tag!

Removed 'Needs change record' again.

Status:Fixed» Closed (fixed)

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