Convert shortcut sets to ConfigEntity

Does 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.

No 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 :)

Attaching 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.

Now 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

It'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'

diffstat 1497380-shortcut-7.patch 
 modules/shortcut/config/shortcut.set.default.yml                   |    2 
 modules/shortcut/lib/Drupal/shortcut/Shortcut.php                  |   38 +
 modules/shortcut/lib/Drupal/shortcut/ShortcutFormController.php    |   95 ++
 modules/shortcut/lib/Drupal/shortcut/ShortcutListController.php    |   61 +
 modules/shortcut/lib/Drupal/shortcut/ShortcutStorageController.php |   72 ++
 modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutLinksTest.php   |   16 
 modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutSetsTest.php    |   75 +-
 modules/shortcut/lib/Drupal/shortcut/Tests/ShortcutTestBase.php    |   35 -
 modules/shortcut/shortcut.admin.inc                                |  247 +------
 modules/shortcut/shortcut.install                                  |   83 +-
 modules/shortcut/shortcut.module                                   |  330 ++++------
 profiles/standard/standard.install                                 |    4 
 12 files changed, 576 insertions(+), 482 deletions(-)

Fixed 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

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Shortcut.phpundefined
@@ -0,0 +1,38 @@
+  /**
+   * The machine name for the configuration entity.
+   *
+   * @var string
+   */
+  public $id;

Can 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.

@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

#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.

#9: 1497380-shortcut-9.patch queued for re-testing.

Re-roll after #1763974: Convert entity type info into plugins

#14: 1497380-shortcut-14.patch queued for re-testing.

Re-roll with fixes, last patches are missed some hunks

And again entity plugin was lost in #18

#19 is final one that needs review

#19: 1497380-shortcut-19.patch queued for re-testing.

Updated issue summary.

+/**
+ * Form controller for the test entity edit forms.
+ */
+class ShortcutFormController extends EntityFormController {

test entity?

+  /**
+   * Returns an array of supported actions for the current entity form.
+   */
+  protected function actions(array $form, array &$form_state) {
+    // Disable delete of default shortcut set.
+    $actions = parent::actions($form, $form_state);
+    $actions['delete']['#access'] = shortcut_set_delete_access($this->getEntity($form_state));
+    return $actions;
+  }

#402760: Regression: Turn "delete tabs" back into buttons suggests that delete should become a tab on all core entities.

+      // Make sure that we have a return value, since if the links were updated
+      // but the shortcut set was not, the call to drupal_write_record() above
+      // would not return an indication that anything had changed.

There is no drupal_write_record above.

1 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

Re-roll

Just 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.

shortcut 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

Reroll

re-tagging

I 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

Cleaned 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 :)

@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

+++ b/core/modules/shortcut/lib/Drupal/shortcut/Plugin/Core/Entity/Shortcut.php
@@ -55,4 +54,17 @@ class Shortcut extends ConfigEntityBase {
+  /**
+   * Overrides \Drupal\Core\Entity\Entity::uri().
+   */
+  public function uri() {
+    return array(
+      'path' => 'admin/config/user-interface/shortcut/manage/' . $this->id(),

+++ b/core/modules/shortcut/shortcut.module
@@ -349,21 +349,6 @@ function shortcut_shortcut_load($entities) {
- * Entity URI callback.
...
-function shortcut_uri(Shortcut $shortcut) {

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

This 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.

Created change notice http://drupal.org/node/1895936

And filed follow-up #1895938: Add label() method to ShortcutSet

Another follow-up #1899682-2: Add upgrade path tests for shortcut module

Can we close this critical?

Updated issue summary.

Change notice looks good.

Followup: #2036629: 'shortcut' entity type is very confusing and should be renamed to 'shortcut_set'

Updated issue summary.