Problem/Motivation

menu_link_content module can provide menu links regardless of whther menu_ui module is installed.

However the cleanup for nodes being deleted is in menu_ui:


/**
 * Implements hook_ENTITY_TYPE_predelete() for node entities.
 */
function menu_ui_node_predelete(EntityInterface $node) {
  ...
}

So, non-node entities are not handled and this is not handled if menu_ui is uninstalled.

Proposed resolution

Generically handle entity deletion in menu_link_content module

Remaining tasks

doit

User interface changes

none

API changes

none

CommentFileSizeAuthor
#22 interdiff.txt2.33 KBamateescu
#22 2350797-22.patch4.02 KBamateescu
#22 2350797-22-test-only.patch1.6 KBamateescu
#19 2350797-19.patch4.12 KBdagmar
#3 2350797-3.patch4.08 KBpwolanin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2350797-3.patch. Unable to apply patch. See the log in the details link for more information. View
#3 increment.txt1.5 KBpwolanin
#2 2350797-2.patch4.02 KBpwolanin
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,074 pass(es), 65 fail(s), and 2 exception(s). View
#2 increment.txt713 bytespwolanin
#1 2350797-1.patch4.03 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,079 pass(es), 65 fail(s), and 2 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
4.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,079 pass(es), 65 fail(s), and 2 exception(s). View

One of many more issues.

pwolanin’s picture

FileSize
713 bytes
4.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,074 pass(es), 65 fail(s), and 2 exception(s). View

test fix

pwolanin’s picture

FileSize
1.5 KB
4.08 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2350797-3.patch. Unable to apply patch. See the log in the details link for more information. View

handle all possible entity routes.

The last submitted patch, 1: 2350797-1.patch, failed testing.

The last submitted patch, 2: 2350797-2.patch, failed testing.

pwolanin’s picture

per dawehner, need to fix Views so it properly returns all the page displays in the list of templates.

dawehner’s picture

How to get all routes provided by a particular view (at the moment):


$routes = [];
$view_id = 'frontpage';
$executable = \Drupal\views\Views::getExecutable($executable);
$view_route_overrides = \Drupal::state()->get('views.view_route_names');
foreach ($executable->displayHandlers as $display_id => display) {
  if ($display instanceof \Drupal\views\Plugin\views\display\DisplayRouterInterface) {
    if (!isset("$view_route_overrides[$view_id.$display_id"])) {
      $routes[] = "views.$view_id.$display_id";
    }
    else {
      $routes[] = $view_route_overrides[$view_id.$display_id]
    }
  }
}
pwolanin’s picture

Here's what I have so far for adding to the View class, but discussing with dawehner, maybe we should postpone additional Views cleanups until e.g. #2350503: Add route generation handlers for entities is done


  protected function linkTemplates() {
    $templates = parent::linkTemplates();
    $executable = $this->getExecutable();
    $view_route_overrides = \Drupal::state()->get('views.view_route_names');
    foreach ($executable->displayHandlers as $display_id => $display) {
      if ($display instanceof \Drupal\views\Plugin\views\display\DisplayRouterInterface) {
        if (isset($view_route_overrides["{$this->id}.$display_id"])) {
          $templates["views-{$this->id}-$display_id"] = "views.{$this->id}.$display_id";
        }
        else {
          $templates["views-{$this->id}-$display_id"] = $view_route_overrides["{$this->id}.$display_id"];
        }
      }
    }

    return $templates;
  }
pwolanin’s picture

bump

mgifford queued 3: 2350797-3.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2350797-3.patch, failed testing.

pwolanin’s picture

bump

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

pwolanin’s picture

Version: 8.2.x-dev » 8.4.x-dev
Issue tags: +Triaged for D8 major current state, +DrupalCampNJ2017

This bug still exists in 8.4

The views bug was fixed, so this should be viable to be worked on now

catch’s picture

Thanks for checking if this was still valid.

Discussed with @alexpott, @cilefen, @cottser, @laurii, and @xjm and we think it's worth bumping this to critical since it's a data integrity bug (albeit leaving cruft as opposed to deleting too much). Looks to me like since the menu links will still be valid, we'd be trying to load the entity to check access every time, even though it's not there - as opposed to it only being extra rows in the db.

catch’s picture

Title: Move node delete cleanup from menu_ui to menu_link_content and handle all entities » Orphaned menu links when nodes are deleted if menu_link_ui is not enabled
xjm’s picture

Priority: Major » Critical
dagmar’s picture

Status: Needs review » Needs work

The last submitted patch, 19: 2350797-19.patch, failed testing.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.6 KB
4.02 KB
2.33 KB

Reviewed the code and it makes sense. Here's a fix for the test and some cosmetic fixes.

Going straight to RTBC because my changes are minor.

The last submitted patch, 22: 2350797-22-test-only.patch, failed testing. View results

  • catch committed e549374 on 8.5.x
    Issue #2350797 by pwolanin, amateescu, dawehner, dagmar: Orphaned menu...
catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

  • catch committed f2667b5 on 8.4.x
    Issue #2350797 by pwolanin, amateescu, dawehner, dagmar: Orphaned menu...
xjm’s picture

Issue tags: +8.4.0 release notes
mikran’s picture

Status: Fixed » Active

I've found a new problem that comes with this. Module uninstall is broken if configuration is being deleted during the uninstall. Here is one example how to reproduce:

  1. Install Drupal using the standard installation profile
  2. Download & Enable token contributed module
  3. Try to uninstall menu_link_content module

This gives error Symfony\Component\Routing\Exception\MissingMandatoryParametersException: Some mandatory parameters are missing ("entity_type_id") to generate a URL for route "entity.entity_view_mode.add_form".

I was able to reproduce this with drush and from UI.

dawehner’s picture

Status: Active » Fixed

@mikran
Please create a new issue and link it from here. This way people don't need to read the old discussion ... Thank you

mikran’s picture

Status: Fixed » Closed (fixed)

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

malik.kotob’s picture

After this change was introduced I'm getting the following error when attempting to delete an entity belonging to a custom content entity type generated by Drupal console:

The website encountered an unexpected error. Please try again later.</br></br><em class="placeholder">Drupal\Core\Entity\EntityStorageException</em>: Parameter &quot;personnel_revision&quot; for route &quot;entity.personnel.revision_revert&quot; must match &quot;[^/]++&quot; (&quot;&quot; given) to generate a corresponding URL.

The link has the following form: "/admin/structure/personnel/{personnel}/revisions/{personnel_revision}/revert". The {personnel} parameter is populated properly, but the {personnel_revision} parameter is missing, hence the error. Is there something we should be doing in code to ensure the revision parameter is passed in? Not too familiar with the routing system.

hchonov’s picture

It looks like the change here is now preventing the deletion of entities having custom link templates with mandatory parameters, which are not automatically set by \Drupal\Core\Entity\EntityInterface::toUrl(). I've created an issue for this - #2924338: Entity::uriRelationships() throws exceptions if an URL cannot be generated because of missing mandatory parameters, which is a major as it looks like an usual case when e.g. using the diff module and suddenly after an update inside 8.4.x it is not possible to delete entities.

cburschka’s picture

In addition to the bug above, we now also lose unparameterized collection routes. For example, menu links that point at /admin/people disappear when a user is deleted.

#2926897: Deleting content entities also deletes collection menu items.