Support from Acquia helps fund testing for Drupal Acquia logo

Comments

opdavies created an issue. See original summary.

weseze’s picture

It is needed in D8. (tested in 8.2)

opdavies’s picture

Thanks @weseze.

I'd tested it recently myself and come to the same conclusion, but forgot to post a comment. :(

johnwebdev’s picture

Yep, I agree.

I did some quick skimming, as we don't have hook_menu_link_update anymore, we'll have to use hook_entity_update().

From, there we could do something like:

function pathauto_menu_link_menu_link_content_update(Drupal\Core\Entity\EntityInterface $entity) {
  // Menu link id
  $menu_link_id = $entity->id();

  $link = $entity->get('link')->first();
  $link_values = $link->getValue();
   
  if ($link->isExternal()) {
    // There is no node...
  }  

  list($entity_type_id, $entity_id) = explode('/', $link_values['uri'], 2);
}

Now we have the node id, and from there we can pretty much do like in Drupal 7. Is there something that SHOULD be different for the 8 version though?

MenuTreeStorage::getAllChildIds gets us all the child ids.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Menu%21Me...

I'd love to contribute on porting this to D8, any chance we can get it started?

opdavies’s picture

Title: Needed for Drupal 8? » Port to Drupal 8
opdavies’s picture

Version: 7.x-1.x-dev » 8.x-1.x-dev

opdavies’s picture

Thanks @johndevman. This is a great start.

I've started a 8.x-1.x branch, and added the code as the second commit. :)

johnwebdev’s picture

Hmm. I'm stuck at the moment. The problem is that the menu tree is updated after all Menu link entities have been updated. So the proposed solution at the moment (or our idea) will work. But, if the Path contains Menu tokens, it won't get the right one, as the tree storage hasn't been updated when we generate the new alias.

  public function updateDefinition($id, array $new_definition_values, $persist = TRUE) {
    $instance = $this->createInstance($id);
    if ($instance) {
      $new_definition_values['id'] = $id;
      // Updates Menu Link Entity.
      $changed_definition = $instance->updateLink($new_definition_values, $persist);

      // Here we update the menu tree.
      $this->treeStorage->save($changed_definition);
    }
    return $instance;
  }

So if we have this menu:

   Foo 1
     Foo 2
   Bar 1
   Bar 2

And we move in Bar 2 in Bar 1. The alias won't "change", but when we move in Bar 2 into Foo 1, it will get bar1/bar2

One way to work around these issues, is that we could store the entity id(s) in some way (State API, Queue, Database) and process them asap after Menu tree has been updated.

Another idea is that we hook in to the Menu UI and add an callback that resaves the Menu links entities after the menu tree has been stored. But then we are LIMITED to people updating through Menu UI, and it won't react to programmatically updates in the menu.

Edit: I got an idea. We could use hook_menu_update() hook to perform the updates of node, at that point menu should be updated. I'll give it a shot and hopefully provide a initial working patch.

johnwebdev’s picture

Here's an initial working patch:

realityloop’s picture

The patch in #10 only updates the dragged menu item, not any of it's children

realityloop’s picture

Status: Active » Needs review
FileSize
2.11 KB

This patch updates node paths for child menu links of the moved menu item as well

realityloop’s picture

was an issue with patch in #12

johnwebdev’s picture

#13 The updating should be done in the latter hook since the parent item haven't been updated yet.

realityloop’s picture

@johndevman thats strange, I've tested this a fair bit and it's working as I'd expect (updates all children node paths).

8thom’s picture

Updating patch in #13 to include is_array check

rli’s picture

Updated patch in #16 to add condition check to rule out <front> and invalid URLs.

johnwebdev’s picture

Cool!

Looks like we're getting somewhere, to move ahead we should add at least a Kernel test to ensure it works as expected.

Covering following scenarios:

- That the moved menu link items path alias has been updated
- That the moved items children alias has been updated
-Ensure menu tokens works as expected

Edit: I'm working on the test.

johnwebdev’s picture

Here's an W.I.P on the Kernel tests that currently verifies:

- That the moved menu link items path alias has been updated

The key part is:

    // Move menu item.
    $menu_link_manager = \Drupal::service('plugin.manager.menu.link');
    $menu_link_manager->updateDefinition($node2_menu_link->getPluginId(), [
      'parent' => $node1_menu_link->getPluginId(),
    ]);

    // Save menu entity so we trigger its hook.
    $this->menuEntity->save();

It updates the second menu link to have the first one created as its parents, and then the Menu entity gets updated to trigger the hook, as done in the Menu UI module.

Not entirely sure, if this is the right way to go? Perhaps we should make an additional Web test to use the most common scenario (rearranging through the Menu UI) - or just scrap this one, and go for only a Web test.

johnwebdev’s picture

Edit: Missposted in the wrong issue.

johnwebdev’s picture

joelpittet’s picture

#19 looks good enough to commit:) Nice job adding a test there too. The test class name should probably match the filename and the modules don't need to be all included I bet but otherwise good work flushing this out.

xavier.masson’s picture

The following code from pathauto_menu_link.module line 25 show warning when using path like route:<current>:

  list($identifier, $entity_id) = explode('/', $link_values['uri'], 2);
  $entity_type = substr($identifier, strpos($identifier, ':') + 1);

Proposed resolution

Avoid to use

 list($identifier, $entity_id) = explode('/', $link_values['uri'], 2);

and use instead :

  $uri_parts = explode('/', $link_values['uri'], 2);
  $identifier = isset($uri_parts[0]) ? $uri_parts[0] : NULL;
  $entity_id = isset($uri_parts[1]) ? $uri_parts[1] : NULL;
  if (empty($identifier) || empty($entity_id)) {
    return;
  }