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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

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!!!

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
17.83 KB

Let er rip

cosmicdreams’s picture

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.05 KB

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.

tim.plunkett’s picture

FileSize
18.18 KB

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

tim.plunkett’s picture

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.

webchick’s picture

Issue tags: +8.x-alpha4

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

Crell’s picture

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!

Crell’s picture

Status: Needs review » Reviewed & tested by the community

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

cosmicdreams’s picture

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.

cosmicdreams’s picture

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.

tim.plunkett’s picture

mpark’s picture

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

webchick’s picture

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

mpark’s picture

Thanks, I hope also! :-)

Berdir’s picture

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:

    // 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?

tim.plunkett’s picture

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.

catch’s picture

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.

tim.plunkett’s picture

catch’s picture

Title: Remove legacy router » Remove 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.

xjm’s picture

Title: Remove legacy router backwards compatibility layer » Change 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
xjm’s picture

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

xjm’s picture

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.

hass’s picture

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.

jhodgdon’s picture

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... :((((((

catch’s picture

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.

effulgentsia’s picture

#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?

jhodgdon’s picture

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

Crell’s picture

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

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Issue summary: View changes
sun’s picture

Issue tags: +API clean-up, +@deprecated
xjm’s picture

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

Damien Tournoud’s picture

Title: Change notice: Remove legacy router backwards compatibility layer » Change notice: Remove legacy router backward compatibility layer
dawehner’s picture

Status: Active » Fixed

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

ianthomas_uk’s picture

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

Restoring title/priority

xjm’s picture

Issue tags: -Missing change record
jessebeach’s picture

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.