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!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

Srry 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

swentel’s picture

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

swentel’s picture

Version: 7.x-dev » 8.x-dev

Changing version

David_Rothstein’s picture

Issue tags: +Needs backport to D7

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

caiovlp’s picture

Status: Active » Needs review
FileSize
2.76 KB

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

xjm’s picture

Cameron Tod’s picture

Status: Needs review » Reviewed & tested by the community

Code looks good style and functionality wise from here.

caiovlp’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.77 KB

Same patch, just switched $set_name and $default_links so that this is easily backportable to 7.x.

cweagans’s picture

Status: Needs review » Reviewed & tested by the community

Oh, good call. I'm going to move this back to RTBC - seems sane, tests pass, and code looks good

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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

ldweeks’s picture

Well, 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!

ldweeks’s picture

Status: Patch (to be ported) » Needs review

Status: Needs review » Needs work

The last submitted patch, fix-shortcut-set-save-1175700-9.patch, failed testing.

webchick’s picture

Awesome, 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.

ldweeks’s picture

Status: Needs work » Needs review
FileSize
3.02 KB

Tests pass on my local dev environment. Here goes...

mgifford’s picture

mgifford’s picture

Issue summary: View changes

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

dcam’s picture

Status: Needs review » Needs work

#15 has some code style issues that need fixing.

  1. +++ b/modules/shortcut/shortcut.test
    @@ -48,9 +48,15 @@ class ShortcutTestCase extends DrupalWebTestCase {
    +     $set = new stdClass();
    +     $set->title = empty($title) ? $this->randomName(10) : $title;
    

    These lines are indented 3 spaces instead of 2.

  2. +++ b/modules/shortcut/shortcut.test
    @@ -75,7 +81,7 @@ class ShortcutTestCase extends DrupalWebTestCase {
    -   * 
    +   *
    
    @@ -316,7 +322,7 @@ class ShortcutSetsTestCase extends ShortcutTestCase {
    -    
    +
    

    These whitespace changes are out of scope.

  3. The patch removes the new line at the end of the file.
mgifford’s picture

Status: Needs work » Needs review
FileSize
2.28 KB

Thanks!

dcam’s picture

Issue tags: +Novice

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

dcam’s picture

Status: Needs review » Needs work
amitgoyal’s picture

Status: Needs work » Needs review
FileSize
1.58 KB

Please review tests-only patch as per #20.

Status: Needs review » Needs work

The last submitted patch, 22: fix-shortcut-set-save-1175700-22-tests.patch, failed testing.

dcam’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.28 KB

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: fix-shortcut-set-save-1175700-24.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: fix-shortcut-set-save-1175700-24.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: fix-shortcut-set-save-1175700-24.patch, failed testing.

Status: Needs work » Needs review
dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 24: fix-shortcut-set-save-1175700-24.patch, failed testing.

David_Rothstein’s picture

Issue tags: -Novice

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

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

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

David_Rothstein’s picture

Title: set_name in shortcut_set_save() doesn't seem to work » shortcut_set_save() "set_name" documentation is confusing, and passing in a nonexistent set name creates orphan menu links
Status: Needs work » Needs review
FileSize
1.05 KB

Well, 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.

Dave Reid’s picture

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

Status: Needs review » Needs work

The last submitted patch, 37: 1175700-shortcut-set-save-fix-set-name-api.patch, failed testing.

Dave Reid’s picture

With the test changes as well.

Dave Reid’s picture

The last submitted patch, 39: 1175700-shortcut-set-save-fix-set-name-api.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 40: 1175700-shortcut-set-save-fix-set-name-api.patch, failed testing.

Dave Reid’s picture

Something is massively broken with the testbot.

lpalgarvio’s picture

I agree with Dave on #37.
A release can include a reference to this issue and possible incompatibilities.

Dave Reid’s picture

Status: Needs work » Needs review

This is fully ready for review, has passing test coverage, and is the same fix as Drupal 8.

  • webchick committed 85a807d on 8.3.x
    Issue #1175700 by caiovlp, swentel, ldweeks: Fixed set_name() in...

  • webchick committed 85a807d on 8.3.x
    Issue #1175700 by caiovlp, swentel, ldweeks: Fixed set_name() in...

  • webchick committed 85a807d on 8.4.x
    Issue #1175700 by caiovlp, swentel, ldweeks: Fixed set_name() in...

  • webchick committed 85a807d on 8.4.x
    Issue #1175700 by caiovlp, swentel, ldweeks: Fixed set_name() in...
scotwith1t’s picture

Any 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

David_Rothstein’s picture

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

 function shortcut_set_add_form($form, &$form_state) {
-  $form['new'] = array(
+  $form['title'] = array(

Why is this being changed? It breaks the form structure for anyone altering the form, and doesn't seem related to this issue.

jollysolutions’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Why did you mark this RTBC, given the reviews above?

  • webchick committed 85a807d on 9.1.x
    Issue #1175700 by caiovlp, swentel, ldweeks: Fixed set_name() in...