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.
Comment | File | Size | Author |
---|---|---|---|
#7 | shortcut_set_rename-7.patch | 26.62 KB | amateescu |
#5 | shortcut_set_rename-5.patch | 27.27 KB | amateescu |
#5 | interdiff.txt | 1.37 KB | amateescu |
#3 | shortcut_set_rename.patch | 25.9 KB | amateescu |
Comments
Comment #1
David_Rothstein CreditAttribution: David_Rothstein commentedMore accurate title, I suppose.
Comment #2
andypostOriginal 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 holdsmenu_link
entities withinComment #3
amateescu CreditAttribution: amateescu commentedI 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.
Comment #5
amateescu CreditAttribution: amateescu commentedComment #6
dawehnerTechnical 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!
Removing this @todo seems to be a problem?
Comment #7
amateescu CreditAttribution: amateescu commentedOops, that slipped in from another issue. No interdiff because I just manually removed that hunk from the patch.
Comment #8
dawehnerThank you
Comment #10
amateescu CreditAttribution: amateescu commentedThat was a random failure, setting back the status.
Comment #11
Dries CreditAttribution: Dries commentedI'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.
Comment #12
xjmWe agreed that the API change was very minor.
Comment #13
oddz CreditAttribution: oddz commentedI 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.
Comment #14
oddz CreditAttribution: oddz commentedI 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]
[/php]
Comment #15
tim.plunkettSee #2010456: Drupal core needs to include a recovery script and http://drupal.org/documentation/rebuild, this is normal.
Comment #16
xjmComment #17
amateescu CreditAttribution: amateescu commentedUpdated the existing change notice to reflect the new name of the entity type:
https://drupal.org/node/1895936/revisions/view/2537192/2778059