Support from Acquia helps fund testing for Drupal Acquia logo

Comments

floretan’s picture

Updated patch with the following changes:
- Convert shortcut sets back to object on rebuild.
- Rename the default hook to hook_shortcut_default_shortcut_set() to avoid conflict with http://api.drupal.org/api/drupal/modules--shortcut--shortcut.api.php/fun....

irakli’s picture

Status: Needs review » Closed (won't fix)

Features export of a third-party module should be implemented as part of that module using Features API or (preferably) CTools Exportable API

DamienMcKenna’s picture

Status: Closed (won't fix) » Active

Shortcut sets are core Drupal functionality and should be handled out of the box by Features.

Dave Reid’s picture

Status: Active » Needs review
DamienMcKenna’s picture

With some initial testing this seems to be working correctly.

Johnny vd Laar’s picture

There is one problem. It only works for updating existing shortcuts. It doesn't create new shortcut sets because it always sets the "set_name".

I wouldn't know how to fix this tho.

jmcneil’s picture

subscribe

febbraro’s picture

Assigned: Unassigned » febbraro
nmc’s picture

subscribe

febbraro’s picture

Assigned: febbraro » Unassigned
DamienMcKenna’s picture

There are two tables involved in the shortcuts system thus two pieces to the export:

  1. shortcut_set table - stores the primary record for the shortcuts, with primary key is the 'set_name' field. These are exported to the MYMODULE.features.shortcut_set.inc file.
  2. menu_links table - stores each of the shortcut links, with menu_links.menu_name=shortcut_set.set_name. These are handled the same as any other menu links thus are exported to the MYMODULE.features.menu_links.inc file.

The second part works as well as any other exported menu - there are some occasional glitches, but the first part is the custom part of this patch and needs some thorough testing.

DamienMcKenna’s picture

Am I the only one who wishes this made each link available separately, similar to the menu system?

javi-er’s picture

I've tested the patch on #1 with a fresh install of the latest release of Drupal and Features and it works well for me.

budda’s picture

I exported some shortcuts on the default shortcut-set to a feature and activated the feature on a fresh Drupal 7 install. Nothing appeared. I performed a drush cc all and was greeted with errors regarding a clash:

WD php: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'shortcut-set-1'  [error]
for key 'PRIMARY': INSERT INTO {shortcut_set} (set_name, title) VALUES (:db_insert_placeholder_0,
:db_insert_placeholder_1); Array
(
    [:db_insert_placeholder_0] => shortcut-set-1
    [:db_insert_placeholder_1] => Default
)
 in drupal_write_record() (line 6868 of /home/quickstart/websites/example.local/includes/common.inc).
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'shortcut-set-1' for key 'PRIMARY': INSERT INTO {shortcut_set} (set_name, title) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array
(
    [:db_insert_placeholder_0] => shortcut-set-1
    [:db_insert_placeholder_1] => Default
)
 in drupal_write_record() (line 6868 of /home/quickstart/websites/example.local/includes/common.inc).

Should a REPLACE INTO / UPDATE be done if an INSERT fails?

budda’s picture

Status: Needs review » Needs work

I renamed the default shortcut set to something custom, exported to a feature and activated the feature on a fresh site install. However the export still uses the machine name of "shortcut-set-1" which causes the same DB errors as above.

scor’s picture

reroll of #1 which worked for me.

itaine’s picture

@scor could you reroll another one up against rc2?

lee20’s picture

Status: Needs work » Needs review
FileSize
3.89 KB

Attached is a patch re-rolled against rc2

coredumperror’s picture

I tried out this patch, and at first it wouldn't work because the file paths are wrong. They appear to be relative to the drupal base path, instead of the module base path.

Once I got that worked out, though, I found that the export and load functionality works great except for one small but critical problem. It saves all the data in the export, but on import it doesn't create the row in the shortcut_set table. It does, however, create the menu_links rows correctly. Thus, manually adding the appropriate row to shortcut_set fixes the import.

I looked through the code in the patch to see if I could solve the problem, but I'm not familiar enough with the Features API to see what's wrong.

coredumperror’s picture

Ah ha! Upon further investigation, I discovered the problem. Unfortunately, the fix is not very straightforward due to a quirk in the shortcut_set_save() API (as described here #1175700: shortcut_set_save() "set_name" documentation is confusing, and passing in a nonexistent set name creates orphan menu links).

On line 87 of features.shortcut.inc the shortcut_set_features_rebuild() function calls shortcut_set_save(&$shortcut_set) with an object much like this for $shortcut_set:

stdClass Object
(
    [set_name] => shortcut-set-2
    [title] => Admin
)

Unfortunately, shortcut_set_save() assumes that if you've provided a set_name value, you want to do an update to an existing shortcut set, rather than adding a new one (see comment #2 on #1175700: shortcut_set_save() "set_name" documentation is confusing, and passing in a nonexistent set name creates orphan menu links). Since there is not yet any shortcut set named 'shortcut-set-2', it doesn't update anything. And due to another bug #602190: drupal_write_record() unable to determine if changes have been made., it reports that it did the update, even though the DB didn't change.

The solution in my patch is probably not optimal, but it works. It calls shortcut_set_save(&$shortcut_set) with:

stdClass Object
(
    [title] => Admin
)

Then grabs the auto-generated set_name that gets added by reference to $shortcut_set. It then updates all the rows in menu_links which had the exported set_name, changing them to the auto-generated set_name.

Depending on how the exporting user had his site set up, these two versions of set_name may very well be the same string; but since we can't be sure of that, this roundabout method seems necessary.

nevergone’s picture

What would there be need for in order for the solution of the issue to go on?

mpotter’s picture

Patch should be re-rolled against the 2.x-beta and also tested for the problems raised in #19. Thus, as it's marked, it "Needs Review".

cedewey’s picture

I've rerolled the patch from #20 against features-7.x-2.0-beta and am not seeing the shortcut sets reflected when I export the feature and enable it on another site. Perhaps I'm missing a step though? What component should these be in? Currently I'm including the Block settings for Shortcuts and the management: --Shortcuts Menu Link.

discipolo’s picture

Version: 7.x-1.x-dev » 7.x-2.x-dev
FileSize
4.25 KB

here is the rerolled patch against 2.x-dev and it seems to work

carsonblack’s picture

Status: Needs review » Reviewed & tested by the community

Applied patch cleanly and shortcut set and menu items are exported into feature code.

coredumperror’s picture

Have you tried importing the saved shortcut sets, though? That was the main problem I had with the system. Drupal 7 implemented the shortcut set system in a way that doesn't lend itself well to importing a shortcut set into a site that already has other shortcut sets.

mpotter’s picture

Status: Reviewed & tested by the community » Needs review

Well, if people haven't tested the *importing* of shortcuts, then I need to move this back to "Review" state.

MiroslavBanov’s picture

I have tested it, importing does work, but I got bizarre issues when the feature was part of the install profile, and I installed different language, and I had translation string in the ".po" file for the string "Default". After profile installation, the feature was "Overridden", and I needed to revert it again. I want to be able to install my profile with a choice language if possible.

matthewmessmer’s picture

Issue summary: View changes

#24 works for me on the latest 2.x-dev branch with a few issues.

It failed to properly import shortcuts saved to the "Default" set, but it worked for any others sets I created.

A standard features reset works as expected and added new shortcuts to the original shortcut set, but if I did a feature reset with --force, a new shortcut set with the same name would be created and the original one would remain untouched.

hefox’s picture

Status: Needs review » Needs work

Needs work based on 29

mkhamash’s picture

I have edited patch number #24 to work as expected with the Drupal core patch for shortcut_set_save() issue #1175700-19: shortcut_set_save() "set_name" documentation is confusing, and passing in a nonexistent set name creates orphan menu links, I think we should go with this path instead of workarounds since the core issue is committed to Drupal 8 and only needs to be back-ported to D7.

hefox’s picture

Title: Export shorcuts sets from the new core Shortcut module » Export shorcuts sets from the core Shortcut module
chrisgross’s picture

#31 worked for me, but ONLY for the default shortcut set. Any additional shortcut sets would not import. That being said, the default set will only add shortcuts, but does not remove the few default shortcuts provided by core.

chrisgross’s picture

Still needs work, but here's #31 re-rolled against 2.7

chrisgross’s picture

chrisgross’s picture

Whoops fixed missing 'contact' module export that must have been added to features recently.

Dave Reid’s picture

Revised patch that must be used in combination with #1175700: shortcut_set_save() "set_name" documentation is confusing, and passing in a nonexistent set name creates orphan menu links and removes the unnecessary db_update('menu_links') since it was a no-op (updating field to the same value).

Dave Reid’s picture

I don't think we even need menu_cache_clear_all() anymore either.

froboy’s picture

I tested #38 and it works for the Default set but not others. As described in #33 the second set exports, but doesn't import correctly.

To the second part of #33, however, seems to be related to a long-term issue where Features can't remove menu items. I'd recommend that not be considered as a requirement here, but that the minimum viable patch be able to export and successfully import any number of shortcut sets.

EDIT: Inspecting the target db shows only "shortcut-set-1" in the "shortcut_set" table. If I manually add an entry for "shortcut-set-2" everything shows up, so it looks like registering the set is the only missing piece here.

EDIT again: After an hour of being stupid, I read #37

froboy’s picture

Status: Needs work » Reviewed & tested by the community

With the patch in #37, this looks ready to go.

The last submitted patch, 31: export_shorcuts_sets-986968-31.patch, failed testing.

The last submitted patch, 31: interdiff-98696-24-31.patch, failed testing.

mpotter’s picture

Status: Reviewed & tested by the community » Postponed

We can't mark this RTBC if it depends on a core Drupal patch that isn't committed yet. Thus, marking this as Postponed until the Drupal core issue mentioned in #37 is applied. Once that is done, somebody can test again and mark this RTBC at that time.

scotwith1t’s picture

Title: Export shorcuts sets from the core Shortcut module » Export shortcuts sets from the core Shortcut module
joegraduate’s picture

Issue tags: +Needs reroll

Patch #38 no longer cleanly applies to 7.x-2.x.

jwmoore1’s picture

Rerolled patch #38 and resolved conflicts, which were just related to the comments in the features.module file changing.