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:
- Try to make menu_link_save() actually handle query string parameters correctly.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#30 | allow-shortcut-query-parameters-614498-29.patch | 4.51 KB | odegard |
#1 | shortcuts-query.patch | 1.9 KB | Gábor Hojtsy |
#6 | shortcuts-query-params2.patch | 3.55 KB | carlos8f |
#4 | shortcuts-query-params.patch | 3.71 KB | carlos8f |
#13 | shortcuts-query-params.3.patch | 3.92 KB | carlos8f |
Comments
Comment #1
Gábor HojtsyThis 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.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedThat'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.
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.
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.
Comment #3
carlos8f CreditAttribution: carlos8f commentedSo 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:
I'm at work on a patch, should be pretty straight-forward :)
Comment #4
carlos8f CreditAttribution: carlos8f commentedHere's my implementation, looks good to me. Update: #651712: Rendered menu tree links do not include proper attributes is committed, so this patch works.
Comment #5
carlos8f CreditAttribution: carlos8f commentedNeed to fix an E_NOTICE
Comment #6
carlos8f CreditAttribution: carlos8f commentedFixed it.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedCould 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.
Comment #8
carlos8f CreditAttribution: carlos8f commented@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.
Comment #9
carlos8f CreditAttribution: carlos8f commentedUpdate: #651712: Rendered menu tree links do not include proper attributes is committed so the patch above should work fine.
Comment #10
sun.core CreditAttribution: sun.core commentedNot sure whether this is really critical. A bug, yes, but the world won't implode if we release without this fix. ;)
Comment #11
bleen CreditAttribution: bleen commentedsubscribe
Comment #12
carlos8f CreditAttribution: carlos8f commentedQuick reroll. This patch could use a test but I'll hold off until there is a review.
Comment #13
carlos8f CreditAttribution: carlos8f commentedduh, the patch :)
Comment #14
abonadad1985 CreditAttribution: abonadad1985 commented#1: shortcuts-query.patch queued for re-testing.
Comment #15
carlos8f CreditAttribution: carlos8f commentedActually, 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.
Comment #16
paskainos CreditAttribution: paskainos commentedsubscribing
Comment #17
iLLin CreditAttribution: iLLin commentedWhat happen with this? Is there a workaround?
Comment #18
ttkaminski CreditAttribution: ttkaminski commentedThe 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.
Comment #19
boclodoa CreditAttribution: boclodoa commentedThe patch #13 applied cleanly to Drupal 7.19 and it works with clean urls enabled. Haven't tested without clean urls.
Comment #20
klaasvw CreditAttribution: klaasvw commentedHere'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.
Comment #22
klaasvw CreditAttribution: klaasvw commentedNow hopefully without failing tests.
Comment #23
klaasvw CreditAttribution: klaasvw commentedComment #24
tstoecklerJust 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
Comment #25
mgiffordBeen a few years since @carlos8f touched this issue so unassigning.
Comment #26
zestagio CreditAttribution: zestagio commented#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.
Comment #27
joel_osc CreditAttribution: joel_osc at OpenPlus commentedPatch works for me! Thx.
Comment #28
odegard CreditAttribution: odegard at Norweb AS commentedChecked this patch on D7.78 and D7.81 with PHP7.2.
I've changed two things.
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.
The other issue I had was this line:
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:
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:
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.
Comment #30
odegard CreditAttribution: odegard at Norweb AS commentedApologies. Left a stray dms() in there. New patch with that line removed.