backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Menu item vs Menu router #226

Open kriboogh opened 10 years ago

kriboogh commented 10 years ago

Quickly looking through the menu.inc file in core/includes there seems to be a mix of router / menu things going on. Would it be an idea to refector menu.inc into router.inc doing only router stuff and moving all menu related stuff (theming for example) in the actual menu.module. The database tables menu_router then becomes router_routes and menu module provides the menu_links and menu_custom tables.

yann-yinn commented 10 years ago

That would be quite an elegant move... hook_routes() and hook_menu() should be two separate concerns... (But this is also some kind of major changes concerning the API ). I always loved hook_menu for routing (especially since D6), always hated menu_local_tasks and stuff like that in the middle of it.

When i explain Drupal to a new developper, this is also almost always something disturbing for them; because they first want implements a hook_menu to create a menu item and then ask how to create a "route"...

rudiedirkx commented 10 years ago

I think separating menu and router logic is good, but not creating separate hooks. It's also so huge a change that it's not really feasible probably. Menu and router are so very intertwined and complex in D7.

quicksketch commented 10 years ago

Also slightly related: Move menu blocks into menu module (out of system.module).

Menu items and routing have been hopelessly intertwined in D7. Separating them would indeed be a worthy goal. However, like @rudiedirkx said, the feasibility of changing this in Backdrop 1.0 is probably low, since other than feature backporting, one of our primary goals for 1.0 is compatibility. The major shift required in simply renaming the project already makes this challenging, but for our initial release we need to smooth the process as much as possible.

kriboogh commented 10 years ago

It is indeed a big chunk to be refactored (spend some time in the code yesterday, started on issue #225). So this is something for the Backdrop 2.x track then.

jenlampton commented 10 years ago

Sounds reasonable, tagging as such