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

Files: 
CommentFileSizeAuthor
#35 shortcut-1497380-35.patch66.91 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 50,831 pass(es). View
#35 interdiff.txt7.82 KBtim.plunkett
#31 1497380-shortcut-31.patch67.19 KBjibran
PASSED: [[SimpleTest]]: [MySQL] 49,678 pass(es). View
#26 1497380-shortcut-26.patch67.19 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 48,801 pass(es). View
#24 1497380-shortcut-24.patch67.12 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 48,126 pass(es). View
#24 1497380-interdiff-24.txt1.35 KBandypost
#19 1497380-shortcut-19.patch67.12 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 47,611 pass(es). View
#18 1497380-shortcut-18.patch65.59 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View
#18 1497380-interdiff-18.txt7.01 KBandypost
#14 1497380-shortcut-14.patch66.8 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 46,408 pass(es), 6 fail(s), and 0 exception(s). View
#14 1497380-interdiff-14.txt2.54 KBandypost
#9 1497380-shortcut-9.patch67 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 46,189 pass(es). View
#9 1497380-interdiff-9.txt3.76 KBandypost
#7 1497380-shortcut-7.patch66.45 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 42,251 pass(es), 5 fail(s), and 0 exception(s). View
#4 shortcut_sets.patch14.38 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 35,696 pass(es). View

Comments

Berdir’s picture

Assigned: Unassigned » Berdir
David_Rothstein’s picture

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.

Berdir’s picture

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

Berdir’s picture

Status: Active » Needs review
FileSize
14.38 KB
PASSED: [[SimpleTest]]: [MySQL] 35,696 pass(es). View

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.

heyrocker’s picture

Status: Needs review » Postponed

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

andypost’s picture

Status: Postponed » Needs work

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

andypost’s picture

Status: Needs work » Needs review
FileSize
66.45 KB
FAILED: [[SimpleTest]]: [MySQL] 42,251 pass(es), 5 fail(s), and 0 exception(s). View

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(-)

Status: Needs review » Needs work

The last submitted patch, 1497380-shortcut-7.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.76 KB
67 KB
PASSED: [[SimpleTest]]: [MySQL] 46,189 pass(es). View

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

RobLoach’s picture

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

andypost’s picture

@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

tim.plunkett’s picture

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

andypost’s picture

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

andypost’s picture

FileSize
2.54 KB
66.8 KB
FAILED: [[SimpleTest]]: [MySQL] 46,408 pass(es), 6 fail(s), and 0 exception(s). View

Status: Needs review » Needs work
Issue tags: -Configuration system, -Config novice

The last submitted patch, 1497380-shortcut-14.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +Configuration system, +Config novice

The last submitted patch, 1497380-shortcut-14.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
7.01 KB
65.59 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module. View

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

andypost’s picture

Issue tags: +Configurables
FileSize
67.12 KB
PASSED: [[SimpleTest]]: [MySQL] 47,611 pass(es). View

And again entity plugin was lost in #18

Status: Needs review » Needs work

The last submitted patch, 1497380-shortcut-19.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

#19 is final one that needs review

andypost’s picture

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

andypost’s picture

Issue summary: View changes

Updated issue summary.

znerol’s picture

+/**
+ * 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.

andypost’s picture

FileSize
1.35 KB
67.12 KB
PASSED: [[SimpleTest]]: [MySQL] 48,126 pass(es). View

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

Berdir’s picture

Assigned: Berdir » Unassigned
andypost’s picture

FileSize
67.19 KB
PASSED: [[SimpleTest]]: [MySQL] 48,801 pass(es). View

Re-roll

moshe weitzman’s picture

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.

andypost’s picture

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

heyrocker’s picture

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

heyrocker’s picture

Status: Needs review » Needs work
jibran’s picture

Status: Needs work » Needs review
FileSize
67.19 KB
PASSED: [[SimpleTest]]: [MySQL] 49,678 pass(es). View

Reroll

jibran’s picture

Status: Needs review » Needs work
Issue tags: -Configuration system
jibran’s picture

Issue tags: +Configuration system

re-tagging

andypost’s picture

Status: Needs work » Needs review

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

tim.plunkett’s picture

Title: Convert shortcut sets to config system » Convert shortcut sets to ConfigEntity
Component: configuration system » configuration entity system
FileSize
7.82 KB
66.91 KB
PASSED: [[SimpleTest]]: [MySQL] 50,831 pass(es). View

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

andypost’s picture

Status: Needs review » Reviewed & tested by the community

@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

webchick’s picture

Title: Convert shortcut sets to ConfigEntity » Change notice: Convert shortcut sets to ConfigEntity
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

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.

andypost’s picture

Status: Active » Needs review
Issue tags: -Needs change record
andypost’s picture

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

Can we close this critical?

andypost’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture

Title: Change notice: Convert shortcut sets to ConfigEntity » Convert shortcut sets to ConfigEntity
Priority: Critical » Normal
Status: Needs review » Fixed

Change notice looks good.

Status: Fixed » Closed (fixed)

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

David_Rothstein’s picture

David_Rothstein’s picture

Issue summary: View changes

Updated issue summary.