The documentation found here states that for the function shortcut_set_save(&$shortcut_set)
, the $shortcut_set object can contain three properties: 'title', 'set_name' and 'links'. Title and links seem to work fine, but set_name doesn't work at all, as far as I can tell. Here is a code snippet that I'm using to test:
$shortcut_set = new stdClass();
$shortcut_set->title = 'Dummy Shortcut Set';
$shortcut_set->set_name = 'dummy-set';
$shortcut_set->links = array(
array(
'link_path' => 'admin',
'link_title' => 'Admin',
),
);
shortcut_set_save($shortcut_set);
If I leave the set_name property out, then it auto-populates the set_name in the DB to something like "shortcut-set-4". But if I ever try to define the set_name, the shortcut_set table doesn't get populated at all.
Thanks!
Comments
Comment #1
aspilicious CreditAttribution: aspilicious commentedSrry for the late response but can you try this again without the '-' in the name? I didn't look at the code yet but it is maybe possible that those names get filtered somehow.
EDIT: hmm this shouldn't be a problem, looking at the code I can't see what possibly could go wrong... hmmm
Comment #2
swentel CreditAttribution: swentel commentedIf set_name is passed in, than the save function will try to update an existing record, it will not create a new set. However, menu links will be saved so there's stale data in the database if you use that function manually. We should prevent that.
Comment #3
swentel CreditAttribution: swentel commentedChanging version
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedThat makes sense to me. We should also probably clarify the function documentation.
For D8, it might also be a good idea to give the Shortcut module full "machine name" support; then this function could behave more like e.g. http://api.drupal.org/api/drupal/modules--filter--filter.module/function... or other functions, and you really would be able to pass in an arbitrary set name to save. That could be a separate issue, though.
Comment #5
caiovlp CreditAttribution: caiovlp commentedI created a patch to make the shortcut_set_save function create a new shortcut set if there isn't an existing one already for the scenario where set_name is specified. Also created a simpletest function to validate this scenario.
Comment #6
xjm#5: fix-shotcut-set-save-1175700-5.patch queued for re-testing.
Comment #7
Cameron Tod CreditAttribution: Cameron Tod commentedCode looks good style and functionality wise from here.
Comment #8
caiovlp CreditAttribution: caiovlp commentedSame patch, just switched $set_name and $default_links so that this is easily backportable to 7.x.
Comment #9
cweagansOh, good call. I'm going to move this back to RTBC - seems sane, tests pass, and code looks good
Comment #10
webchickNice catch, nice fix, and nice test! :) David Rothstein makes a good point in #4, but I think that can be a follow-up issue for D8.
Committed and pushed to 8.x. Moving to 7.x for backport.
Comment #11
ldweeks CreditAttribution: ldweeks commentedWell, I hope this isn't more trouble to you all than its worth. I have created a patch against D7 head using the patch in #8 above. It's the first time that I have attempted a core patch. Tests are new territory for me, but I did my best to port over the tests as well.
Thanks for the work on this!
Comment #12
ldweeks CreditAttribution: ldweeks commentedComment #14
webchickAwesome, ldweeks! Thanks for the patch!
The test failure is strange. The patch definitely looks right on visual inspection.
What might help to debug this is to turn on the "Testing" module on localhost, and run the test manually there. In the test results, you'll see links where it says "Verbose message" or something, where basically you can click back and forth to see what the testing framework "sees" as it's doing those tests. Hopefully that can help uncover where the issue is.
If you need any help, don't hesitate to drop by #drupal-contribute on IRC as well.
Comment #15
ldweeks CreditAttribution: ldweeks commentedTests pass on my local dev environment. Here goes...
Comment #16
mgifford15: fix-shortcut-set-save-1175700-10.patch queued for re-testing.
Comment #17
mgiffordStill seems to apply. I agree with @webchick on the visual review of the patch. Still have to look at the code/tests in the summary.
Comment #18
dcam CreditAttribution: dcam commented#15 has some code style issues that need fixing.
These lines are indented 3 spaces instead of 2.
These whitespace changes are out of scope.
Comment #19
mgiffordThanks!
Comment #20
dcam CreditAttribution: dcam commented@mgifford I'm probably being too picky since it was already committed to 8.x, but I'm not comfortable RTBCing this without a tests-only patch. It seems like this bug only pops up with custom code and can't be reproduced through the UI. Yet no one provided a tests-only patch. When someone comes along to commit this I don't want there to be doubt that there was a bug and the patch fixes it. Would you (or anyone else reading this) mind posting a tests-only patch for us?
Tagging as Novice for the addition of a tests-only patch.
Comment #21
dcam CreditAttribution: dcam commentedComment #22
amitgoyal CreditAttribution: amitgoyal commentedPlease review tests-only patch as per #20.
Comment #24
dcam CreditAttribution: dcam commentedThank you, @amitgoyal. I'm happy to RTBC this now.
I'm reposting #19 because I don't want Testbot to keep auto-retesting #22 and set the issue back to Needs Work every time. I am not the author of this patch and it should not be attributed to me.
Comment #27
dcam CreditAttribution: dcam commentedComment #30
dcam CreditAttribution: dcam commentedComment #33
dcam CreditAttribution: dcam commentedComment #35
David_Rothstein CreditAttribution: David_Rothstein commentedActually, I'm not sure this is safe for Drupal 7 - there is a fair amount of code which assumes the shortcut set machine names will all follow a particular pattern (and generally to make sure they are namespaced so as not to interfere with other modules that add menu links to the menu system). For example:
- https://api.drupal.org/api/drupal/modules!shortcut!shortcut.module/function/shortcut_set_get_unique_name/7
- https://api.drupal.org/api/drupal/modules!shortcut!shortcut.module/function/shortcut_set_name/7
I don't believe these are concerns for Drupal 8, but it is a problem in Drupal 7.
So perhaps we should just fix the actual bug here, which is this (from @swentel in #2):
And maybe also change the function definition to make clear that passing in a set name is only something you're allowed to do for existing shortcut sets (I can see how the current documentation is a bit unclear, but I don't believe it was ever intended that you can pass in an arbitrary set name and expect it to be used)...
Comment #36
David_Rothstein CreditAttribution: David_Rothstein commentedWell, I thought this might be a relatively easy fix, but trying it out it's not so trivial because drupal_write_record() returns SAVED_UPDATED whenever it tries to update an existing record in the database (even if the update doesn't change anything)... so it would require more effort to detect this situation.
But here's at least a patch that clarifies the documentation.
Comment #37
Dave ReidI frankly think the API in shortcut_set_save() is unacceptable with regards to not being able to provide a set_name value. It makes exporting shortcut sets via Features way more difficult since like all other APIs, we cannot provide this object a pre-determined machine name. Let's just fix it rather than documenting a frustrating API.
Comment #39
Dave ReidWith the test changes as well.
Comment #40
Dave ReidFixed logic error in shortcut_set_save()
Comment #43
Dave ReidSomething is massively broken with the testbot.
Comment #44
lpalgarvio CreditAttribution: lpalgarvio commentedI agree with Dave on #37.
A release can include a reference to this issue and possible incompatibilities.
Comment #45
Dave ReidThis is fully ready for review, has passing test coverage, and is the same fix as Drupal 8.
Comment #50
scotwith1tAny way this could be committed in D7 since it's already been committed in D8 and is the exact same change to the code? It's really nice to be able to export shortcuts in Features now! Blocker for #986968: Export shortcuts sets from the core Shortcut module Thanks @Dave Reid
Comment #51
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis looks relatively backwards-compatible to me and hopefully could get in. But I think #35 needs to be addressed somehow (via documentation changes in the functions mentioned there at the very least, and possibly code changes too).
Why is this being changed? It breaks the form structure for anyone altering the form, and doesn't seem related to this issue.
Comment #52
jollysolutionsComment #53
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedWhy did you mark this RTBC, given the reviews above?