Follow-up from/blocked by #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit.

Once we have a new way to define menu links committed, remove all the implementations of hook_menu from core modules.

Files: 
CommentFileSizeAuthor
#59 hook_menu-2177041-59.patch63.74 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 64,715 pass(es).
[ View ]
#55 interdiff.txt735 bytesdawehner
#55 menu_links-2177041-55.patch67.35 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links-2177041-55.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#53 interdiff.txt773 bytesdawehner
#53 menu_links-2177041-53.patch67.28 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 64,509 pass(es).
[ View ]
#51 interdiff.txt2.22 KBdawehner
#51 menu_links-2177041-51.patch67.23 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 64,491 pass(es).
[ View ]
#44 interdiff.txt1.45 KBdawehner
#44 menu_links-2177041-44.patch67.83 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 64,366 pass(es).
[ View ]
#40 menu_links-2177041-40.patch67.76 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 64,414 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#40 interdiff.txt1.11 KBdawehner
#35 menu_links-2177041-34.patch67.95 KBjibran
FAILED: [[SimpleTest]]: [MySQL] 64,236 pass(es), 5 fail(s), and 2 exception(s).
[ View ]
2177041.patch68.06 KBGithub sync
FAILED: [[SimpleTest]]: [MySQL] 64,228 pass(es), 5 fail(s), and 2 exception(s).
[ View ]
#26 menu_links-2177041-23.patch82.79 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links-2177041-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#26 interdiff.txt4.69 KBdawehner
#22 menu_links-2177041-22-interdiff.txt832 bytesBerdir
#22 menu_links-2177041-22.patch79.96 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 63,615 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#21 menu_links-2177041-21.patch79.19 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 63,689 pass(es), 17 fail(s), and 0 exception(s).
[ View ]
#21 interdiff.txt10.41 KBdawehner
#19 interdiff.txt1.46 KBdawehner
#19 menu_links-2177041.patch54.14 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 63,346 pass(es), 45 fail(s), and 2 exception(s).
[ View ]
#12 menu_links-2177041.patch52.88 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 63,652 pass(es), 45 fail(s), and 2 exception(s).
[ View ]
#12 interdiff.txt3.35 KBdawehner
#9 menu-2177041-9.patch49.52 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 63,635 pass(es), 49 fail(s), and 2 exception(s).
[ View ]
#9 interdiff.txt2.28 KBdawehner
#5 menu-2177041-5.patch47.24 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 63,525 pass(es), 67 fail(s), and 5 exception(s).
[ View ]
#3 hook_menu-2177041.patch85.49 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 63,250 pass(es), 79 fail(s), and 5 exception(s).
[ View ]

Comments

dawehner’s picture

Assigned:Unassigned» dawehner

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

dawehner’s picture

Assigned:Unassigned» dawehner

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

dawehner’s picture

Status:Active» Needs review
StatusFileSize
new85.49 KB
FAILED: [[SimpleTest]]: [MySQL] 63,250 pass(es), 79 fail(s), and 5 exception(s).
[ View ]

This is the first round.

Status:Needs review» Needs work

The last submitted patch, 3: hook_menu-2177041.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new47.24 KB
FAILED: [[SimpleTest]]: [MySQL] 63,525 pass(es), 67 fail(s), and 5 exception(s).
[ View ]

Removed the changes to the ajax system.

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

Status:Needs review» Needs work

The last submitted patch, 5: menu-2177041-5.patch, failed testing.

The last submitted patch, 5: menu-2177041-5.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

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

dawehner’s picture

StatusFileSize
new2.28 KB
new49.52 KB
FAILED: [[SimpleTest]]: [MySQL] 63,635 pass(es), 49 fail(s), and 2 exception(s).
[ View ]

This fixes at least the one for custom_block.

Status:Needs review» Needs work

The last submitted patch, 9: menu-2177041-9.patch, failed testing.

tim.plunkett’s picture

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.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new3.35 KB
new52.88 KB
FAILED: [[SimpleTest]]: [MySQL] 63,652 pass(es), 45 fail(s), and 2 exception(s).
[ View ]

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

herom’s picture

+++ 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_

Status:Needs review» Needs work

The last submitted patch, 12: menu_links-2177041.patch, failed testing.

Berdir’s picture

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.

xjm’s picture

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?

Berdir’s picture

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.

Sutharsan’s picture

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?

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new54.14 KB
FAILED: [[SimpleTest]]: [MySQL] 63,346 pass(es), 45 fail(s), and 2 exception(s).
[ View ]
new1.46 KB

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

@herom
Fixed your points.

Status:Needs review» Needs work

The last submitted patch, 19: menu_links-2177041.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new10.41 KB
new79.19 KB
FAILED: [[SimpleTest]]: [MySQL] 63,689 pass(es), 17 fail(s), and 0 exception(s).
[ View ]

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

Berdir’s picture

StatusFileSize
new79.96 KB
FAILED: [[SimpleTest]]: [MySQL] 63,615 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
new832 bytes

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?

Berdir’s picture

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

The last submitted patch, 21: menu_links-2177041-21.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 22: menu_links-2177041-22.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new4.69 KB
new82.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links-2177041-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

Berdir’s picture

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

Status:Needs review» Needs work

The last submitted patch, 26: menu_links-2177041-23.patch, failed testing.

The last submitted patch, 26: menu_links-2177041-23.patch, failed testing.

Berdir’s picture

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

dawehner’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 26: menu_links-2177041-23.patch, failed testing.

The last submitted patch, 26: menu_links-2177041-23.patch, failed testing.

jibran’s picture

StatusFileSize
new67.95 KB
FAILED: [[SimpleTest]]: [MySQL] 64,236 pass(es), 5 fail(s), and 2 exception(s).
[ View ]

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

The last submitted patch, 34: 2177041.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 35: menu_links-2177041-34.patch, failed testing.

dawehner’s picture

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

Gábor Hojtsy’s picture

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

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new1.11 KB
new67.76 KB
FAILED: [[SimpleTest]]: [MySQL] 64,414 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

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

dawehner’s picture

Assigned:dawehner» Unassigned

.

dawehner’s picture

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

Status:Needs review» Needs work

The last submitted patch, 40: menu_links-2177041-40.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new67.83 KB
PASSED: [[SimpleTest]]: [MySQL] 64,366 pass(es).
[ View ]
new1.45 KB

Fixed the failures and some whitespace.

dawehner’s picture

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

pwolanin’s picture

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

dawehner’s picture

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.

damiankloip’s picture

  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.

Gábor Hojtsy’s picture

@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 :)

damiankloip’s picture

Seems like the right fit, yes :)

dawehner’s picture

StatusFileSize
new67.23 KB
PASSED: [[SimpleTest]]: [MySQL] 64,491 pass(es).
[ View ]
new2.22 KB

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!

jibran’s picture

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.

dawehner’s picture

StatusFileSize
new67.28 KB
PASSED: [[SimpleTest]]: [MySQL] 64,509 pass(es).
[ View ]
new773 bytes

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.

pwolanin’s picture

re: #47 - you know i'm happy to be pragmatic, but we should change that line and/or type-hint once book is de-coupled to maybe we need a todo in the code at least?

dawehner’s picture

StatusFileSize
new67.35 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links-2177041-55.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new735 bytes

Seriously this TODO should have not blocked the patch.

Added a small todo.

webchick’s picture

Issue tags:+alpha target

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.

Berdir’s picture

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

Status:Needs review» Needs work

The last submitted patch, 55: menu_links-2177041-55.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new63.74 KB
PASSED: [[SimpleTest]]: [MySQL] 64,715 pass(es).
[ View ]

Rerolled.

jibran’s picture

Status:Needs review» Reviewed & tested by the community

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

catch’s picture

Status:Reviewed & tested by the community» Needs review
+++ 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?

dawehner’s picture

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

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community
catch’s picture

Status:Reviewed & tested by the community» Fixed

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

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

xjm’s picture

Awesome!

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

Gábor Hojtsy’s picture

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.

Status:Fixed» Closed (fixed)

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

webchick’s picture

Status:Closed (fixed)» Needs work
Issue tags:-beta blocker, -alpha target+Needs change record

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? :)

dawehner’s picture

webchick’s picture

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).

Gábor Hojtsy’s picture

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?

webchick’s picture

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.

Gábor Hojtsy’s picture

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.

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

webchick’s picture

Status:Needs work» Closed (fixed)

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!