Deleted menus are still referenced as available menus on content types and cause an error when content is edited

Thank you for reporting this. I followed the steps to reproduce (thank you for including some!) on 8.0.x HEAD but the error did not occur. Is it possible a step towards the error was omitted?

I have tagged this "Needs title update" because the title as written does not describe the problem. Something more like "Deleted menus are still referenced as available menus on content types and cause an error when content is edited" may be better.

Was the "home" node added to the test2 menu at any stage?

Actually the error did occur but it is in my watchdog log because I don't display them. So, the steps are reproducible. I am sorry to have had you do it twice.

That is totally in scope of 8.0.x, so its just a simple bug.

Simplified issue summary, using issue template, moving to correct component, taking over.

Contains both fixes and tests. See what testbot thinks.

Great! Attaching test only patch.

Summary of above patch (#15):

It contains tests and fixes:

Ensures the Parent item <select> element found on node forms do not contain an empty <option> where a deleted menu would normally occupy (see screenshot in issue summary). This simultaneously fixes the PHP notice that appeared.

When a menu configuration is deleted, an entity hook will iterate over all node types and remove any menus referenced in the third party configuration storage. Tests ensure the entity delete hook purges invalid data on menu deletion. A new test file was created since there were no pre-existing kernel tests for menu_ui. The test creates a menu configuration, adds the menu to a node type's available menu settings, then deletes the menu configuration entity.

Patch is ready for review since #15 passed, and test only patch failed on the introduced assertions.

1. +++ b/core/modules/menu_ui/menu_ui.module
   @@ -17,7 +17,9 @@
   @@ -112,6 +114,21 @@ function menu_ui_menu_delete(Menu $menu) {
   
   @@ -112,6 +114,21 @@ function menu_ui_menu_delete(Menu $menu) {
   +  if (\Drupal::moduleHandler()->moduleExists('node')) {
   

   Couldn't the node module implement node_menu_delete() ?

2. +++ b/core/modules/menu_ui/menu_ui.module
   @@ -272,12 +289,12 @@ function menu_ui_form_node_form_alter(&$form, FormStateInterface $form_state) {
   -  $menu_names = menu_ui_get_menus();
   ...
   +  }, Menu::loadMultiple($type_menus));
   

   Replacing this we're losing the sort behaviour from menu_ui_get_menus()?

Couldn't the node module implement node_menu_delete() ?

The node module does not know about the menu items, or available menus. I put that functionality in menu_ui_menu_delete since menu_ui module is responsible for it all.

Replacing this we're losing the sort behaviour from menu_ui_get_menus()?

No problem, didnt realise this was a thing from reading the function doc block.

Patch changes reverts to using menu_ui_get_menus. Invalid menu ID's filtered with array_intersect_key.

Updated patch for 8.4.x-dev

See now that the tests will fail with patch. But unassigning for now.

Hi, this happens not only when a menu has been created and then deleted, but also when a migration takes place.

To reproduce

1. Install a standard Drupal 7 site (7.56 in my case)
2. Create a content type and select Main menu as an available menu for that content type
3. Install a standard Drupal 8 site (8.4.0 in my case)
4. Migrate content from Drupal 7 to Drupal 8
5. In D8 try to edit a node from the content type you migrated from D7

I guess that this happens because in D7 the menu is 'main-menu' but in D8 the menu is 'main', so from the D8 perspective, 'main-menu' doesn't exist. So I guess that this can be handled too in the migration process as seems that that information of the available menu for a given content type is not migrated/updated.

Not sure if a new issue must be created but as this is related I report it here. Let me know if I had to create a new issue.

This resolved the issue for me after migrating from Drupal 7 to Drupal 8.

We also have this issue on migrated site. Is there a way to fix this on already migrated site?

I resaved my content types and all errors gone away:)

Confirming resaving my content types worked for me as well.

Thank you for sharing this. Perfect solution.

Thanks,
Anoop Singh

Thanks dillix

For those tracking site configuration with Git, you could re-save the content type and then export configuration or you could edit the YAML directly. For example, this is happening with my "article" content type, so I can update "node.type.article.yml"

from:

third_party_settings:
  menu_ui:
    available_menus: {  }
    parent: 'main-menu'

to:

third_party_settings:
  menu_ui:
    available_menus: {  }
    parent: ''

Still an issue in 8.5.4 as well. I have updated the parent.

I made an update to the patch for 8.5.5.

Previous patch was wrong - here is the the updated patch for 8.5.x.

I had to manually run the patch it didn't play nice with composer. Anyone have any ideas?

I just wish there was better integration for third_party_settings. For example, a way to say "By the way, that thing is now a dependency!". Or handling for default settings, so the provider is not added as a dependency when it's added config is empty.

Then we could do something like this:

function menu_ui_form_node_type_form_builder($entity_type, NodeTypeInterface $type, &$form, FormStateInterface $form_state) {
  $available_menus = array_values(array_filter($form_state->getValue('menu_options')));
  if (!empty($available_menus)) {
    $menus = \Drupal::entityTypeManager()->getStorage('menu')->loadMultiple($available_menus);
    foreach ($menus as $menu) {
      // Does not work: Protected function and dependencies is cleared in ConfigEntityBase::calculateDependencies().
      $type->addDependency($menu->getConfigDependencyKey(), $menu->getConfigDependencyName());
    }
  }
  $type->setThirdPartySetting('menu_ui', 'available_menus', $available_menus);
  $type->setThirdPartySetting('menu_ui', 'parent', $form_state->getValue('menu_parent'));
}

I followed the steps to reproduce on 8.9.x, and the error seems not to be occurring anymore.

Here is the test I wrote, (following the steps listed in the issue)

  /**
   * Tests that a deleted menu is no longer referenced as an available menu.
   */
  public function testDeletedMenuIsNoLongerReferencedAsAnAvailableMenu() {
    // Create a new menu.
    $menu = Menu::create([
      'id' => 'llama',
      'label' => 'Llama',
    ]);
    $menu->save();

    // Enable the llama menu.
    $edit = [
      'menu_options[llama]' => TRUE,
    ];
    $this->drupalPostForm('admin/structure/types/manage/page', $edit, t('Save content type'));

    // Ensure the llama menu is available.
    $this->drupalGet('/node/add/page');
    $this->assertOption('edit-menu-menu-parent', 'llama:');
    $this->assertOptionByText('edit-menu-menu-parent', '<Llama>');

    // Delete the llama menu.
    $menu->delete();

    // Ensure the llama menu is not available.
    $this->drupalGet('/node/add/page');
    $this->assertNoOption('edit-menu-menu-parent', 'llama:');
  }

which passes.

However, it looks like the "llama" menu, is still present on the node type configuration after deleting the menu.

So the following patch updates the test above, to ensure the third party settings is no longer present, which currently fails.

I am not entirely sure when and how this was fixed, and perhaps we could refactor the test to a kernel, because the fix we need here currently would only need to update the third party settings, however, I guess unless not covered somewhere else, it would be nice to prove that we don't actually have this problem anymore. I'll let someone else decide that :)

Triaged as part of bug smash initiative - this needs a reroll, looks like the last passing patch was from @dpi in #21

.

As mentioned in #46: displaying the error in the UI cannot be reproduced, but the deleted menu is still available in the node type configuration in third party settings.

Run this snippet after deleting a menu in step 4 above:

use Drupal\node\Entity\NodeType;
use Drupal\system\Entity\Menu;
$node_type = NodeType::load('page');  // Replace 'page' if necessary.
$menu_ids = $node_type->getThirdPartySetting('menu_ui', 'available_menus');
var_dump($menu_ids);
$menus = Menu::loadMultiple();
var_dump(array_keys($menus));

Rebasing: two merge conflicts in MenuUiNodeTest.php and menu_ui.module. New patch based on #21 en #46:

menu_ui.module:

1. Moved patch code in menu_ui_menu_delete() to MenuDeleteForm.php

   Replace $menu->id() with $this->entity->id().

   This hunk fixes the third party settings after deleting a menu via the UI.

2. Skipped re-applying patch code in menu_ui_form_node_form_alter(). This seems no longer necessary.

menu_ui/tests/src/Kernel/MenuNodeTest.php:

1. This kernel test, which does not require a UI, always fails. It was replaced with the testDeletedMenuIsNoLongerReferencedAsAnAvailableMenu() (see below)

menu_ui/tests/src/Functional/MenuUiNodeTest.php:

1. Add testNodeFormInvalidMenu()
    

   Retrieved from menu_ui/src/Tests/MenuNodeTest.php (cfr. point 2)

   Add property and modify setup() accordingly.

   Replace assertEqual() with assertEquals().
   Replace assertOption() with assertSession()->optionExists().
   Replace assertNoOption() with aassertSession()->optionNotExists.

2. Add testDeletedMenuIsNoLongerReferencedAsAnAvailableMenu() from 2592829-46--TEST-ONLY.patch in comment #46.

   Replace $menu->delete(); with:

     $this->drupalGet('admin/structure/menu/manage/llama/delete');
     $this->getSession()->getPage()->pressButton('Delete');
     $this->assertSession()->statusCodeEquals(200);
   

   Replace drupalPostForm() with drupalGet() and submitForm().
   Replace assertOptionByText() with assertSession()->optionExists().
   Replace assertOption() with assertSession()->optionExists().
   Replace assertNoOption() with aassertSession()->optionNotExists.

3. A few minor changes to pass Drupal code quality checks.

Hmm, I can't reproduce this anymore. Following the steps in the IS, it works as expected.

Before deleting:

After deleting:

And no warnings in the logs.

I'm going to close this since the original issue is no longer valid. I will create a follow up for the lingering config.