Opened 14 years ago
Closed 13 years ago
#7399 closed defect (fixed)
Expand one more "/" while parsing "href"
Reported by: | anonymous | Owned by: | Ryan J Ollos |
---|---|---|---|
Priority: | normal | Component: | MenusPlugin |
Severity: | normal | Keywords: | |
Cc: | chuang@… | Trac Release: | 0.12 |
Description
I found when you configure as below in trac.ini
browser_timeline = enabled browser_timeline.label = Recent Changes browser_timeline.href = /timeline?changeset=on browser_timeline.parent = browsergrp browser_timeline.order = 30
it will translate to html segment like "a href="//timeline?changeset=on">Recent Changes</a>", here one more "/" is added so it doesn't work.
Attachments (0)
Change History (19)
comment:1 Changed 14 years ago by
comment:3 Changed 13 years ago by
Owner: | changed from Catalin BALAN to Ryan J Ollos |
---|---|
Status: | new → assigned |
comment:3 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:5 Changed 13 years ago by
comment:6 Changed 13 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Hi guys!
The fix in r11039 is still not working. This should be the right line.
menu[name]['label']=menu[name].setdefault('label', html.a())(href=value.startswith('/') and (req.href().rstrip('/') + value) or value)
comment:7 follow-up: 8 Changed 13 years ago by
Sorry for butting in, but can't that just be written as href=req.href(value)
? The Href
class is supposed to take care of the path joining for its arguments.
comment:8 follow-ups: 9 10 Changed 13 years ago by
Replying to osimons:
Sorry for butting in, but can't that just be written as
href=req.href(value)
? TheHref
class is supposed to take care of the path joining for its arguments.
Actually, in some older versions of Href
that may not be the case. So to be sure I guess it can be slightly modified to href=req.href(value.lstrip('/'))
just to be sure that it works across all versions for 0.11 -> 0.13dev.
comment:9 Changed 13 years ago by
Replying to osimons:
Actually, in some older versions of
Href
that may not be the case. So to be sure I guess it can be slightly modified tohref=req.href(value.lstrip('/'))
just to be sure that it works across all versions for 0.11 -> 0.13dev.
I appreciate the feedback! I'll make that change and do some testing against the two versions this evening and commit the change.
comment:10 Changed 13 years ago by
Replying to osimons:
Actually, in some older versions of
Href
that may not be the case. So to be sure I guess it can be slightly modified tohref=req.href(value.lstrip('/'))
just to be sure that it works across all versions for 0.11 -> 0.13dev.
It looks like the issue was probably fixed by 0.11: href @ 0.11. I did some testing and the change you suggested is working nicely against 0.11 and the latest trunk. I'll commit that fix shortly.
dom: could you please let me know the result of testing on your Trac instance? Thanks!
comment:11 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
comment:12 Changed 13 years ago by
comment:13 Changed 13 years ago by
comment:14 Changed 13 years ago by
comment:15 Changed 13 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
I seem to have overlooked two things. One is that absolute URLs can be used in the href
field. As long as the URLs begin with the scheme, that can be handled without much trouble:
href = urlparse(value).scheme and value or req.href(value) menu[name]['label']=menu[name].setdefault('label', html.a())(href=href)
The second issue was noted comment:2:ticket:7680. If the string contains query or path arguments, they get quoted. For example, newticket_defect.href = newticket?type=defect
results in /tracenv/newticket%3Ftype%3Ddefect
and the error No handler matched request to /newticket?type=defect.
osimons, do you have any suggestions on how best to proceed? Maybe I should cut everything after and including the ?
character and then append it back after passing the result to req.href
?
comment:16 follow-up: 17 Changed 13 years ago by
And the value
can even have a #fragment
in addition to the query string or instead of the query string.
I don't know. I just commented with a simplification of a particular suggestion in comments, but have no inclination to find out what the "right" thing would be for this plugin in this context... Sorry :-)
comment:17 Changed 13 years ago by
Replying to osimons:
And the
value
can even have a#fragment
in addition to the query string or instead of the query string.I don't know. I just commented with a simplification of a particular suggestion in comments, but have no inclination to find out what the "right" thing would be for this plugin in this context... Sorry :-)
No problem. I think I'll go back to building up the href manually. I need to work out more clearly what the possible cases are, but it seems like a brute-force way would be something like:
req.href.rstrip('/') + '/' + value.lstrip('/')
comment:18 Changed 13 years ago by
comment:6 seems like the best solution here ... or at least the best I'm going to be able to do for now.
comment:19 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
(In [11166]) Fixes #7399, Refs #7680, #8877, #8953: Yet another attempt to fix invalid links when req.href = '/'
. If the href
for a menu item specifies a relative path, the trailing forward slashes are stripped from req.href()
so that the two cases are handled: 1) req.href()
is '/'
and 2) req.href()
is '/somepath'
.
To fix it change tracmenus/web_ui.py
--->