Remove menu_get_item() and every use of it.

core broke my module

yeah well. get used to it

Those quotes summarize #2185465: menu_get_item() does not work with hook_menu_link_defaults() pretty well.

What menu_get_item() does is try and load a menu item for a path. If it can't find an item for the given path, it removes the last path fragment and tries again.

However, we no longer deal with paths, but with routes, and menu links explicitly specify the machine names of their parent links, we no longer have an easy way to find the menu item closest to a given path, as we don't have a way to move up the tree anymore, and moving down the tree is too intensive.

As menu_get_object() relies on menu_get_item() we need to solve that meta issue first.

Postponing on #2091399: [META] Remove menu_get_object().

Note: This removes all calls which are obvious, but leaves the one in menu.inc, which are still part of the API workflow. These can and should be cleaned up without too many problems later.

1. +++ b/core/modules/block/custom_block/custom_block.local_actions.yml
   @@ -10,5 +10,5 @@ custom_block_add_action:
   +    - custom_block.list
   
   +++ b/core/modules/block/custom_block/custom_block.module
   @@ -38,25 +38,6 @@ function custom_block_help($path, $arg) {
   -      $item['localized_options']['query']['destination'] = 'admin/structure/block/custom-blocks';
   

   This is why we didn't use that already. We need that destination...

2. +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Plugin/Menu/LocalAction/CustomBlockAddLocalAction.php
   @@ -24,6 +25,10 @@ public function getOptions(Request $request) {
   +      $options['query']['destination'] = 'admin/structure/block/custom-blocks';
   

   Nevermind! LOL.

This looks great.

Let's invite the testbot as well.

Removed a lot of the old menu_get_item() as well as the code / tests which rely on it.

This was too agressive, let's just remove the ones we actually don't need but also remove menu_get_item() itself.

Can a maintainer add the "Approved API change", as we will remove stuff like local task support?

The general

15: local_task-2177031-15.patch queued for re-testing.

PHP Fatal error: Call to a member function rebuildIfNeeded() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Menu/LocalTaskManager.php on line 193

Any ideas?

I've had a look and can't see anything obvious why that wouldn't be there at that point.

Oh, some phpunit tests fail.

21: menu_get_item-2177031-21.patch queued for re-testing.

Thank you for your reviews!

Can you explain why this is needed?

Just have a look at: #2115025: Content admin views title saved localized to the menu table, this will explain the need for it. This patch just reapplies the same pattern.

Also, rebuilding the routes in the middle of the manager seems a bit odd - was that a bug you ran into?

Yes, this was a bug, see https://qa.drupal.org/pifr/test/726203
Saving a view triggers at some point a rebuild of everything, so the router system and local tasks. The notice you see exists, because the router is not rebuilt yet at that point.
The reason why this patch fails is the removal of menu_get_item(), which had a call to

-    \Drupal::service('router.builder')->rebuildIfNeeded();

integrated.

I thought about putting this into the render code as well, as there is no route provider access during the rebuild, though it seems to be something we really want to avoid be being
triggered on the actual side, unless something complex like a local task rebuild happens.

Needed a reroll, and had some stray comments referring to menu_get_item that I removed.

+++ b/core/modules/system/lib/Drupal/system/SystemManager.php
@@ -160,7 +179,9 @@ public function getMaxSeverity(&$requirements) {
+    $route_name = $this->request->attributes->get(RouteObjectInterface::ROUTE_NAME);
+    $items = $this->menuLinkStorage->loadByProperties(array('route_name' => $route_name));
+    $item = reset($items);

Follow-up idea: Provide a dedicated method on menu link storage (or elsewhere) for loading by route name.

I think this is ready to go, but I'm going to wait for green before reRTBCing.

Great! Since my only work on this issue was doc fixing at the end, I feel okay with RTBCing this.

This is a huge blocker, great job @dawehner!

Needs a change record draft to be RTBC.

I don't see that we should document more, all of that code is really deprecated already anyway
https://drupal.org/node/2203305

We need some before/after code of what you're supposed to do if you were depending on menu_get_item().

Updated: https://drupal.org/node/2203305/revisions/view/6951895/6952293

Looks good to me. The existing change notice(s) should indeed be sufficient for now — developers will have to re-implement all of their code for the request/response architecture anyway.

Just the DX of retrieving the current route name or route object looks a bit ugly - wondering whether we can improve that prior to release... But that's a different issue, of course.

menu_get_item-2177031-25.patch no longer applies.

error: patch failed: core/includes/menu.inc:431
error: core/includes/menu.inc: patch does not apply

@sun the DX issue is currently part of #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API, it's incredibly ugly but it's already being tracked.

Re-roll, only conflicted because a drupal_alter() was changed to moduleHandler in menu_get_item(), so just re-removed that part.

Added an extra note to the change notice pointing out the request attributes stuff is provisional.

34: menu_get_item-2177031-34.patch queued for re-testing.

Needs reroll tag removed.

Back to RTBC

The change notice wasn't exactly what I had in mind (more a before/after code comparison of D7 vs .D8). I tried to find one in the patch to use and came across this:

-    $menu_item = menu_get_item('menu-test/context');
+   $menu_items = \Drupal::entityManager()->getStorageController('menu_link')->loadByProperties(array('route_name' => 'menu_test.context'));

Which, um. Yikes! ;)

So given that people seem to use menu_get_item() for all manner of things, and given that we can't possibly ship with DX that bad, I think the way the current change notice is phrased is fine. Basically, if you needed stuff from menu_get_item() before, you now get it from $request->attributes->get(RouteObjectInterface::ROUTE_OBJECT); until we improve that in #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API. This code snippet also highlights that we pretty desperately need #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities.

Since this currently applies, getting it in while it's hot!

Committed and pushed to 8.x. WOOT.

- $menu_item = menu_get_item('menu-test/context');
+ $menu_items = \Drupal::entityManager()->getStorageController('menu_link')->loadByProperties(array('route_name' => 'menu_test.context'));
Which, um. Yikes! ;)

Well this is like comparing apples and oranges to be honest.

Added #2124749: [meta] Stop using $request->attributes->get(MAGIC_KEY) as a public API to the change record and a comment there WRT updating it when possible.