Remove all implementations of hook_menu

Let's continue with this now given that it allows us to
figure out how much we already can remove from menu.inc

Let's continue with this now given that it allows us to
figure out how much we already can remove from menu.inc

This is the first round.

Removed the changes to the ajax system.

Advertisment: #1959574: Remove the deprecated Drupal 7 Ajax API

#2095959: Remove instances of menu_get_object('node') would probably also fix a couple of failures ...

This fixes at least the one for custom_block.

I checked BookTest locally, and is fixed by #2095959: Remove instances of menu_get_object('node').
That explains a lot of the failures, since node_is_page() is broken.

This should fix most of the issues with the admin index page.

+++ b/core/modules/comment/comment.module
@@ -214,12 +214,12 @@ function comment_menu_link_defaults() {
+function comment_menu_links_defaults_alter(&$links) {

fix this on the next patch. should be _menu_link_

I'd suspect that the language related fails are related to that fact that negotiation plugins still use pathes instead of routes, they all have an annotation like this: " * config_path = "admin/config/regional/language/detection/browser"".

I would expect that it should be fairly easy to convert that to a common configure route that gets the plugin id as argument and calls methods on those plugins, similar to how blocks work. Needs a separate issue.

I would expect that it should be fairly easy to convert that to a common configure route that gets the plugin id as argument and calls methods on those plugins, similar to how blocks work. Needs a separate issue.

Blocking this one then?

I'm not 100% sure that it is related, maybe we're just missing a route or some definition there, just thought that it might help.

Since this patch changes the last three implementations of hook_menu_alter() we could (should?) also change the hook calling code drupal_alter('menu', $callbacks); in menu.inc. Or will there be a followup of menu.inc changes?

@Sutharsan
In case you are aware of the content_translation_menu_link_alter I would be happy to get help.

@herom
Fixed your points.

Applying the menu_get_object() patch fixes the following tests:

• book functionality
• node blocks
• statistics logging tests

menu.path_roots needed to be ported to the new routing system using an event subscriber.

#2081963: Shortcut add/remove link on titles don't work on route only pages fixes the shortcut failures.

/me passes the ball to @berdir

You were probably looking for a way more complicated reason why that the language test sf failing I think, it's quite simple actually. The reason is because interface translation for menu link descriptions doesn't work anymore.

The test uses the description of the languages menu link ("Configure languages for content and the user interface.") as a translatable string and proof that language negotiation and translation works.

The test can be fixed by picking a string that can actually can be translated. "Hide descriptions" seems to work.

There is however, a bigger problem here, and that's those translations. I know you kind of count on menu link translation for this using entity translation, but Core currently has no concept at all to handle this, track them on localize.drupal.org and provide translations out of the box. I don't know how that can possibly work?

Also, I'm seeing a weird random fail in the language negotation test related a language switch block on line 402. sometimes the language switch block is there and sometimes not, no idea why. It's either related to the environment/php version of my local system, because I've never seen that on the testbot or this patch. Looking at the recent test fails, there are always 12 fails, while I have 14 fails locally when that breaks. So I guess it's my PHP version?

And yay that the Path alias caching test still exists and finally started to fail, was a huge task to get that thing in core and I was worried that we'll break that somewhere without noticing :) Tests++

There is however, a bigger problem here, and that's those translations. I know you kind of count on menu link translation for this using entity translation, but Core currently has no concept at all to handle this, track them on localize.drupal.org and provide translations out of the box. I don't know how that can possibly work?

I would assume that you would parse 'link_title' from hook_menu_links_default similar as it worked with hook_menu()?

Wow this was a ride, this fix is probably the best workaround for now.

I also added a test coverage for the uninstall menu problem.

Sure, but then? hook_menu() was simple t() strings, through the existing API, so localize.drupal.org just worked. This would somehow need to be stored separately, for example based on the machine name or also the source text, and then downloaded and translations of all menu links need to be created? That doesn't exist for anything else right now...

Looks like the testbot now has the same failure as I did locally for Drupal\language\Tests\LanguageUILanguageNegotiationTest? Weird.

26: menu_links-2177041-23.patch queued for re-testing.

jibran opened a new pull request for this issue.

Reroll of #26. Please delete/unpublish #34 as I don't want to make a mess in critical issue.

Please just don't use this github sync if it does not work yet for core development, that is my opinion.

@Berdir: my understanding the description translation should not have been broken (yet?). The patch deliberately left in runtime translations of titles too. Eventually, menu links have the same problem as shipped config though. They have shipped versions and then may change based on admin UI actions. Their shipped versions need to be translatable and updateable with locale module in core and localize.drupal.org while once they change their values, the changed values should be left intact. Configuration has a parallel system developed for this to maintain the config translations across locale and the config system identically and cross-sync them. For this same system to work for menu items, we'd need to reimplement the same locale cross-syncing solution for content entities. Looks like otherwise we are up to some serious regressions (ie. not possible to pre-translate the admin menu even on localize.drupal.org) which is release blocking at least :)

I've been saying there is a pretty huge gap in this area for months to get Drupal 8 code leads appreciate how much work is still needed here. Plenty.

Opened #2193777: Menu link translations need to work with content translation, shipped translations need to be compatible trying to explain this whole stuff. Fun, eh?

Let's make it green, though we still have to figure out content_translation_menu()

.

40: menu_links-2177041-40.patch queued for re-testing.

Fixed the failures and some whitespace.

44: menu_links-2177041-44.patch queued for re-testing.

Why would the _translate code ever be called on something that's not a menu link entity? If it's from book module, we should consider this patch blocked on #2084421: Phase 2 - Decouple book module schema from menu links

Well, yes, book module on the longrun also calls this method, though can't we be pragmatic for this single line, given that we need to rethink translation/localization support for menu links anyway?
There are quite some functions still calling we could cleanup after #2177031: Remove menu_get_item() and every use of it. is in.

1. +++ b/core/includes/menu.inc
   @@ -633,7 +634,9 @@ function _menu_check_access(&$item, $map) {
   +  // @todo Figure out a proper way to support translations of menu links.
   

   Is there an issue for that?

2. +++ b/core/includes/menu.inc
   @@ -652,6 +655,7 @@ function _menu_item_localize(&$item, $map, $link_translate = FALSE) {
   +    $item['title'] = isset($item['link_title']) ? $item['link_title'] : $item['title'];
   

   I would find this easier to read if it was only changed if link_title is set, and not use the ternary. This is all personal preference though!

3. +++ b/core/lib/Drupal/Core/EventSubscriber/PathRootsSubscriber.php
   @@ -0,0 +1,78 @@
   + * Provides all available first bits of all route paths.
   

   Sounds weird.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/PathRootsSubscriber.php
   @@ -0,0 +1,78 @@
   +   * Stores the path roots available in the router.
   +   *
   +   * A path root is the first virtual directory of a path, like 'admin', 'node'
   +   * or 'user'.
   

   Needs an @var too.

5. +++ b/core/lib/Drupal/Core/EventSubscriber/PathRootsSubscriber.php
   @@ -0,0 +1,78 @@
   +    $events[RoutingEvents::ALTER][] = array('onRouteAlter', -1024);
   

   Do you think we should add a comment for the low priority?

6. +++ b/core/modules/comment/comment.module
   @@ -178,35 +178,18 @@ function comment_theme() {
   +    'parent' => \Drupal::moduleHandler()->moduleExists('node') ? 'node.admin.content' : 'system.admin',
   

   This seems pretty fragile, not sure what we can do about that right now though.

7. +++ b/core/modules/locale/lib/Drupal/locale/StringStorageException.php
   @@ -6,8 +6,14 @@
   +    parent::__construct($message, $code, $previous); // TODO: Change the autogenerated stub
   

   Whoops. Also, is this constructor even needed?

8. +++ b/core/tests/Drupal/Tests/Core/EventSubscriber/PathRootsSubscriberTest.php
   @@ -0,0 +1,86 @@
   + * Tests Drupal\Core\EventSubscriber\PathRootsSubscriber.
   

   @group too.

@damiankloip: the menu translation issue is at #2193777: Menu link translations need to work with content translation, shipped translations need to be compatible. That should be linked from there if that todo is related to what I think it is :)

Seems like the right fit, yes :)

I would find this easier to read if it was only changed if link_title is set, and not use the ternary. This is all personal preference though!

The goal was to minimize the needed changes, given that 'title' appears allover the function.

Sounds weird.

Any suggestion? It is what this thing is doing, and we have the rule to fit in 80 damn chars.

This seems pretty fragile, not sure what we can do about that right now though.

I copied the approach from another issue. Maybe we should really have admin/content as part of system module, but this is kind of out of scope of this issue. More 'of's

Whoops. Also, is this constructor even needed?

Nope, this was just debugging.

@group too.

Good catch!

Only two minor issues we can ignore these as well. Patch looks perfect. RTBC for me but I'll let @damiankloip do the honour.

1. +++ b/core/includes/menu.inc
   @@ -633,7 +634,9 @@ function _menu_check_access(&$item, $map) {
   +  // @todo Figure out a proper way to support translations of menu links.
   

   Please add link to https://drupal.org/node/2193777

2. +++ b/core/includes/menu.inc
   @@ -633,7 +634,9 @@ function _menu_check_access(&$item, $map) {
   +  $title_callback = $item instanceof MenuLinkInterface && !$item->customized ? 't' :  $item['title_callback'];
   

   I think we should convert it to if then else and replace t with \Drupal::t.

I think we should convert it to if then else and replace t with \Drupal::t.

Well, we don't have a \Drupal::t() for a reason.

Seriously this TODO should have not blocked the patch.

Added a small todo.

The sooner this patch makes it in, the better. Unfortunately we now ship with two ways of doing menu links which is very confusing to new D8 developers. Tagging as an alpha target.

55: menu_links-2177041-55.patch queued for re-testing.

Rerolled.

It is green again. It was RTBC in #52 so changing it to RTBC. Thanks @dawehner.

+++ b/core/lib/Drupal/Core/EventSubscriber/PathRootsSubscriber.php
@@ -0,0 +1,81 @@
+    // Try to set a low priority to ensure that all routes are already added.

This is a regression from the current code (that's not removed by the patch?) that guarantees all routes are there before setting anything. At a minimum needs a critical follow-up - for example could we make the routes available to the onRouteFinished event?

We do have one issue which tackles that particular problem: #2158571: Routes added in RouteSubscribers cannot be altered though sadly it got unmotivated.

I bumped #2158571: Routes added in RouteSubscribers cannot be altered to critical.

Bumping that to critical works great. Committed/pushed to 8.x, thanks!

https://drupal.org/node/2184797 covers this for the change notice.

Awesome!

Updated [#2184797] with a reference to this issue.

This removed part but not all of content_translation_menu_alter(). Should that not have been renamed to content_translation_menu_link_defaults_alter()? Alex Pott opened #2210077: Remove content_translation_menu_alter as hook_menu_alter no longer exists.

Sorry to re-open this old issue, but there is no change record about what happened to hook_menu_alter(), and the hook_menu_links_defaults_alter() that this patch switched things to no longer exists either. The change notice at https://drupal.org/node/2184797 that catch links to in #64 is a 404.

Halp? :)

Feel free to adapt the existing change records and open actual follow ups.

• https://www.drupal.org/node/2044515
• https://www.drupal.org/node/2007444
• https://www.drupal.org/node/2187643
• https://www.drupal.org/node/2165243

Well, if I were capable of writing the change notice myself, I would've just done it. :) I literally have no idea how it should be done, hence re-opening the issue so someone who worked on this can do so.

https://www.drupal.org/node/2187643 seems like the closest but a) it's a handbook page, so doesn't show up in https://www.drupal.org/list-changes/published?keywords_description=menu_... and b) it also doesn't have a D7 'before' example showing what gets converted to what (which is fine, since it's a D8 documentation page, but we need that before/after for the change notices).

https://www.drupal.org/node/2118147 is the documentation page that has D7 before and D8 after examples with all kinds of things. I think it makes more sense to keep this as a doc page. That is prominently linked from https://www.drupal.org/node/1800686 which is THE main menu change record. I just added this issue to that change record as well. Good?

Unfortunately not, because cmd+F for "menu_alter" on that page returns nothing (awesome page, though!!), and neither does searching for it on the change records list.

For a start, changed the page title to "D7 to D8 upgrade tutorial: Convert hook_menu() and hook_menu_alter() to Drupal 8 APIs" and added this section:

Replacements for hook_menu_alter()

If you used hook_menu_alter() (incorrectly) to define dynamic items, see above for dynamic routes, actions, etc. Altering existing items in the new systems is different based on what you are to alter.

• To alter routes, implement a RouteSubscriber
• To alter menu links, use hook_menu_links_discovered_alter()
• To alter tasks, use a hook_menu_local_tasks_alter()
• To alter contextual links, use a hook_contextual_links_view_alter()
• To alter action links, use a hook_menu_local_actions_alter()

I know that is far from before/after examples, but at least a start.

Great. Also added this blurb to the main change notice at https://www.drupal.org/node/1800686:

"hook_menu_alter() is no more, and instead becomes a number of possible extension mechanisms which are enumerated in Replacements for hook_menu_alter(). (links to your new heading)"

So now at least "hook_menu_alter" will bring something up in change notice searches.

I think that's all we need here, so closing this back down. Thanks!