Unrouted URLs break toolbar but are hidden by caching

Let's ensure we write test coverage first. Having such a user input flow is not that uncommon.

Plus, one of them is a content entity.) Is it reasonable to write a BrowserTest to attempt to simulate pushing all the buttons?

Well, yeah, why not?

Once we have found a fix we can write an additional test on the lower level system where we fix the actual issue.

A before/after database dump ought to flush out the actual issue assuming it's a reference elsewhere in the database.

Also can you confirm rebuild.php definitely does not restore this? Issue summary isn't explicit.

Looking into this.
I reproduced, and can confirm that rebuild.php does not help.

Working on a test only.

Steps 2 and 3 (the Views parts) are a red herring.
Views helps because it clears way too many caches when a view is saved.
But doing just Step 1 and hitting rebuild.php should trigger it just the same.

Problem 1: The toolbar cache isn't being updated when the menu is.
Problem 2: When the toolbar is updated, it fatals on unrouted URLs.

Step 1 creates a link with a URL of base:/admin/foo, which will fatal.
So would http://example.org, which should be valid.

This patch addresses problem 2, but skips over problem 1 by putting a drupal_flush_all_caches() call in the test.

At the end of toolbar_toolbar(), it adds in cacheability info to the administration portion of the toolbar. This includes #cache][tags][] = config:system.menu.admin
I'm guessing that should be the entity? Or is that supposed to work?

At the end of toolbar_toolbar(), it adds in cacheability info to the administration portion of the toolbar. This includes #cache][tags][] = config:system.menu.admin
I'm guessing that should be the entity? Or is that supposed to work?

Does that basically mean the menu tree cache entry should have all of its items as cache tags?

+++ b/core/modules/toolbar/toolbar.module
@@ -248,7 +248,7 @@ function toolbar_menu_navigation_links(array $tree) {
     $url = $link->getUrlObject();
-    if ($url->isExternal()) {
+    if (!$url->isRouted()) {
       // This is an unusual case, so just get a distinct, safe string.

Oh I see! Yeah there was a point in time, where this was the same, but this is no longer true.

here is a patch with a regression test for the current issue summary. (It needs quite some time, so I kinda assume it will fail)

Attempt at a better title.

The underlying question, can / should we protect against such mistakes and maybe trigger an ERROR + return an empty string?

Well an external URL should work fine. I'm not sure what to do about an unrouted one.

Fixing a minor problem in that test, and adding it to my patch.

I don't understand toolbar caching, menu tree caching, or entity "list cache tags" enough to move this further.

+++ b/core/modules/views_ui/src/Tests/DisplayPathTest.php
@@ -149,4 +151,69 @@ public function testMenuOptions() {
+   * Tests the regression in https://www.drupal.org/node/2532490.

Better desc please.

1. +++ b/core/modules/toolbar/src/Tests/ToolbarAdminMenuTest.php
   @@ -378,6 +380,29 @@ public function testBackToSiteLink() {
      /**
   +   */
   

   Meh, we probably need some text here.

2. +++ b/core/modules/toolbar/src/Tests/ToolbarAdminMenuTest.php
   @@ -378,6 +380,29 @@ public function testBackToSiteLink() {
   +    // @todo Remove this.
   

   Remove this as part of a bugfix or as part of the patch?

#17 addresses #19. Time-travel!

I always knew tim has superpowers! Finally its out!

Does that basically mean the menu tree cache entry should have all of its items as cache tags?

That could cause us trouble - lots of cache tags to check for invalidations, and the number of menu items in a menu is almost unlimited.

When we change an item in the menu tree, we should be able to invalidate the menu cache tag no?

I'm pretty sure we used to do that, that's what caused tons of identical cache tag invalidations and lead to adding the static cache there. Not sure if we still do.

We definitely still do I think. See MenuTreeStorage.:save(), which is called by MenuLinkManager::add/updateDefinition, which for example is called by MenuLinkContent::postSave().

Right, but saving a view also now triggers potentially the menu tree to change, I guess we don't take that yet into account?

So all of the proper cache tags are invalidated, but the menu tree is still loaded from an old cache.

This is because it's cached as config:system.menu., with no menu name.
Both toolbar_prerender_toolbar_administration_tray() and _toolbar_do_get_rendered_subtrees() call $menu_tree->load(NULL, $parameters); for no apparent reason, and that NULL is messing everything up AFAICS.

Changing it to 'admin', which is what it was before MenuLinkNG pt 4, fixes the tests.

The FAIL patch has the new fix without the other toolbar changes.

I kinda dislike that toolbar is not written at all in a generic way, seriously, it feels more like a custom module, but sure, coupling it to a specific menu does not make the situtation worse at all, given that its already coupled to a specific menu item.

+++ b/core/modules/toolbar/toolbar.module
@@ -218,7 +218,7 @@ function toolbar_prerender_toolbar_administration_tray(array $element) {
-  $tree = $menu_tree->load(NULL, $parameters);
+  $tree = $menu_tree->load('admin', $parameters);

@@ -296,7 +296,7 @@ function _toolbar_do_get_rendered_subtrees(array $data) {
   $parameters->setRoot('system.admin')->excludeRoot()->setMaxDepth(3)->onlyEnabledLinks();
-  $tree = $menu_tree->load(NULL, $parameters);
+  $tree = $menu_tree->load('admin', $parameters);

Do you mind adding a follow up which ensures that you cannot pass in NULL, there? I guess an assertion would be the perfect usecase.

First: if a wrong cache tag is the root cause, then let's add a assertCacheTag() assertion.

I kinda dislike that toolbar is not written at all in a generic way, seriously, it feels more like a custom module, but sure, coupling it to a specific menu does not make the situtation worse at all, given that its already coupled to a specific menu item.

It kinda does make it worse, because now you must use the admin menu; in D8 HEAD, you can create your own admin menu and the toolbar module would just use that instead: whichever menu contains the system.admin menu link (i.e. the system.admin route, i.e. /admin) that represents the root of the admin menu, would win. At least, that's what I remember from when Jesse Beach worked on this.

Isn't the actual problem this:

      $menu_name = $link->getMenuName();
      …
      // Set cache tag.
      $build['#cache']['tags'][] = 'config:system.menu.' . $menu_name;

i.e. the fact that $link->getMenuName() returns NULL apparently? But actually… I cannot reproduce that: it actually is 'admin', without any of the patch applied. So I don't think that's the actual cause?`

That code is in MenuLinkTree, and it absolutely has the correct $menu_name.

The problem is in \Drupal\Core\Menu\MenuTreeStorage::loadTreeData(), which is called directly from \Drupal\Core\Menu\MenuLinkTree::load(), which toolbar purposefully passes NULL as the menu name.

    $tree_cid = "tree-data:$menu_name:" . serialize($parameters);
    ...
      $this->menuCacheBackend->set($tree_cid, $data, Cache::PERMANENT, ['config:system.menu.' . $menu_name]);

There is nothing in this method that I can see that would be able to populate that menu name.

So it's not the #cache cache tags that are wrong, it's the ones for the actual cache_menu entry, which AFAIK are not passed to X-Drupal-Cache-Tags.

Ahh! Oops, sorry! In that case, +1, but here's another review first:

+++ b/core/modules/toolbar/toolbar.module
@@ -296,7 +296,7 @@ function _toolbar_do_get_rendered_subtrees(array $data) {
   $parameters->setRoot('system.admin')->excludeRoot()->setMaxDepth(3)->onlyEnabledLinks();
-  $tree = $menu_tree->load(NULL, $parameters);
+  $tree = $menu_tree->load('admin', $parameters);

If we do this, then I think we might as well remove the setRoot() call.

Then the toolbar is simply tied to the admin menu, and not to both a specific menu item and a menu.

This then opens the door in 8.1 to make it configurable which menu is used.

Do you mind adding a follow up which ensures that you cannot pass in NULL, there? I guess an assertion would be the perfect usecase.

+1 in principle. But note that \Drupal\system\Controller\SystemController::overview() + \Drupal\system\SystemManager::getAdminBlock() also use MenuLinkTree::load(NULL, …)! And this dates back to before #2301317: MenuLinkNG part4: Conversion. The idea is to query the entire menu tree I guess. So we should investigate it further before just removing it.

Opened #2535208: Require a menu name in \Drupal\Core\Menu\MenuTreeStorageInterface::loadTreeData() and \Drupal\Core\Menu\MenuLinkTreeInterface::load() to discuss removing support for NULL.

Moving back to RTBC, because this is broken, we have a fix, and the magic "whatever menu contains system.admin" feature seems unimportant in comparison, and removing it is not a regression from D7.

Cross post.

The idea is to query the entire menu tree I guess. So we should investigate it further before just removing it.

No question, it should not be part of this issue.

So the remaining bits of this issue is to remote the setRoot() call?

So the remaining bits of this issue is to remote the setRoot() call?

Yep, and I've been talking with Tim about that. Patch soon, I think. Then this is RTBC.

Had to tweak the parameters a bit more than just removing the setRoot call.

This now makes #1869638: Make the menu shown in the administration menu tray configurable a bit easier.

I think I won't RTBC that.

Ugh forgot to pull. automatically resolved during rebase.

#2534830: Toolbar subtrees not working and #2534856: Remove unused _toolbar_get_user_cid() touched the same file, no conflicts.

Should be green, manually tested again, RTBC.

+1 to RTBC

Tim told me to do this.

@mtift helped me with #28, which was a big breakthrough.
Adding Wim to the proposed commit message as well.

Sorry, I went in hoping to commit this, but ended up instead with questions. :(

+    $edit = [
+      'title[0][value]' => 'External URL',
+      'link[0][uri]' => 'http://example.org',
+      'menu_parent' => 'admin:system.admin',
+    ];
+    $this->drupalPostForm($menu->urlInfo('add-link-form'), $edit, 'Save');
+
+    $this->drupalGet($menu->urlInfo('edit-form'));
+    $this->assertText('External URL');
+
+    $this->drupalGet(Url::fromRoute('<front>'));
+    $this->assertText('External URL');

I do not understand the correlation here at all between what data was mocked and what is being asserted. Why is 'edit-form' pointing to this URL? Why is <front> an external URL? Can we explain the thinking here a bit in the comments?

+  $parameters->setMinDepth(2)->setMaxDepth(2)->onlyEnabledLinks();

+  $parameters->setMinDepth(2)->setMaxDepth(4)->onlyEnabledLinks();

It is non-obvious what these magical numbers mean, so it would be great to get a comment above each to explain.

+    $this->drupalPostForm($menu->urlInfo('add-link-form'), $edit, 'Save');
...
+    $this->drupalPostForm('admin/structure/menu/manage/admin/add', $edit, t('Save'));

Minor, but both of these are doing the same thing but are written two different ways. We should probably standardize on the route name lookup.

+    $this->drupalPostForm('admin/structure/views/add', $edit, t('Save and edit'));

Same feedback here.

+    $this->drupalPostForm(NULL, [
+      'tab_options[type]' => 'normal',
+      'tab_options[title]' => 'Parent title',
+    ], t('Apply'));
+
+    $this->drupalPostForm(NULL, [], t('Save'));

Here we do some set up, but don't seem to assert that anything happened. Omission?

In the meantime, saved the recommended commit credit (including Crell, who reported the issue).

#46

1) That was me being fancy with URLs, I agree it makes it more confusing and harder to read. Reverted.
Also added comments to clarify what we're asserting.

2) Agreed, did my best to document.

3/4) Switched to path-based

5) Without the fix, this would throw an exception and simpletest would register that. But to make it clear, added an assertResponse and comment.

I am boldly setting this back, as there were no functional changes (just adding one assert).

Fantastic, thanks!!

Committed and pushed to 8.0.x. W00t!

I followed the original instructions (which are no longer available) to reproduce this issue and was able to. Rebuild did not resolve it.

I applied patch #47 and it fixed the problem. Nice work!