shortcut_set_save() no longer deletes existing links when a new set of links is passed in

uhm.. webchick... look closer to your step 3.... there is where the first bug happens

Step 3: Click "Add to shortcuts" on the confirm form.

Shoudn't that button read delete shortcuts? (and why isn't there a test for deleting shortcuts?)

secondly, note the notice your first screenshot!

lastly, note the last shortcut in your second screenshot!

The original bug report is correct. (What happened is that she used the inline link to add a shortcut from that page...) I can reproduce something this, and #633920: Add shortcut - doesn't display seems related also.

I haven't tracked it down exactly, but it seems like this is due to the shortcut menu rendering getting confused about menu link parents, and sometimes failing to render a shortcut if it can't find its parent. Particular problems seem to occur when you add a local task as a shortcut...

The above is the main bug, and the most serious one. Other side issues which come out of this:

#607348: html tags are escaped when adding shortcuts
#635814: Some pages should not display the "Add to shortcuts" link

Not sure whether this qualifies as critical.

I think it does. What's not totally clear from the above comments is that this happens on a significant number of pages. On those pages, the "add shortcuts" button is totally broken. It might not reach the "completely breaks Drupal" level of criticality, but it would look pretty ridiculous if the module is released with this behavior.

The issue seems to occur because the mlids in menu_links get's changed when you add another shortcut, which of course makes the shortcut to the deletion page of the shortcut invalid, since that menu link now has another mlid.

It's easy to reproduce:

* Create a shortcut, and note the mlid (For instance by looking at the path to the deletion page)
* Create another shortcut, and check the mlid again. It's now different.

Does somebody now why the system is designed in such a manner? I can't think of any reason to change the mlid, which is the primary key, in the menu system myself.

EDIT: I guess it is because it's a convenient way to make sure that all values are updated without having to mess around to much...

The replacement occurs here, in the shortcut_set_save function, shortcut.module:

// If links were provided for the set, save them, replacing any that were
  // there before.
  if (isset($shortcut_set->links)) {
    menu_delete_links($shortcut_set->set_name); <--- There it is! :D
    foreach ($shortcut_set->links as &$link) {
      // Do not specifically associate these links with the shortcut module,
      // since other modules may make them editable via the menu system.
      // However, we do need to specify the correct menu name.
      $link['menu_name'] = $shortcut_set->set_name;
      menu_link_save($link);
    }

It seems that the menu_link_save function *can* handle updating existing menu links, so there seems to be little point in deleting them and adding them again.
I tried removing the menu_delete_links line and everything seems to be working nicely.

Here is a patch doing the above.

Changed comment above the foreach loop to make more sense.

Nice find indeed! (And I'm also now realizing that my comment in #3 may have actually mixed another bug with this one - but looks like that one is already fixed in #680380: Shortcut bar cannot deal with deep menu items.)

However, I'm not so sure about the fix...

The reason menu_delete_links() was in there is that shortcut_set_save() is intended to be an API function. When you pass it an array of links, you are supposed to be saying that these are the exact list of links that you want to associate with the set. With this patch, doesn't it instead mean that there are potentially old links hanging around?

Is there some other way we can think of to fix this?

Actually, perhaps it's as simple as just changing the internal logic of shortcut_set_save() to go back to deleting links, but instead of deleting all links (as before), only delete links whose 'mlid' is not contained in the provided list?.... Not sure :)

How about something like this attached patch then?

It just saves off all mlids for every link in the set into an array, then it fetches all mlids in the set that isn't in the array from the database, which shouldn't be there, and deletes them.

You could do this with a call menu_load_links instead of a query, which would look a little cleaner but take up a little more performance. What do you guys think?

Damn... forgot to actually save the mlids in the array =) Here's a correct version.

Looks pretty good, although instead of a direct database query, I think it might be a little cleaner to use the API here - so for example, loading all existing links using menu_load_links(), then deleting the ones from that list that were not also provided in $shortcut_set->links?

There are also a few coding style issues here - see http://drupal.org/coding-standards for more details.

It also occurs to me, the shortcut module currently has no tests (gulp) and this might be a good place to add one, since it's a tricky part of the API. We could add a test to make sure that when shortcut_set_save() is called, it correctly updates existing links and deletes unused ones within the set.

Fixed comments according to coding standards, and changed to using the api instead of a query.

I agree with David that we need a test for this, since it's quite hard to know if it actually works otherwise. I post this patch without a test right now however, just to get a patch using the api in here.

Here's an update of the previous patch again, this time with a space before the curly braces. Damn, I never get all of those things right it seems :P

There's a separate issue for the unit test here:

http://drupal.org/node/680850

I think we should wait and get this in when we have a proper test for it. Some work has started on tests for shortcut module, and we can retest this patch against head when it is in. What do you guys think? Postpone? =)

shortcut tests are getting close (#680850: Tests for Shortcut module) ... can someone familiar with this issue take a look?

Subscribinate

+    	if(array_search($link['mlid'],$mlids) === FALSE) {

Please add a space after the if, and between the function arguments.

Would be good to have the tests in but would probably be enough to post a composite patch here, just to prove the patch passes with the new tests rather than postpone it.

Since the critical bug here is already fixed I'm downgrading this, since we have a very busy criticals queue at the moment.

Cleanup!

Avoid a run-around. Tests for this issue didn't make it into #680850: Tests for Shortcut module. The new issue covering a test for this patch was going to be #733924: Investigate where Shortcut module lacks test coverage. However, that issue pointed back to this issue stating that the tests should be written for this issue before it can be committed. So I'm back here.

I was looking to review this issue and its test, but the test doesn't exist and I am not skilled enough to write one, yet.

I can't reproduce this bug because step 3 is no longer possible; "Add to shortcuts" isn't on the confirm form now.

Unable to reproduce. The UI has changed and removed the 3rd step to reproduce.

Either update steps to reproduce or close.

My bad for not retitling this properly for the followup - we are now just trying to fix a (less serious) regression caused by the original patch. The patch in #23 is still worth reviewing (and I had meant to review it much earlier - bad me!)

The patch looks good, but the indentation on the foreach ($shortcut_set->links as &$link) part is wrong, and the code comments should wrap at 80 characters - these look like they're longer. Also, I'd suggest avoiding the use of "mlids" in the code comment; maybe it's better to use something more descriptive (e.g., "menu link IDs")?

For the tests, @bleen18 did start writing some at #680850: Tests for Shortcut module. I swiped this from the patch at #44 in that issue:

+  function testShortcutSetSave() {
+    $set = $this->set;
+    $old_mlids = $this->getShortcutInformation($set, 'mlid');
+
+    /* 
+    @todo: Currently shortcut_set_save() does not handle deleted links
+           so the code below fails.
+    $link = array_pop($set->links);
+    $deleted_mlid = $link['mlid'];
+    shortcut_set_save($set);
+    $saved_set = shortcut_set_load($set->set_name);
+    */

Basically the idea would be to do something like that, and then assert that the $deleted_mlid does in fact get deleted when shortcut_set_save() is called.

I fixed the issues noted above and added a test for this in the testShortcutSetSave() function. ran the test both with and without the added code in shortcut.module, and it seems to work nicely.

Couple grammatical errors in this patch... otherwise, the test looks good

+++ modules/shortcut/shortcut.test	14 May 2010 17:22:33 -0000
@@ -251,6 +251,16 @@ class ShortcutSetsTestCase extends Short
+    // Make sure that shortcut_set_save() removes links that does not exist in

"that do not exist"

+++ modules/shortcut/shortcut.test	14 May 2010 17:22:33 -0000
@@ -251,6 +251,16 @@ class ShortcutSetsTestCase extends Short
+    $this->assertFalse(array_search($deleted_mlid, $new_mlids), 'shortcut_set_save() removed items that was not provided in the set from the database.');

that were not

+++ modules/shortcut/shortcut.test	14 May 2010 17:22:33 -0000
@@ -251,6 +251,16 @@ class ShortcutSetsTestCase extends Short
+    // Make sure that shortcut_set_save() removes links that does not exist in
...
+    $this->assertFalse(array_search($deleted_mlid, $new_mlids), 'shortcut_set_save() removed items that was not provided in the set from the database.');

"that do not exist"

Powered by Dreditor.

Fixed grammatical errors above.

+++ modules/shortcut/shortcut.module	14 May 2010 17:22:33 -0000
@@ -354,13 +354,24 @@ function shortcut_set_save(&$shortcut_se
+      if (array_search($link['mlid'], $mlids) === FALSE) {

Is there any reason for not using if (in_array($link['mlid'], $mlids)) {?
Not that it matters, but that would seem a bit more natural to me.

New patch.

Oops. Typo. This should work.

Fixing text format

This issue doesn't exist in Drupal 8.2.x anymore so moving back to D7.