When viewing a page that has query parameters in the URL and adding a shortcut link, things kind of break. The shortcut you get doesn't work right, and sometimes things get messed up even worse (I hear this is particularly bad when combined with the overlay).

A large part of the problem is that menu_link_save() -- where the shortcut link eventually gets passed to -- is not designed to take query string parameters, only full paths. When you try to pass a string containing query parameters to this function, it looks like the router path doesn't get calculated correctly, and other bad things happen...

I suspect this issue is responsible for a number of strange bugs in the shortcut module.

Seems like our options are:

  1. Try to make menu_link_save() actually handle query string parameters correctly.
  2. Remove the "feature" that allows you to bookmark links that have query string parameters in them. In other words, do not pass along any query string parameters in the "Add to shortcuts" link generated in shortcut_page_build(), and invalidate any links that have them in shortcut_valid_link().

While supporting query string parameters would be nice, it seems like the easier fix is probably option #2.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Status: Active » Needs work
FileSize
1.9 KB

This bug was extensively documented with the overlay issue in relation. The problem is that the query string parts get URLencoded as it they are part of the path itself, stored and then requested that way. So what we get is a 404 given no matching path.

In case of the overlay, the bookmarked link would have a "render=overlay" query string, but that is not to be bookmarked even if query string arguments are supported, because then people who have toolbar access but no overlay access will get badly themed pages which would behave strange (with the overlay child JS actions in place).

However, query strings are meaningful when the query arguments are used to filter or sort lists.

Because David asked to break this out of #610204: Overlay API changes at http://drupal.org/node/610204#comment-2191252 I'm including the relevant patch from there and reproducing his comment on the patch, so we can improve from here.

If we're going to disallow query parameters from shortcut links, we should check for that inside shortcut_valid_link() rather than in a menu callback, and we shouldn't bother adding the query parameters in shortcut_page_build() in the first place if they are only going to get stripped out later on.

David_Rothstein’s picture

The problem is that the query string parts get URLencoded as it they are part of the path itself, stored and then requested that way. So what we get is a 404 given no matching path.

That's the part I forgot to mention above and is (presumably) easier to fix... However, the menu_link_save() problem is trickier. Bad things can happen when the router path doesn't get set correctly.

In case of the overlay, the bookmarked link would have a "render=overlay" query string, but that is not to be bookmarked even if query string arguments are supported, because then people who have toolbar access but no overlay access will get badly themed pages which would behave strange (with the overlay child JS actions in place).

Hm. In the case of a user who does not have overlay access, why does the overlay child JS get added to the page at all? I think I'm missing something here.

However, query strings are meaningful when the query arguments are used to filter or sort lists.

Agreed. It many cases it's not a big deal to skip the query arguments, but for something like Views or Solr it does seem like it would be a shame if you could only add a shortcut that pointed to the initial result set.

carlos8f’s picture

Assigned: Unassigned » carlos8f

So after working with overlays and shortcuts for a few days, I came across the bugs discussed here. It took me a while, but I eventually decided on a course of action:

  1. menu_link_save() actually does support a query parameter, as part of the 'options' array. The only problem is, it's currently non-functional due to the bug #651712: Rendered menu tree links do not include proper attributes. Once that's in, shortcuts can use query strings.
  2. Separating the query string from the path before calling menu_link_save() would solve the 404's caused by query strings being borked when saved as part of the path. shortcut_valid_link() would stay the same, as long as the query string isn't passed to it.
  3. On generating the 'add shortcut' link, certain non-relevant query parameters need to be filtered out. 'render', and 'page' are all that I can think of at the moment. These can simply be added to the drupal_get_query_parameters() exclude argument in shortcut_page_build(). @Gábor, this should solve your concerns.
  4. The shortcut edit interface should include the query string in the path, so it can be edited. Since path and query would be separated in storage, they would need to be joined temporarily for editing, then separated before saving again.

I'm at work on a patch, should be pretty straight-forward :)

carlos8f’s picture

Title: Add handling for query parameters in shortcut links » Figure out what to do about shortcut links with query parameters
FileSize
3.71 KB

Here's my implementation, looks good to me. Update: #651712: Rendered menu tree links do not include proper attributes is committed, so this patch works.

carlos8f’s picture

Need to fix an E_NOTICE

carlos8f’s picture

Status: Needs work » Needs review
FileSize
3.55 KB

Fixed it.

moshe weitzman’s picture

Could I submit a plea to fix the menu links system so that we can store querystring? This would be very helpful in lots of places. For example, consider the link on drupal.org which shows the number of critical issues in D7. I can easily imagine us wanting that in the menu system and url aliases to /d7/criticals. Right now, we can't alias the horrid URL or add to menu.

carlos8f’s picture

Title: Figure out what to do about shortcut links with query parameters » Add handling for query parameters in shortcut links

@moshe, I know this has been a problem for a while... I've always wanted the ability to make URL aliases to filtered pages :) I could imagine querystring support for the menu/path system being a large undertaking... a D8 initiative for sure, even though we need it in D6 for drupal.org. If you get a chance, please review #651712: Rendered menu tree links do not include proper attributes, a one-line bug that is disabling a bunch of menu link functionality, including query strings.

Also, changing title since it seems like we figured it out.

carlos8f’s picture

Update: #651712: Rendered menu tree links do not include proper attributes is committed so the patch above should work fine.

sun.core’s picture

Title: Figure out what to do about shortcut links with query parameters » Add handling for query parameters in shortcut links
Priority: Critical » Normal

Not sure whether this is really critical. A bug, yes, but the world won't implode if we release without this fix. ;)

bleen’s picture

subscribe

carlos8f’s picture

Quick reroll. This patch could use a test but I'll hold off until there is a review.

carlos8f’s picture

duh, the patch :)

abonadad1985’s picture

#1: shortcuts-query.patch queued for re-testing.

carlos8f’s picture

Status: Needs review » Needs work

Actually, I believe this patch would only work if clean_url is on, since it assumes '?' starts the query parameters whereas when clean_url = 0, '?q=' would be included there along with '&' + params. Also, it needs some functional tests.

paskainos’s picture

subscribing

iLLin’s picture

What happen with this? Is there a workaround?

ttkaminski’s picture

The patch applied cleanly to Drupal 7.15 and it works with clean urls enabled. Haven't tested without clean urls, but I agree that I don't think it will work in that case. Fortunately, most sites use clean urls.

boclodoa’s picture

The patch #13 applied cleanly to Drupal 7.19 and it works with clean urls enabled. Haven't tested without clean urls.

klaasvw’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.71 KB

Here's a patch against 7.x

I made some changes to the original patch from #13:

- The query string is removed in the `shortcut_valid_link` instead of the validation handler. This will also cover validation in other cases.
- Parsing the query string into a menu link option is done in the submit handler instead of the validation handler.
- Added support for show the query parameters in the shortcut overview.

Status: Needs review » Needs work

The last submitted patch, 20: allow-shortcut-query-parameters-614498-20.patch, failed testing.

klaasvw’s picture

Now hopefully without failing tests.

klaasvw’s picture

Status: Needs work » Needs review
tstoeckler’s picture

Just verified that this is still a problem in 8.0.x. Not reclassifying because in 8.0.x it's not actually a bug, because the query gets stripped properly. So if we go with 2. from the issue summary then this should stay 7.x

mgifford’s picture

Assigned: carlos8f » Unassigned

Been a few years since @carlos8f touched this issue so unassigning.

zestagio’s picture

#22 ok for me, but there is a bug with #22. Quick link for remove shortcut not working. For example, open a page admin/config?destination=node, click to link "Add to default shortcuts". The url of this page will be added to shortcuts, but after re-open of this url (admin/config?destination=node) link not changed to "Delete from default shortcuts". You can add one more shortcut for this url. It's a bug. I attached a patch with fix.

joel_osc’s picture

Patch works for me! Thx.

odegard’s picture

Checked this patch on D7.78 and D7.81 with PHP7.2.

I've changed two things.

if ($link == ltrim(url($shortcut['link_path'], $shortcut['options']), '/')) {

The $link variable is built using $_GET['q'], while the url-function includes possible subdirs and language prefixes. I guess this worked on monolingual site at the domain root but in my case it did not. I've changed this line building a $link_shortcut variable simliarly to how $link is built so they are compatible. I added a !empty on the query key too.

if (!empty($shortcut['options']) && !empty($shortcut['options']['query'])) {
    // Check all url with get parameters on same format as $link.
    $link_shortcut = $shortcut['link_path'] . '?' . drupal_http_build_query($shortcut['options']['query']);
	if ($link == $link_shortcut) {

The other issue I had was this line:

$path = drupal_substr($path, strpos($path, $position));

drupal_substr take a path, a $start and a $length. I'm sure there should've been a 0 as second argument there. Also, since the position is already found just a few lines further up, I reuse it ending up with:

$path = drupal_substr($path, 0, $position);

So far soo good. It annoys me however that the shortcuts are given name according to drupal_get_title. This is good UX when used for it's original intent. This is not good UX when we're doing this to allow multiple shortcuts on the same path, just with different query parameters.

In my case, I've decided to remove the add-link-inline mechanism alltogether. I like the star-thingy, so I'm just changing the link from add-link-inline to add-link. Then, in a custom module I have the following hook_form_alter:

  if ($form_id != 'shortcut_link_add') {
    return;
  }

  if (isset($_REQUEST['token']) && drupal_valid_token($_REQUEST['token'], 'shortcut-add-link') && shortcut_valid_link($_GET['link'])) {
    $form['shortcut_link']['link_path']['#default_value'] = $_GET['link'];
  }
 

This autofills the shortcut path field. You just give it a name, save it, and you're back to wherever you were.

Of course, this takes over the original usecase where the auto titling is a good idea. I don't think this patch will ever be included until this is solved, I'm just leaving my code here in case someone finds it useful.

Status: Needs review » Needs work

The last submitted patch, 28: allow-shortcut-query-parameters-614498-28.patch, failed testing. View results

odegard’s picture

Apologies. Left a stray dms() in there. New patch with that line removed.