shortcut_install() should be in standard_install()

Subscribing, because I agree, and would like to see this happen.

Without shortcut_install(), the module won't work (because there would be no default shortcut set). It would take some refactoring to change that.

On the other hand, it would be easy enough to remove the specific links that are installed and put them in the install profile instead... but then what would you get if you turn the module on later? An empty shortcut bar? We should have a reasonable user experience for people turning on the module post-installation also.

It should be very simple for an install profile to replace the default shortcut links with ones of its own. Something like the code below (and it would be the same code regardless of whether or not the shortcut module provides its own default links or leaves them empty).

It's possible that it's not the same code currently, but if so, that's a bug to be fixed - I think due to #609122: shortcut_set_save() no longer deletes existing links when a new set of links is passed in ?

  $shortcut_set = shortcut_set_load(SHORTCUT_DEFAULT_SET_NAME);
  $shortcut_set->links = array(
    array(
      'link_path' => 'link1-path',
      'link_title' => st('First link'),
      'weight' => -20,
    ),
    array(
      'link_path' => 'link2-path',
      'link_title' => st('Second link'),
      'weight' => -19,
    ),
  );
  shortcut_set_save($shortcut_set);

This issue would likely have fixed #1187906: Shortcut module cannot be installed via an install profile if the menu module wasn't installed first. If having a default shortcut set is required, an empty one could be created, or we'd need to account for no default set.

+++ b/modules/shortcut/shortcut.install
@@ -13,18 +13,7 @@ function shortcut_install() {
   // Create an initial default shortcut set.
   $shortcut_set = new stdClass();
   $shortcut_set->title = $t('Default');
...
   shortcut_set_save($shortcut_set);

+++ b/profiles/standard/standard.install
@@ -426,4 +426,20 @@ function standard_install() {
+  $shortcut_set = shortcut_default_set();

The shortcut set saved in shortcut_install() is not necessarily the shortcut_default_set().

I'm not sure what the expected behavior of that default installation set is, but I do wonder why it's not setting

$shortcut_set->set_name = SHORTCUT_DEFAULT_SET_NAME;

It looks like that is or was the intention, but I'm not sure.

+++ b/profiles/standard/standard.install
@@ -426,4 +426,20 @@ function standard_install() {
+      'link_path' => 'node/add', ¶
+      'link_title' => $t('Add content'), ¶
...
+      'link_path' => 'admin/content', ¶
+      'link_title' => $t('Find content'), ¶

Trailing white-space.

11 days to next Drupal core point release.

I'd highly prefer this patch over #1376150: Shortcut module installation fails in tests when installed later (due to menu system not saving menu links correctly) (which kinda opened a rabbit hole on its own)

The essential question to ask here is:

Does Shortcut module require at minimum one shortcut set to work (without errors)?

- "No" means that the "default" shortcut set should be configured by an installation profile.

- "Yes" means that shortcut_install() has to create an empty set.

Let's see.

Though frankly, if the answer is "Yes", then Shortcut module's design ought to be fixed, IMHO.

Alright, Shortcut requires a default set to exist, so this stuff needs to be split appropriately.

What happens if you install with a profile that does not create the default shortcut set and enable the shortcut module after installation?

With the latest patch, Shortcut module always creates an initial (empty) default shortcut set, which is required for it to operate. Thus, there is always a default shortcut set.

The only part that is moved into the install profile is the creation of links in the default shortcut set, because those links heavily depend on which other modules are installed. E.g., Standard profile can add links to content admin pages there, because it installs Node module. Other profiles may not install Node module.

I think that just simply makes sense. :) The remaining task here would be to fix the shortcut test failures - it looks like one of the test cases assumes the default links to be there.

Oh yes, the solution makes sense. The empty shortcut set on shortcut.module install will avoid the problem nicely.

I'll have a look at the failing tests. Looks like testShortcutQuickLink() assumes that the default shortcut set is populated, but since we no longer use the standard install profile for tests, it's not.

This could be one solution.

#20: drupal8.shortcut-install.20.patch queued for re-testing.

Did some manual testing while I was at it, as my D8 dev site was hosed (for other reasons). Installing with the standard profile works as expected, as well as installing with the minimal profile and enabling the shortcut module. I can add, edit and delete shortcuts from the default shortcut set.

Awesome, thanks! :)

David's concerns are valid, but just so you know, without the patch, this is the default behaviour when you enable this module from minimal profile, without this patch:

I'm sure someone had some kind of good reason for making the Shortcut module not have Toolbar as a dependency, but it's already a total WTF situation. So I'm not sure we should be as concerned about this particular aspect. OTOH, it's worth looking into.

When you then enable Toolbar without this patch, you get what looks like the standard default links:

With this patch, you get this:

I actually think that's sufficient for getting people on their way. While the shortcut bar is empty, the link to edit the shortcuts is plainly visible, ripe for the clicking.

This patch actually does a good job of removing some extremely weird code that needs 30 lines of comments to explain in favour of reasonable lines of code that make a lot sense. I think this is worth doing, but #701974: Add hook for example content might be a good general issue to work on to solve this across modules which might not be as easy to "start from scratch" for an end user (Forum comes to mind).

Therefore, committed and pushed to 8.x. We'll need a change notice since profiles depending on this behaviour will need to be adjusted accordingly.

I'm sure someone had some kind of good reason for making the Shortcut module not have Toolbar as a dependency

#1262966: Make the Shortcut module depend on the Toolbar module (closed - won't fix) :)

#701974: Add hook for example content might be a good general issue to work on to solve this across modules which might not be as easy to "start from scratch" for an end user

As long as issues like that exist, I don't actually have any real objections here. (Though I don't know if that will get implemented any time soon...)

We discussed this in IRC, and it doesn't seem to make sense to create a change notice for this, since install profiles will have to be completely rewritten for D8 either way.