In #1497380: Convert shortcut sets to ConfigEntity a new entity type was introduced to represent shortcut sets.

However, the machine name of this entity is 'shortcut'. This is extremely confusing, because throughout the administrative UI a clear distinction is made between shortcut sets (the group of shortcuts) and shortcuts themselves (the individual menu links within the set). I came across this while trying to understand the code, and it took me a while to understand that the code I was reading was actually working with shortcut sets rather than shortcuts themselves.

The entity type should be changed to 'shortcut_set' instead, since that's what it represents.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

David_Rothstein’s picture

Title: 'shortcut' entity is very confusing and should be renamed to 'shortcut_set' » 'shortcut' entity type is very confusing and should be renamed to 'shortcut_set'

More accurate title, I suppose.

andypost’s picture

Original intent was to make the name shorter to type less symbols for entity_load() and friends so DX++.
Shortcut set is represented by shortcut config entity and holds menu_link entities within

amateescu’s picture

Priority: Normal » Major
Status: Active » Needs review
FileSize
25.9 KB

I disagree with #2, this is very much a DX-- because of the huge confusion it causes. Plus, we need the 'shortcut' entity type namespace available in order to decouple shortcuts from menu links in #2021779: Decouple shortcuts from menu links.

I'm gonna call it a major because it actually blocks a critical now.

Status: Needs review » Needs work

The last submitted patch, shortcut_set_rename.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.37 KB
27.27 KB
dawehner’s picture

Technical this is an API change, but from my perspective I can't imagine that many people are using the shortcut API.
Just in general the rename is really great, as it removes confusion!

+++ b/core/modules/shortcut/shortcut.installundefined
@@ -8,17 +8,6 @@
 /**
- * Implements hook_uninstall().
- */
-function shortcut_uninstall() {
-  // Delete the menu links associated with each shortcut set.
-  // @todo find a way to clean-up associated menu links.
-  /*foreach (entity_load_multiple('shortcut') as $shortcut_set) {
-    menu_delete_links('shortcut-' . $shortcut_set->id());
-  }*/
-}

Removing this @todo seems to be a problem?

amateescu’s picture

FileSize
26.62 KB

Oops, that slipped in from another issue. No interdiff because I just manually removed that hunk from the patch.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you

Status: Reviewed & tested by the community » Needs work

The last submitted patch, shortcut_set_rename-7.patch, failed testing.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community

That was a random failure, setting back the status.

Dries’s picture

Title: 'shortcut' entity type is very confusing and should be renamed to 'shortcut_set' » [ChangeNotice] 'shortcut' entity type is very confusing and should be renamed to 'shortcut_set'
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

I'm not sure this is truly a major bug. I don't believe it actually blocks the critical bug. It's a 'soft blocker' at best. I've committed it to 8.x nonetheless.

xjm’s picture

Issue tags: +Approved API change

We agreed that the API change was very minor.

oddz’s picture

I believe I'm receiving an error relating to changing the id of this entity.

InvalidArgumentException: The entity (shortcut) did not specify a access. in Drupal\Core\Entity\EntityManager->getControllerClass() (line 158 of /Users/username/Sites/drupal/d8-2/drupal/core/lib/Drupal/Core/Entity/EntityManager.php).

Upon pulling the latest files from git the php directory was deleted and all cache_* tables cleared. Still have this issue. So somewhere the getControllerClass method is being called using the old name of shortcut instead of shortcut_set. At least I believe that is what is happening.

oddz’s picture

I figured it out. It appears that the router table (cache) had to be rebuilt. However, merely deleting everything from the router table just resulted in more errors. There is an actual function that needs to be called to rebuild the table. Also, a user is required otherwise permission errors will occur. So I created the below script that seems to rebuild the router table router.

[php]


/**
 * Force rebuild of router. This could probably also be used to
 * flush the drupal cache as well though that is not tested.
 */ 
   
require_once __DIR__ . '/core/includes/bootstrap.inc';

// Full bootstrap  
drupal_bootstrap(DRUPAL_BOOTSTRAP_FULL);
	
// Get DIC
$container = drupal_container();
	
// Get user controller
$userController = $container->get('plugin.manager.entity')->getStorageController('user');
	
// Get user entity for admin account
$account = $userController->load(1)->getBCEntity();

// Set global user	
global $user;
$user = $account;

// Rebuild router	
menu_router_rebuild();
	
/*
 * This would ideal but results in undefined session function error.
 * I'm not completely certain why but I would assume the session has not not been initiated
 * but don't really care to investigate any further. It is probably safe to say that
 * if this were to work it would be compeltely safe to clear the entire cache. Instead
 * though I'm calling the function that I need without setting any session data which
 * seems to function fine for rebuilding the router though it may not for other things.
 */
// Finalize login
//user_login_finalize($account);
  

[/php]

tim.plunkett’s picture

xjm’s picture

Category: bug » task
Priority: Major » Critical
amateescu’s picture

Title: [ChangeNotice] 'shortcut' entity type is very confusing and should be renamed to 'shortcut_set' » 'shortcut' entity type is very confusing and should be renamed to 'shortcut_set'
Category: task » bug
Priority: Critical » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Updated the existing change notice to reflect the new name of the entity type:

https://drupal.org/node/1895936/revisions/view/2537192/2778059

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