The shourtcuts at admin/config/user-interface/shortcut need to be converted to use the new configuration system.
Part of #1802750: [Meta] Convert configurable data to ConfigEntity system
Tasks
- Create a default config file and add it to the shortcut module.
- Convert the admin UI in shortcut.admin.inc to read to/write from the appropriate config.
- Convert any code that currently accesses the {shortcut_set} table to use the config system.
- Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the {shortcut_set} table.
This would be a good task for someone wanting to get up to speed on how the new config system works. The implementation would be really similar to the one used in #1479466: Convert IP address blocking to config system. Feel free to ping me on IRC if you need help.
Follow-ups
#1899682: Add upgrade path tests for shortcut module
#1895938: Add label() method to ShortcutSet
Related issues
#1814916: Convert menus into entities
#916388: Convert menu links into entities
Comment | File | Size | Author |
---|---|---|---|
#35 | shortcut-1497380-35.patch | 66.91 KB | tim.plunkett |
#35 | interdiff.txt | 7.82 KB | tim.plunkett |
#31 | 1497380-shortcut-31.patch | 67.19 KB | jibran |
#26 | 1497380-shortcut-26.patch | 67.19 KB | andypost |
#24 | 1497380-shortcut-24.patch | 67.12 KB | andypost |
Comments
Comment #1
BerdirComment #2
David_Rothstein CreditAttribution: David_Rothstein commentedDoes it actually make sense to convert this without converting the entire {menu_links} table also? Otherwise, it seems like the shortcut sets would be "configuration" that has no real meaning when transferred from one site to another.
Sorry in advance if this was already discussed somewhere else.
Comment #3
BerdirNo sure. I mean, there are also references to users, this has no meaning when moving it around either :) And heyrocker said that part doesn't matter. We could possibly replace it with uuid's or so when moving around :)
Comment #4
BerdirAttaching a first patch that seems to works, no upgrade path yet.
Bunch of hackish stuff, but the tests seem to pass, so it's enough for a first review.
The default shortcut set stuff needs to be taken care of, the patch removes the special handling (unable to delete it) after some discussion with heyrocker but this means the the default shortcut set function needs to be updated to deal with the fact that there might not be a shortcut set named shortcut-set-1.
Unassigning myself as I might not have time to continue this for the moment.
Comment #5
gddI can only blame the lack of sleep I was suffering from at DrupalCon, but I agree now that this is possibly a bad idea. First off, the uid stuff isn't going to be transportable until we get UUIDs in entities, no idea how I didn't see that. In some ways this mirrors the issues in #1479454: Convert user roles to configurables in that part of it specifically relates to users (which shortcut set each user is using) and some of it is very clearly configuration (what shortcut sets are available.) Does it make more sense for a user's choice of set to be a field on the user entity, and the list of available sets to be in configuration?
As far as the menu links, I was under the impression that there is interest in them becoming entities in Drupal 8 #916388: Convert menu links into entities.
So maybe it is a good idea to just postpone this for now and see how it plays out.
Comment #6
andypostNow we have uuids everywhere. Comming from #1811640: Convert shortcuts into configuration
Upcoming tasks:
- Convert to entity
- use EnityFormController for administration
- use EntityListController and maybe view for listings
Related issues:
- implement link to menu-item as entity reference
#1801304: Add Entity reference field
#916388: Convert menu links into entities
Comment #7
andypostIt's not novice patch... mostly everything is converted.
Suppose other forms could be converted in follow up because there's collisions with #916388-50: Convert menu links into entities
Shortcut now config entity.
Own storage controller is needed to operate with assigned menu links, but it's loading implemented in hook_EntutyType_load() shortcut_shortcut_load()
I found constant is useless so changed all 4 places to 'default'
Comment #9
andypostFixed broken uninstall with @todo and commented out deletion of related menu-links
entity system is inaccessible at this time throwing
Fatal error: Class name must be a valid object or a string in .../core/includes/entity.inc on line 315
Comment #10
RobLoachCan we get some encapsulation here? Get/setters for each if they're public properties. If $id, $uuid and $label are persistent for each ConfigEntityBase class, then we should introduce a ConfigEntity class with these properties.
Comment #11
andypost@Rob Loach, methods id() uuid() label() declared in /core/lib/Drupal/Core/Entity/EntityInterface.php and implemented in /core/lib/Drupal/Core/Entity/Entity.php there's already enough of incapsulation
Comment #12
tim.plunkett#10, they don't have to be named those. It's dependent on the entity keys specified in hook_entity_info(). Perhaps $uuid/$label could be shared, but $id is often $name or $machine_name.
Comment #13
andypost#9: 1497380-shortcut-9.patch queued for re-testing.
Comment #14
andypostRe-roll after #1763974: Convert entity type info into plugins
Comment #16
tim.plunkett#14: 1497380-shortcut-14.patch queued for re-testing.
Comment #18
andypostRe-roll with fixes, last patches are missed some hunks
Comment #19
andypostAnd again entity plugin was lost in #18
Comment #21
andypost#19 is final one that needs review
Comment #22
andypost#19: 1497380-shortcut-19.patch queued for re-testing.
Comment #22.0
andypostUpdated issue summary.
Comment #23
znerol CreditAttribution: znerol commentedtest entity?
#402760: Regression: Turn "delete tabs" back into buttons suggests that delete should become a tab on all core entities.
There is no
drupal_write_record
above.Comment #24
andypost1 and 3 fixed, see interdiff.
2 is debatable so is not included to minify a patch changes, see opposite opinions #1834002: Configuration delete operations are all over the place
Comment #25
BerdirComment #26
andypostRe-roll
Comment #27
moshe weitzman CreditAttribution: moshe weitzman commentedJust like #5, I'm pretty uneasy about this. For me, user specific shortcut sets are definately content, not configuration. They could be represented as with a multiple value entity reference Field on the user entity (since menu links are now entities). Sitewide or shortcut sets could be config but I don't think they have to be. I feel like we should postpone this to D9 if ever.
Comment #28
andypostshortcut set already has a machine name, and there's not a lot of them mostly used on sites.
Suppose this small feature could be a just task to make set names are transportable
Comment #29
gddYeah lets make the sitewide sets configuration, then figure out a way to move the user-specific sets onto the user entity. I think that sounds like a plan.
Comment #30
gddComment #31
jibranReroll
Comment #32
jibranComment #33
jibranre-tagging
Comment #34
andypostI think #31 is good to be commited. The discussion about user-specific sets should be done in follow-up
EDIT Also Menus already are config entities so better to make all menu-creating staff the same
Comment #35
tim.plunkettCleaned up some of the docs and methods, adding in parent:: calls where appropriate.
This looks pretty much done to me, I'd RTBC but I just made all these tweaks :)
Comment #36
andypost@tim thanx for clean-up
Suppose it's RTBC time. A follow up for entity_uri probably is #1783964: Allow entity types to provide menu items
Do we have an issue to unify this and document which kind is preferable?
Suppose while we have Entities "a bit of decoupled" from hook_menu() better to leave this in procedural part
Comment #37
webchickThis looks consistent with what we've done elsewhere with config entities. It's pretty painful though that you can export the name/uuid/etc. of a shortcut set, but not the actual links and stuff (the part that's actually useful). :( Doesn't look like #916388: Convert menu links into entities is going to help in this regard, either.
However, it's generally good to bring Shortcut inline with other modules in D8, so...
Committed and pushed to 8.x. Thanks!
This will need a change notice.
Comment #38
andypostCreated change notice http://drupal.org/node/1895936
And filed follow-up #1895938: Add label() method to ShortcutSet
Comment #39
andypostAnother follow-up #1899682-2: Add upgrade path tests for shortcut module
Can we close this critical?
Comment #39.0
andypostUpdated issue summary.
Comment #40
BerdirChange notice looks good.
Comment #42
David_Rothstein CreditAttribution: David_Rothstein commentedFollowup: #2036629: 'shortcut' entity type is very confusing and should be renamed to 'shortcut_set'
Comment #42.0
David_Rothstein CreditAttribution: David_Rothstein commentedUpdated issue summary.