Modify

Opened 4 years ago

Closed 3 years ago

#7399 closed defect (fixed)

Expand one more "/" while parsing "href"

Reported by: anonymous Owned by: rjollos
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 4 years ago by anonymous

To fix it change tracmenus/web_ui.py

menu[name]['label']=menu[name].setdefault('label', html.a())(href=value.startswith('/') and req.href()+value or value)

--->

menu[name]['label']=menu[name].setdefault('label', html.a())(href=value.startswith('/') and value or req.href()+value)

comment:3 Changed 3 years ago by rjollos

  • Owner changed from cbalan to rjollos
  • Status changed from new to assigned

#7680, #8877 and #8953 closed as duplicates.

comment:3 Changed 3 years ago by rjollos

  • Resolution set to fixed
  • Status changed from assigned to closed

(In [11038]) Fixes #7399, Refs #7680, #8877, #8953: (0.1.1) Links were invalid when Trac was accessed from the top-most path.

comment:4 Changed 3 years ago by rjollos

Well, I think #7680 had a better solution.

comment:5 Changed 3 years ago by rjollos

(In [11039]) Fixes #7399, Refs #7680, #8877, #8953: (0.1.1) [11038] was the wrong solution. This fix should work better.

comment:6 Changed 3 years ago by dom@…

  • Resolution fixed deleted
  • Status changed from closed to 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: Changed 3 years ago by osimons

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 in reply to: ↑ 7 ; follow-ups: Changed 3 years ago by osimons

Replying to osimons:

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.

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 in reply to: ↑ 8 Changed 3 years ago by rjollos

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 to href=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 in reply to: ↑ 8 Changed 3 years ago by rjollos

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 to href=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 3 years ago by rjollos

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [11154]) Fixes #7399, Refs #7680, #8877, #8953: Allow Href to take care of path joining. Tested in 0.11 and 0.13dev-r10909.

comment:12 Changed 3 years ago by rjollos

(In [11155]) Fixes #7399, Refs #7680, #8877, #8953: Removed egg and pydev files accidentally committed in [11154].

comment:13 Changed 3 years ago by rjollos

(In [11156]) Fixes #7399, Refs #7680, #8877, #8953: Removed egg and pydev files accidentally committed in [11154].

comment:14 Changed 3 years ago by rjollos

Sorry about [11155] and [11156]. I was trying out Subversive in Eclipse and I feel like it lied to me with the commit list.

comment:15 Changed 3 years ago by rjollos

  • Resolution fixed deleted
  • Status changed from closed to 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: Changed 3 years ago by 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 :-)

comment:17 in reply to: ↑ 16 Changed 3 years ago by rjollos

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 3 years ago by rjollos

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 3 years ago by rjollos

  • Resolution set to fixed
  • Status changed from reopened to 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'.

Add Comment

Modify Ticket

Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.