Saving a shortcut from the admin screen doesn't work correctly if a path alias is used

Confirm I can reproduce this bug.

The seems to fix the issue...

To test I:
- started with a fresh drupal install
- created node/1 with an alias
- added a shortcut with a path "alias"
- rejoiced at seeing the shortcut added properly

@bleen18 Working great. Thank you. Could you please also write the test for this case?

I can ... but probably not for a few days. If someone else wants to write a test go for it, otherwise Ill give it a whirl later this week

Thought: should we also set a drupal message telling people that we normalized the path the way the menu module does?

...with test

Test works well. Do we need more people to test before setting this issue to 'reviewed & tested by community'?

ygerasimov: if your confident that the code is RTBC and you have thoroughly tested, then you can set it as RTBC. If you're not confident, then we need another review.

...Dont forget that either webchick or dries will look at the patch too before committing it

Retitling this to clarify that it only occurs on the admin screen - adding shortcuts inline via the "plus sign" link on a page already works fine even if that page has an alias.

Patch looks good. Really I think this is more of a menu system bug (or at least menu system limitation) as you really should be able to save an alias to the database and have it work? But anyway, that is unlikely to happen for D7.

Three things though:

1. -    parent::setUp('toolbar', 'shortcut');
   +    parent::setUp('toolbar', 'shortcut', 'path');
   

   I don't think this change is necessary? As far as I can see you are only using the path API in these tests (which is contained in path.inc, not path.module). The path module is only needed if you are using the path user interface.

2. +    path_save($path);
   +    drupal_clear_path_cache();
   

   Why do you need to explicitly clear the cache here? As far as I know, path_save() takes care of that internally.

3. Thought: should we also set a drupal message telling people that we normalized the path the way the menu module does?

   I tend to agree we should do something, although the way the menu module does it is IMO very bad for usability. The drupal_set_message() is cryptic and does not give people the information when they need it - the time you need it isn't right after you submit the form, but only when you go back to the form later and see that the default value of the field is different from the one you entered.

   So perhaps we could do something with the description of the form field - i.e. it currently says "The path to the shortcut" when you go back to edit the shortcut, but maybe we could check drupal_get_path_alias() and if that returns an alias we could also add a sentence explaining that the system path is shown on the form but the alias will be used on display.

   If this proves too complicated I would say we could defer it to a followup issue. It's kind of a separate bug anyway, since we already have this issue when adding shortcuts inline via the "plus sign" link.

Actually, there is still a bug here. With the patch, adding a new shortcut works fine, but editing an existing shortcut and using a path alias there is still broken :(

This patch handles both the adding and the editing of shortcuts (good catch) and it adds some text to the description although the text maybe could use a polish ... I'm not sure. Also, this patch fixes up the test.

oops ... use this patch instead.

this patch removes the DSM() I left in there... doh!

bot

Looks pretty good, although I don't understand why the space is appended at the beginning of the description?

However, it just occurred to me - why not do the attached? This makes it so the alias, not the system path, is always displayed as the default value of the form field. It's actually what is already done in at least one other place in Drupal (the frontpage setting on the "Site Information" screen) and makes a lot more sense to me. This way we don't need to explain anything to the user; instead, the form just shows them the URL they expect :)

Does that make sense? (If so, the rest is RTBC as far as I'm concerned.)

re #15: I like that even better. If a user enters "my/alias" as the shortcut path, then why not show him "my/alias" when he comes back to the form to edit. Makes complete sense.

I cant RTBC, but I would if I could

If I think the code you wrote is RTBC, and you think the one line I added to it is also RTBC, then combined, it is probably good enough :)

Removing tag

Committed to HEAD. Thanks!