Break the old router for contrib

Be sure to refer to the modules enable disable test, it already has logic for identifying core modules

why not

tests only and fix

+++ b/core/includes/menu.incundefined
@@ -2753,7 +2753,15 @@ function menu_router_build($save = FALSE) {
+    if (strpos($module_details->uri, 'core/modules') !== 0) {

This does not work, because it will break things like contextual links, local actions and all this other subsystems which use the menu_router internally. We have to get rid of them in menu.inc first before we can do that. :( I guess we have to write a test for that as well.

A different idea might be the following: In LecacyUrlMatcher::matchDrupalItem look at the "include_file" property and see which module this belongs to. Putting it into menu_get_item() does not work,
because this also checks access for some of the subsystems mentioned above.

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterTest.phpundefined
index b3d9bbc..6ce9848 100644
--- a/core/profiles/testing/modules/drupal_system_listing_compatible_test/drupal_system_listing_compatible_test.module

There is already a .info.yml file existing here, so that is fine.

#2046367: Document the internals of the router filed.

Touche!

This is a new approach directly acting on the legacy route integration. assertLinkByHref, i really love your second parameter!

New critical bug found while creating this issue: #2046385: _menu_navigation_links_rebuild fails when menu_link module is not enabled.

Ew. Really? I'm completely against this idea. We cannot ship Drupal 8 in any way shape or form without converting core to its own new subsystems first.

It's going to be ready when it's ready. Arbitrary deadlines and hacks are not going to do us any good.

The idea is to force contrib module developers to use the route system now!

Well. If we start doing this, there is no guarantee that we are not going to later decide to ship with the old router still in there. This is just a very slippery slope.

I disagree, as this patch solves an actual problem and nothing will stop us from working on https://drupal.org/project/issues/search/drupal?issue_tags=WSCCI-conversion

This is 100% about allowing us to fix the routing in core without tripping up contrib developers who start porting there modules now. We won't ship with a unfinished routing system, there is no way we can or would do that. However, people are starting to convert there modules now. And since we are not finished with the conversion of all of our hook_menu entries in core yet this is the only way to solve the problem.

A core-specific BC-layer if you will.

100% +1 on this.

We obviously need a critical follow-up for removing this before we ship...

Agree with @fubhy in #13 plus some @todo comments in code.

Mh, the problem is that drupal_get_path() does not really return the path in core/profiles mh...

Well, is it a good idea to force-feed an API to contrib developers that we think is so hard to use we cannot even convert core to it in a timely manner?

The point is not that it is hard to use, the point is that it needs time as we should reorganize certain pieces of the code and at the same time rewrite entire related subsystems.

Well, if the API is not complete yet, why force-feed it on contrib developers? Really, core is the best playground we have to test the robustness, ease of use and completeness of an API. We should complete core, fix the issues that pops up (and they will), and then invite contrib to use it.

Most of these API changes are backwards compatible, so it does not affect contrib developers if they don't want to.

Believe me, I have complained about the router as much as anyone can ( I stopped short of adding a comment to every single router issue asking for the rollback of it although I did consider it ) but it is crystal clear that, despite better judgement, the router will not be removed and catch stated he will not ship Drupal 8 with two menu systems. There are extremely few people who can overrule catch (officially one, in real world, alas, two) and I have not heard either of them talking against the router.

So, the (broken) router will stay and the (working) menu system will be removed for no technical reason aside from the insane notion that everything the Drupal contributors wrote during the last decade (and got battle tested on countless sites) must be inferior to the Symfony code (plus the un-battletested integration code the Drupal people added...) and that's a fact and there's no use to cry any more over it (I won't stop though Edit: Rather, I stopped commenting on the router but also everything else, I am out of Drupal 8 aside from finishing my ongoing issues. ) and so contrib better gets used to this.

Given the facts above, this patch is a go and I wish it weren't.

It is also about menu links, local actions and local tasks and I really believe that modules are really hard to use without it. This would discourage people from porting modules.

I just created #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit which I think is potentially related to or overlaps this issue.

Also, trying to put a more positive spin on what chx comments - if we have a commitment to fully convert D8 to use routes, then there is still a huge amount of refactoring to do, new APIs to write, and cleanup and testing. We need a vision of and commitment to shipping Drupal 8 with a coherent menu and routing API, and not half broken and half converted.

So this is reverting back to approach at #3 but instead of ignoring all hook_menu entries for non core, we unset those with a 'page callback' and then log a warning to watchdog.
Expanded the test to also include a menu-link entry in hook_menu and ensure that it works.
interdiff from #3

Can we use moduleList instead of system_rebuild_module_data ? If I remember correctly it contains the filename you need as value.

@@ -2775,6 +2777,16 @@ function menu_router_build($save = FALSE) {
+      // This is a non-core module, legacy hook_menu() page callbacks are not
+      // supported.
+      watchdog('system', 'Callback for %path from module %module uses a legacy page callback and has been ignored', array(
+        '%path' => $path,
+        '%module' => $router_item['module'],
+      ), WATCHDOG_WARNING);
+      unset($callbacks[$path]);

This really reminds me on #2051877: Log error when people use invalid route parameters ... so maybe we should throw also an exception?

I don't really understand why we have to force this on contrib developers now, when as far as I know this hasn't been done with any other API; surely it's better to first spend effort documenting the routing system, fixing outdated hook_menu() API docs, etc, so contrib developers can start figuring out how they might upgrade?

https://api.drupal.org/api/drupal/core!modules!system!system.api.php/fun... still mentions 'page callback' and many other keys that are deprecated, and will deliberately be broken by this patch. What happened to the documentation gate?

So we'll delete those documented keys in this patch as well.

As a contrib developer, if this gets in, I guess I can just revert this patch or comment out the unset() line temporarily - that way I can upgrade my page callbacks one by one rather than all in one shot, at least until the old router is removed entirely.

We should certainly make a big public announcement before the patch gets in.

We should push to get the menu links hook ready also?

I'd rather not have this invoked at all on contrib modules

Yeah I'd prefer to keep it, ie drop the unset
And add a Drupal set message
Also drop the legacy hook menu docs

Then later on drop the entry by adding back the unset

Ie a staged approach

Or perhaps we make it a settings option?
That way we can turn it off during porting a module?
We retain the watchdog and add a drupal_set_message() too so its really obvious.
We call the settings 'legacy_routing_debug' and default to 0.

This updates the hook_menu() doco, adds a 'legacy_route_debug' setting support so we can disable it while porting our contrib modules. Also fixes as per #26.
Note I'm getting issues with drupal_system_listing_compatible_test showing up in system/test/modules instead of the profile in ->getModuleList(), but I think that's because I installed with drush instead of the UI.

So indeed module handler stores the wrong file path when a profile provides a module

Not clear at all why this is useful. The old router has to go like any other critical API change, when it goes, it goes, if you didn't port properly by then, you'll have to port when it's gone.

Otherwise "I didn't change anything and it still worked, so I'll just leave it... WAIT, why did that break now???" is going to be a common pattern. Not everyone porting to D8 is going to be reading all change notices; they'll just look up what they need to when their module doesn't work.

This, very much. I've been keeping Honeypot working with head since just before the first alpha release (when I remember the call to contrib developers was put out—"start porting your modules to find API flaws!").

It's been frustrating (but understandable, since we're not in API freeze) to be getting tests and functionality broken every few weeks, but what's more frustrating is when something that was either just marked deprecated or didn't have any notes at all is completely removed one day, and I need to fish around the issue queues to find the problem.

For something as foundational as the menu router, we need to do this asap; non-core-tracking contrib developers will start porting modules soon, and I don't think they'll care to use the new router system if they aren't forced. And they'll get doubly frustrated if/when the old system is actually disabled, since 99% of them aren't following this issue.

I've been a big supporter of this issue for a long time, way before @Crell filed it. IIRC, I heard the idea first from @EclipseGc prior to July 1, and have recommended it to Dries and Crell every chance I had. Primarily because https://drupal.org/core/release-cycle#polish-phase says:

Beta releases have a relatively stable API, and we recommend that all module developers begin porting their modules once a beta release is available.

How soon beta is, what the definition of "relatively stable" is, and whether this sentence will end up getting updated, is all something the core maintainers are trying to figure out right now, and will communicate well ahead of releasing the first beta. Regardless of how the details of that turn out, I think our highest priority right now should be fixing the APIs that are the least stable, so that we can get to a "relatively stable" state as soon as possible. Since we know the old router is 100% unstable (in the sense of being guaranteed to be removed by 8.0 release), and that instability affects ~100% of contrib modules, IMO, that means we need to either:

• Finish converting all of core to the new routing system, and remove the old one entirely, or
• Remove the old router from the API as far as contrib modules are concerned (either by breaking it as this issue is suggesting, emitting a E_DEPRECATED warning, or whatever else we consider to be sufficiently noticeable).

And that one of those needs to happen before we can say that our APIs are even "relatively" stable.

My preference for the 2nd of those options has been based on two assumptions:

• That fixing all the remaining controller conversions will take many weeks.
• That those conversions are mostly bogged down by an attempt to make each new controller as OO and DI pure as possible, and not by anything significant missing from the new routing system itself.

However, as alluded to in #19, that 2nd assumption isn't true. For example, #1938318: Convert book_remove_form to a new-style Form object is stuck on the lack of an access checker equivalent to node_access() that is compatible with the new routing system. Breaking the old router for contrib would therefore mean breaking the ability for contrib modules to have routes with a {node} parameter that needs to be access checked.

Given that, my suggestion for how to proceed is the following:

1. Identify all WSCCI-conversion tagged issues that, like #1938318: Convert book_remove_form to a new-style Form object, require a change or addition to the new routing system, and raise their priority to critical: all WSCCI-conversion issues are in fact critical, but we've resisted raising all of their priorities in order to not overwhelm the critical queue, but I think that ones that reflect something incomplete about the routing system (including things like common access checkers not yet migrated from the old routing system) warrant the greater visibility.
2. For all of the conversion issues, be less nit-picky about how perfect (in terms of dependency injection and use of globals/superglobals) the new controllers are in the initial patch, and just focus on getting them using the new routing system. Then file a follow up to improve the controllers beyond that.
3. After all of the "critical" (per first bullet point) ones are done, evaluate the list of remaining ones, and see if it's gotten small enough that we can wrap them up quickly, or if we want to proceed with this issue of breaking the old router for contrib while still wrapping up the stragglers in core.

Thoughts?

My only quibble with #42.1 is I'd prefer the critical spun up to be a separate "Add equivalent to node_access for the routing system" (or whatever) and postpone the relevant issues on that, rather than having some "Convert foo_bar() to routing system" issues at "normal" and some at "critical." Because:

a) That makes it extremely clear what the actual release-blocking issue is, without having to read 60+ replies to figure it out.
b) It allows us to attract attention of the architect/core generalist/core maintainer folks more quickly and directly (I'm guessing they are not out there keeping track of all 3200 conversion issues, at least before they're RTBC, or at least I know I'm not. :))
c) It doesn't confuse some well-meaning issue queue farmer who then says "oh, look, these few issues are inconsistently prioritized" and cause a nuisance back-and-forth of prioritization in just trying to help.

Elsewise, your list sounds good to me!

Yes I think spinning off API changes or additions into their own issue is good. IMO it's not reasonable to do this issue at all until those issues are fixed, it's setting up people to have to convert twice, or start converting then get blocked on known issues.

I bumped #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system to critical, along with #1947880: Replace node_access() by $entity->access(). Postponing this per #42.

I strongly think we should move links out per #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit as part of this solution. It gives us a chance to have an understandable API.

#2106709: Remove legacy router backward compatibility layer