Support fragment only links

NaX - October 22, 2006 - 00:49
Project:Drupal
Version:7.x-dev
Component:menu system
Category:feature request
Priority:normal
Assigned:Unassigned
Status:by design
Description

I need to have menu items that ether link to external sites, have a query string (GET variables) or named anchors. But I had problems with this because user defined menu items were being url encoded and this would break the links. So first I tried hacking the drupal_urlencode function using the parse_url function to try and make the drupal_urlencode function more flexible, but because this function is used more generally I had a problem with getting it to work under all conditions.

So I tried hacking the menu system and menu module, and I think I got it write.

before:
href="node/123%3Fvar1%3Done%2526var2%3Dtwo%2523section1"

after:
href="node/123&var1=one&var2=two#section1"

Not a 100% (&) but the links seem to work.

I searched drupal.org and found quite a few people also having problems with urls being incorrectly encoded.

http://drupal.org/node/85331
http://drupal.org/node/72812
http://drupal.org/node/59674
http://drupal.org/node/56755
http://drupal.org/node/76034

I think this is pretty important since users cant define any url they want as a menu item. And it is something I personally often require. I am not sure if this is a bug or feature request, but since the links were broken I thought it would be considered a bug.

AttachmentSize
menu_paths_nax.patch3.52 KB

#1

chx - October 22, 2006 - 06:06
Title:complex menu items = broken urls» Menu should support query and fragment in links
Category:bug report» feature request
Priority:critical» normal

#2

jacauc - October 24, 2006 - 06:45

How do I apply this patch to my currently running setup?
It doesn't seem to work in the same way as when I normally apply patches.

Thanks
jacauc

#3

NaX - October 24, 2006 - 09:52

This patch is for drupal CVS not for 4.7

I tested it using patch -p0 < menu_paths_nax.patch and it worked. How do you apply patches.

#4

NaX - January 28, 2007 - 20:23
Version:x.y.z» 6.x-dev
Priority:normal» critical
Status:patch (code needs review)» patch (code needs work)

#5

moonray - February 5, 2007 - 22:20

Brilliant. I was just about to post an issue about this.

I was going to work on a patch as well, but then I saw that chx is working on a complete menu overhaul, so can't do anything until that's done.

One comment, though: I found the linking system to be inconsistent. Menu links have one syntax, links from hook_link() and hook_alter_link() another, and breadcrumbs yet again are different. It would be great if we could use the same syntax for all three (the syntax for hook_link() seems the best).

#6

Steven - February 6, 2007 - 08:00

Note that the following patch does a lot to simplify links, and introduces a structure that should be reused by other modules: http://drupal.org/node/111347#comment-191811

Waiting for a reroll for HEAD...

#7

bdragon - September 11, 2007 - 20:13

My strings REALLY SUCK. But, this feature is very easy to implement in the new system!

AttachmentSize
menu-fragment-query.patch1.62 KB

#8

bdragon - September 11, 2007 - 20:13

My strings REALLY SUCK. But, this feature is very easy to implement in the new system!

AttachmentSize
menu-fragment-query_0.patch1.62 KB

#9

bdragon - September 11, 2007 - 20:16

Possibly put these in a collapsed fieldset ("Advanced?")?

#10

chx - September 20, 2007 - 09:53
Title:Menu should support query and fragment in links» Menu does support query and fragment in links
Category:feature request» bug report
Priority:critical» normal

I asked grugnog whether fragment or anchor and he basically said why separate fields. Right.

AttachmentSize
menu_fragment_query-90570-10.patch5.36 KB

#11

chx - September 20, 2007 - 09:54

I asked grugnog whether fragment or anchor and he basically said why separate fields. Right.

AttachmentSize
menu_fragment_query-90570-11.patch2.04 KB

#12

Grugnog2 - September 20, 2007 - 20:44
Status:patch (code needs work)» patch (reviewed & tested by the community)

Tested and menus save and edit, validation still works on invalid paths and the query/anchor is added back in correctly on edit/view. Looks good.

#13

Gábor Hojtsy - September 25, 2007 - 11:37
Status:patch (reviewed & tested by the community)» patch (code needs review)

Hm, how does the UI look like (screenshot) for this? I wonder how people provide query strings and anchors, which need to be derived differently from the actual site URLs on different sites. I see it is easy to split and to join strings here, but what is the user experience?

#14

moshe weitzman - September 27, 2007 - 02:33
Status:patch (code needs review)» patch (reviewed & tested by the community)

Looks to me like there is no UI impact at all. We just use any querystring and fragment that is provided in the path field. i think thats reasonable, and even expected.

#15

Gábor Hojtsy - September 27, 2007 - 17:00
Status:patch (reviewed & tested by the community)» fixed

OK, committed.

#16

Anonymous - October 11, 2007 - 17:12
Status:fixed» closed

#17

telcontar - October 12, 2007 - 09:01

Any chance backporting this to 5.x?

#18

telcontar - October 14, 2007 - 09:19
Status:closed» active

I've tested this on HEAD. Menu paths of the form 'path/to/node#fragment' work. But one might like to add a link to a local anchor on the current page, e.g. use '#fragment' as a menu path. This does not work on HEAD.

A practical example where this would be useful is "Skip Navigation" links for accessibility.

-- telcontar

#19

catch - October 19, 2007 - 14:07
Status:active» patch (code needs work)

seems reasonable and arguably a bug with the original patch, so setting back to CNW.

#20

xamount - November 20, 2007 - 19:17

I second the backport to 5.x

#21

chx - December 23, 2007 - 10:40
Title:Menu does support query and fragment in links» Support fragment only links
Version:6.x-dev» 7.x-dev
Category:bug report» feature request
Status:patch (code needs work)» active

#22

sonicthoughts - June 8, 2008 - 06:55

please backport to 5.6/5.7. this has been very frustrating!
tx

#23

kimothy777 - June 8, 2008 - 06:57

I'm glad I'm not the only confused party here.

http://www.mychildcanbehave.com

#24

pwolanin - June 15, 2008 - 15:04
Status:active» by design

I'm dubious about the anchor-only links - making those work is going to be very theme dependent. You could also add such a "skip" link to your template, or use hook_translated_menu_link_alter.

#25

tobias - August 4, 2008 - 07:01

+1 on backporting to a drupal 5.x version - would this still be possible or has it been done?

cheers,

tobias

 
 

Drupal is a registered trademark of Dries Buytaert.