Decouple shortcuts from menu links

I investigated this a bit a couple of days ago, and it's bad, really bad :/

Shortcut links don't have any kind of relationship with the menu link they were originally created from, so we have no good way of updating their 'route_name' property when the original menu link changes.

One way to do it would be in a hook_entity_presave() for menu_link, but that means doing one more select for each saved menu link, and _menu_navigation_links_rebuild() is already slow as hell.

The only way forward that I can think of is to decouple shortcuts from menu links and convert them to a separate entity type with an entity reference field.

I was pointed to this issue here from #1895160-63: Convert admin/content to a View, keep a non-views fallback with no bulk operations...

STR:

1. disable the views_ui.module
2. disable the views.module
3. Symfony\Component\Routing\Exception\RouteNotFoundException: Route "view.content.page_1" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 126 of /home/s4d78154f0ffa1ad/www/core/lib/Drupal/Core/Routing/RouteProvider.php).

The patch just adds a test for this bug, it doesn't try to solve anything..

ok, thanx. I edited my comment above.

In addition to #3:

Same error occurs if you try to enable Views again after disabling.

I can't really imagine this to just be major to be honest.

Sadly we can't really know what is updated but there is at least a method on MenuLink.php which just would have to be executed again, to get the proper route name: findRouteName(). So we could truncate all route names, on module disable and then lazy-rebuild them on usage?

Yeah this seems critical enough and I can't see it only happening if a module gets disabled/uninstalled - would be affected by any change in alters too no?

I thought about introducing an event subscriber for module enable/update/disable/delete or router rebuild which updates all menu_links but this won't really work, as you can't mass-update menu links (and you shouldn't).

The question is: Is the entity storage the right place to store the route_name? Isn't that information which is just between the route system and menu links, and don't belong to the actual entity. If we store the mapping for example in a key value store, or maybe in some cache array, resetting the values on a router rebuild would be simple.

With a cache array you would also get lazy rebuilding of the information.

I just replicated the problem in #3 on SimplyTest.me twice.

Can the router be re-built when modules are disabled?

Thanks Tim. I just ran into this issue when trying to address another one. Appreciate the info though.

This happened to me while simply disabling the admin/content view at admin/structure/views, i.e. not even disabling modules.

Why would you ever want that ;)

Feedback on the idea of #10 is still wanted.

I still prefer the decoupling from menu links and turning shortcuts into a separate entity type with a reference property. This wasn't possible when shortcut.module was written (D7) but it should be really easy to do now.

I somehow doubt that a) this can actually happen due to API freeze and b) this will happen, given that people are already quite busy.

Afaik, API freeze does not apply to critical bugs. And b) you knew exactly what buttons to push to make me work on it, didn't you? :P

First of all, we need to do #2036629: 'shortcut' entity type is very confusing and should be renamed to 'shortcut_set' so we have the 'shortcut' namespace available here.

API freeze doesn't apply to critical bugs when an API change is required to fix the bug in a sane way. #10 looks sane enough to me.

The thing is, I'm doing #16, not #10... Let me know if I should stop :)

Unless there's a particular reason not to do #10 let's do that please.

Here are my reasons:

1. #10 can be done regardless of #16
2. my approach is more like "port shortcut.module to D8", since not a lot of people care about its fate and I've been burned a lot by it while working on the conversion of menu links to entities
3. it is 80% done, for #10 we would need to find someone to do it
4. @pwolanin wants to turn 'link_path' into a machine name for menu links, and that won't be unique as long as we keep shortcuts in there

@amateescu
Just to be clear, does your port will solve it automatically or do you still need some similar logic as #10?

It will solve it because shortcuts will not be saved as menu links anymore (basically duplicating the menu link with a different menu and link_title), but as standalone entities with a reference to a menu link.

Here's what I have so far. The new entity type seems to work, but we still need to update the old code that expected menu links and receive shorcuts now.

anyway this approach requires re-sync of links and references when shortcut module disable/enable and there's no code that listens for changes on menu-link entity

otoh moving from string translation to entity translation looks promising

+++ b/core/modules/shortcut/shortcut.installundefined
@@ -22,6 +22,98 @@ function shortcut_uninstall() {
 function shortcut_schema() {
+  $schema['shortcut'] = array(
+    'description' => 'Stores shortcut items.',
...
+  $schema['shortcut_property_data'] = array(
+    'description' => 'Stores shortcut properties.',

another abstraction on top of menu link? suppose translation become worth

+++ b/core/modules/shortcut/shortcut.installundefined
@@ -22,6 +22,98 @@ function shortcut_uninstall() {
 function shortcut_schema() {
+  $schema['shortcut'] = array(
...
+      'menu_link' => array(

maybe better use route_name here?

anyway this approach requires re-sync of links and references when shortcut module disable/enable

I guess you missed the critical issue about removing module enable/disable functionality from D8? :)

And we can't reference a route name, because we need to reference an actual path (that would be route name + params), but it's totally not worth it because menu links will already do it for us.

P.S. Could you edit your comment to remove those table schemas? Keep the first row of the declaration, but the rest doesn't bring anything for someone trying to scan this issue :)

So the only translation of shortcut title is questionable.
#1199946: Disabled modules are broken beyond repair so the "disable" functionality needs to be removed is not in core yet.

PS: so shortcut becomes content entity? why not use link field for that?

It's not questionable, it's an improvement at the moment, because menu links are not yet translatable.

Yeah, removal of module disable is not in core yet, but it's critical, so it certainly will be.

And yes, shorcuts become content entities, and I'm not using a configurable field because this is kind of a "hidden" property, it's not even shown in the shortcut form, so I think that would be overkill.

Here's the current state of things. It's almost there, just need a couple fixes and an upgrade path.

And, of course, I need to fix the initial problem, update the route name of the shortcut when the route is changed, but that's just copying the code that does it for menu links.

#2083123: Shortcut cleanup: Remove duplicated code and deprecate legacy functions

Getting there.

Now with upgrade path.

I believe it is an API change. Issues summary is not in proper format and I think we should list all the api changes.

A bit more progress. One of the fails will be fixed by #2029569: Convert node_admin_nodes to a new-style Controller.

Nope, I didn't want to remove those tags.

...adding back tags that got removed again :/

Keeping up with HEAD and a tentative fix for aliases.

Turns out drupal_schema_get_field_value() had to be fixed, not removed from the storage controller.

And this fixes the last failures, spoke to @dawehner and it turns out getPathFromRoute() is not really deprecated after all. This should be green so I'll write a new issue summary.

Update the issue summary and let's also add the original test from Tim :)

1. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -33,4 +33,30 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   
   @@ -33,4 +33,30 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +  public static function findRouteNameParameters($link_path) {
   

   As written below, this should not really be a static function.

2. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkInterface.php
   --- a/core/modules/menu_link/tests/Drupal/menu_link/Tests/Plugin/Core/Entity/MenuLinkTest.php
   +++ b/core/modules/menu_link/tests/Drupal/menu_link/Tests/Plugin/Core/Entity/MenuLinkTest.php
   
   +++ b/core/modules/menu_link/tests/Drupal/menu_link/Tests/Plugin/Core/Entity/MenuLinkTest.php
   @@ -8,6 +8,7 @@
   +use Drupal\Core\Routing\UrlMatcher;
   
   @@ -65,11 +66,11 @@ public function testFindRouteNameParameters() {
    
   -    $this->assertEquals(array('view.frontpage.page_1', array()), MenuLink::findRouteNameParameters('node'));
   -    $this->assertEquals(array('node_view', array('node' => '1')), MenuLink::findRouteNameParameters('node/1'));
   -    $this->assertEquals(array('node_edit', array('node' => '2')), MenuLink::findRouteNameParameters('node/2/edit'));
   +    $this->assertEquals(array('view.frontpage.page_1', array()), UrlMatcher::findRouteNameParameters('node'));
   +    $this->assertEquals(array('node_view', array('node' => '1')), UrlMatcher::findRouteNameParameters('node/1'));
   +    $this->assertEquals(array('node_edit', array('node' => '2')), UrlMatcher::findRouteNameParameters('node/2/edit'));
    
   -    $this->assertEquals(array(), MenuLink::findRouteNameParameters('non-existing'));
   +    $this->assertEquals(array(), UrlMatcher::findRouteNameParameters('non-existing'));
      }
   

   We should try to move the test given that this does not belong into the class anymore.

3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.php
   @@ -0,0 +1,37 @@
   +    if ($shortcut_set = shortcut_set_load($entity->bundle())) {
   ...
   +    if ($shortcut_set = shortcut_set_load($entity_bundle)) {
   

   Should we just use the entity manager, which is already part of the EntityAccessController

4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutFormController.php
   @@ -0,0 +1,180 @@
   +    $form_state['redirect'] = 'admin/config/user-interface/shortcut/manage/' . $entity->bundle();
   ...
   +    $form_state['redirect'] = array('admin/config/user-interface/shortcut/link/' . $this->entity->id() . '/delete');
   

   It would be great to already use redirect_form here.

5. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutPathValue.php
   @@ -0,0 +1,52 @@
   +use InvalidArgumentException;
   

   Afaik we don't use classes which are in the root namespace.

6. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetStorageController.php
   @@ -16,15 +22,60 @@
   +   * @var \Drupal\Core\Entity\EntityManager
   ...
   +   * @param \Drupal\Core\Entity\EntityManager $entity_manager
   ...
   +  public function __construct($entity_type, array $entity_info, ConfigFactory $config_factory, StorageInterface $config_storage, QueryFactory $entity_query_factory, UuidInterface $uuid_service, EntityManager $entity_manager) {
   

   We do now have an interface for the entity manager.

7. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
   @@ -55,15 +55,14 @@ function testShortcutLinkAdd() {
   +        'title' => $title,
   +        'path'  => $test['path'],
   

   It would be great if we could drop that odd spacing here.

8. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php
   @@ -108,30 +109,43 @@ function testShortcutLinkChangePath() {
   +  function testShortcutLinkChangeRoute() {
   

   Let's mark it public if we create a new function.

9. +++ b/core/modules/shortcut/shortcut.install
   @@ -5,23 +5,104 @@
   +  $schema['shortcut_property_data'] = array(
   

   node is using node_field_data instead of _property_data ... do you know which one is the recommended one?

10. +++ b/core/modules/shortcut/shortcut.install
    @@ -101,6 +182,143 @@ function shortcut_update_8001() {
    +/**
    + * Migrate shortcuts into their own storage tables.
    + */
    +function shortcut_update_8003() {
    

    I am wondering whether we need to put the update into a batch process given that some sites might use shortcuts for every user.

11. +++ b/core/modules/shortcut/shortcut.module
    @@ -400,19 +334,41 @@ function shortcut_valid_link($path) {
    +  $shortcut_links = NULL;
    

    Why not default to an empty array?

12. +++ b/core/modules/shortcut/shortcut.module
    @@ -400,19 +334,41 @@ function shortcut_valid_link($path) {
    +
    +  $shortcuts = \Drupal::entityManager()->getStorageController('shortcut')->loadByProperties(array('shortcut_set' => $shortcut_set->id()));
    +  foreach ($shortcuts as $shortcut) {
    +    $links[] = array(
    +      'title' => $shortcut->label(),
    +      'href' => $shortcut->path->value,
    +    );
    +  }
    

    theme_links should be able to render links using the route_name and parameters, so we could already set it to drop additional work in the future.

13. +++ b/core/modules/shortcut/shortcut.module
    @@ -434,10 +390,11 @@ function shortcut_preprocess_page(&$variables) {
    +    if (!($route_info = UrlMatcher::findRouteNameParameters($link))) {
    

    I really think that this should be part of the service, not backed into yet another static function.

14. +++ b/core/profiles/standard/standard.install
    @@ -63,24 +63,21 @@ function standard_install() {
    +    'title' => t('Add content'),
    ...
    +    'title' => t('All content'),
    

    Just a general question: Do we really want to translate the title here and not while rendering the output?

1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php
   @@ -0,0 +1,204 @@
   + *   base_table = "shortcut",
   + *   data_table = "shortcut_property_data",
   + *   translatable = TRUE,
   

   Yes, I think this should be shortcut_field_data.

2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php
   @@ -0,0 +1,204 @@
   +    foreach ($entities as $entity) {
   +      $route_parameters = $entity->route_parameters->value;
   +      $entity->route_parameters->value = unserialize($route_parameters);
   +    }
   

   I have no idea what this is doing now that route_parameters is a map_field... Seems like the unserialize should happen earlier, before it's assigned?

3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetStorageController.php
   @@ -16,15 +22,60 @@
   +  /**
   +   * Constructs a FieldStorageController object.
   +   *
   

   wrong class name.

4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetStorageController.php
   @@ -16,15 +22,60 @@
      protected function attachLoad(&$queried_entities, $revision_id = FALSE) {
        parent::attachLoad($queried_entities, $revision_id);
    
        foreach ($queried_entities as $id => $entity) {
   -      $links = menu_load_links('shortcut-' . $id);
   -      foreach ($links as $menu_link) {
   -        $entity->links[$menu_link->uuid()] = $menu_link;
   +      $shortcuts = $this->entityManager->getStorageController('shortcut')->loadByProperties(array('shortcut_set' => $id));
   +      foreach ($shortcuts as $shortcut) {
   +        $entity->links[$shortcut->uuid()] = $shortcut;
          }
        }
   

   FYI: A lot of attachLoad stuff in storage controllers is moving to entity classes in #2095283: Remove non-storage logic from the storage controllers

Great reviews, thanks guys! Fixed everything except the following:

Should we just use the entity manager, which is already part of the EntityAccessController

Sadly enough, the entity manager is not part of EntityAccessController.

I am wondering whether we need to put the update into a batch process given that some sites might use shortcuts for every user.

I wouldn't worry about it, this will be converted to a migration soon enough.

theme_links should be able to render links using the route_name and parameters, so we could already set it to drop additional work in the future.

I looked at theme_links() and it's checking for a 'path' property, so it doesn't look able to use route names and parameters.

Just a general question: Do we really want to translate the title here and not while rendering the output?

Until we introduce the mechanism for default content entities (with translation support), this is the best we got for having those titles translatable on l.d.o

Noticed a problem in the form controller, we didn't use the shortcut set storage anywhere and the parent class needs the entity manager.

Rerolled.

And again.

And again. Sad to see this one still in NR.

And again. Sad to see this one still in NR.

Let's see what we can do.

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutSetStorageController.php
@@ -7,26 +7,63 @@
+   *
+   * @var \Drupal\Core\Entity\EntityManager

We should typehind for the interface instead.

Note: This was not an actual review, but to tease a bit.

Looks like this needs a reroll, patch applies to 14336d94b8da95f2c2f26edb7ffb5c12d58a1212.

Rerolled and fixed the review from #57.

Yet Another Reroll. Critical bug crying for reviews here :)

This looks really rock-solid.

public function findRouteNameParamters() {
}

Still not sure whether the url matcher is the right place. Can we replace that functionality by just using \Drupal::service('router')->match? I see why it is here ...

protected $entityQueryFactory;

We don't use the query factory as far as I can see.

$path = \Drupal::urlGenerator()->getPathFromRoute

We could use \Drupal::url() directly.

d.o, what failed testing?

Still not sure whether the url matcher is the right place. Can we replace that functionality by just using \Drupal::service('router')->match? I see why it is here ...

As far as I see.. we can't. Here's what I get for \Drupal::service('router')->match('node/1');

Symfony\Component\Routing\Exception\ResourceNotFoundException: None of the routers in the chain matched this request GET node/1 HTTP/1.1 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8 Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.7 Accept-Language: en-us,en;q=0.5 Host: localhost User-Agent: Symfony/2.X in Symfony\Cmf\Component\Routing\ChainRouter->doMatch() (line 202 of D:\xampp\htdocs\drupal-8-dev\core\vendor\symfony-cmf\routing\Symfony\Cmf\Component\Routing\ChainRouter.php). 

We don't use the query factory as far as I can see.

That's right, fixed.

We could use \Drupal::url() directly.

Nope, we can't.. something to do with aliases.

@herom the patch failed initially and then I asked for a retest directly on qa.d.o

The real patch would be nice to have.

Without this patch we can not import shortcut sets (configuration) entities since they have a hard dependency on the menu_link (content) entities because they sort the UUIDs for them in their configuration file.

Looks RTBC except few nitpicks

1. +++ b/core/lib/Drupal/Core/Routing/UrlMatcher.php
   @@ -33,4 +33,30 @@ public function finalMatch(RouteCollection $collection, Request $request) {
   +  public function findRouteNameParameters($link_path) {
   ...
   +      $result = \Drupal::service('router')->matchRequest($request);
   

   It makes UrlMatcher depends on container/service?
   is it ok?

2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Controller/ShortcutSetController.php
   @@ -34,21 +34,22 @@ class ShortcutSetController extends ControllerBase {
   -        drupal_set_message(t('Added a shortcut for %title.', array('%title' => $link['link_title'])));
   ...
   +        drupal_set_message($this->t('Added a shortcut for %title.', array('%title' => $name)));
   ...
   -        drupal_set_message(t('Unable to add a shortcut for %title.', array('%title' => $link['link_title'])));
   ...
   +        drupal_set_message($this->t('Unable to add a shortcut for %path.', array('%path' => $link)));
   

   Any reason to mix title/path?

3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutAccessController.php
   @@ -0,0 +1,37 @@
   +  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
   +    if ($shortcut_set = shortcut_set_load($entity_bundle)) {
   

   $entity_bundle optional? maybe better to use here storage controller directly?

4. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutPath.php
   @@ -0,0 +1,42 @@
   +class ShortcutPath extends StringItem {
   ...
   +        'class' => '\Drupal\shortcut\ShortcutPathValue',
   

   It would be nice to have special PathValue in core :)

Thanks for reviewing!

1. Not sure about this one, I'll ask later today in the WSCCI meeting.
2. Fixed.
3. That's how the parent class sets it :) And injected the shortcut_set storage.
4. I guess if we introduce it here, we'll just need a followup issue to move it around. It's probably the issue about adding internal path support to the link field.

69: shortcut_rewrite-69.patch queued for re-testing.

I forgot to post back in this issue: opened #2153891: Add a Url value object for #68.1

Or, since this patch is pretty much RTBC at this point, we can just go ahead here with a @todo (see interdiff) and make #2153891: Add a Url value object a non-critical followup?

I don't think we should postpone this. And if the Url value object doesn't block anything, could just be major yeah.

Proper fix for #68.2.

Awesome! I see no other nitpicks to stop the critical

Great to see this one green. Original shortcut issue to add that module discussed a separate entity at one point, in case anyone wants to read back and wish they had a time machine.

Committed/pushed to 8.x, thanks!

Woot! I did a self-review on the patch in #76 and opened this followup: #2156923: Various code cleanups for the new shortcut entity

Wrote a change notice here: https://drupal.org/node/2156931

Sadly this has broken HEAD. :(

git revert d443c1

It seems this is a general problem with creating translatable content entities during installation. Opened an issue for it here #2156945: Clean up installer entity defaults following bugfix

I agree with the temporary revert :(

Edit: can someone unpublish the change record node?

Looking at the verbose results, the failed assertions happen on an installer page where there's an exception displayed at the bottom:

There are no fields available to insert with.

Excitingly thrown from core/lib/Drupal/Core/Database/Query/Insert.php. :) So something is going funky when installing standard in the installer test. I did double-check and standard installs fine when I install it for real in a browser or via drush si.

I unpublished the change record. Soon we will be able to revert it to a draft state instead. :) #2151041: Add a "draft" status for change records

I did double-check and standard installs fine when I install it for real in a browser or via drush si.

It breaks only when installing in another language than English. But we can continue this in the issue I opened for it :)

This should fix it. Thanks to @Berdir for the tip in #2156945: Clean up installer entity defaults following bugfix.

Can we merge #2156923: Various code cleanups for the new shortcut entity in this patch?

Oh nvm. We don't have the revert yet.

Confirmed that the change fixes the testfail, we have the other issue to try and improve it.

Cool, thanks for the fast turnaround, folks!

Committed and pushed #86 to 8.x.

@xjm or @webchick please publish back change notice https://drupal.org/node/2156931

Fixed. :)

FYI : #2354177: Shortcut::getRouteParams() should be named getRouteParameters() for consistency ?