Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

#8159 closed defect (fixed)

req.href can generate empty URLs

Reported by: Remy Blank Owned by: Remy Blank
Priority: normal Milestone: 0.12
Component: web frontend Version: 0.11-stable
Severity: normal Keywords: Href
Cc: Branch:
Release Notes:
API Changes:

Description

For Trac instances located at the root of a website, req.href has an empty base. This means that the following uses will generate empty URLs:

req.href()
req.href('')
req.href('/')

If such a link is used as an href attribute in an <a> tag, it will not point to the root of the site, but to the page where the tag is located. See #8059 for an example of this symptom.

An attempt was made in [7930] to fix this corner case, by having the calls above generate '/' instead of the empty string. It was reverted in [7988] due to its interference with another use of req.href:

req.href() + path

The intention here is that path starts with a /, and is appended to the (relative) base URL. With the proposed fix, the URL gets broken by an unintended additional /.

I see two solutions:

  1. Change req.href() to return '/', and fix the second use with:
    req.href().rstrip('/') + path
    
    The advantage is that the semantics of Href are simpler: it always generates a valid relative URL. But this is an API change which must be handled with care. A quick survey on trac-hacks shows that some plugins rely on the current behavior of req.href().
  1. Leave Href as-is, and fix occurrences of the first use with:
    req.href() or '/'
    
    The drawback is that the problematic cases are more difficult to detect. For example, the following breaks when path is empty (taken from #8059, fixed in [7988]):
    req.href(path)
    

So, is it worth fixing Href, or is the current behavior more desirable? All input appreciated.

Attachments (1)

8159-href-empty-base-r8501.patch (4.4 KB ) - added by Remy Blank 10 years ago.
Proposed fix

Download all attachments as: .zip

Change History (10)

comment:1 by ThurnerRupert, 10 years ago

as it caused a couple of bugs i d prefer fixing it, for 0.12.

comment:2 by Christian Boos, 10 years ago

Component: generalweb frontend
Keywords: Href added
Milestone: 0.12

Well, I think that the following:

req.href() + path

should be used only when path is actually not a path, but e.g. an anchor.

When path is a path, one should write:

req.href(path)

which should do the right thing whether path starts with a '/' or not.

>>> href = Href('')
>>> href('trac')
'/trac'
>>> href('/trac')
'/trac'
>>>

So my point is that in practice one shouldn't write code like in 1. (with the rstrip). So I'm in favor of the API change.

in reply to:  2 comment:3 by Remy Blank, 10 years ago

Replying to cboos:

When path is a path, one should write:

req.href(path)

which should do the right thing whether path starts with a '/' or not.

Except when path ends with a '/':

>>> href = Href('/base')
>>> href('/trac/')
'/base/trac'

Slashes are stripped from both ends of the arguments. See #7617 for examples of this behavior.

Other than that, I agree with your argument. Maybe we should avoid stripping trailing slashes from the last argument?

by Remy Blank, 10 years ago

Proposed fix

comment:4 by Remy Blank, 10 years ago

The patch above fixes Href so that:

>>> Href('')()
'/'
>>> Href('')('/')
'/'

For the specific use case req.href() + path, it adds an __add__() operator to Href, so one can now write req.href + path instead. This will join href.base and the argument, avoiding a double / at the join.

Comments?

comment:5 by Christian Boos, 10 years ago

Seems OK. Could we simplify things further and strip the base's trailing slash in the constructor?

in reply to:  5 comment:6 by Remy Blank, 10 years ago

Replying to cboos:

Could we simplify things further and strip the base's trailing slash in the constructor?

Yes, good idea.

While implementing this patch, I had a dilemma: should I use .lstrip('/') or .rstrip('/') to remove all leading or trailing slashes, or just remove a single leading or trailing slash? I guess I'll switch to the former.

comment:7 by Remy Blank, 10 years ago

Simpler patch applied in [8550].

comment:8 by Remy Blank, 10 years ago

The new __add__() operator has been backported to 0.11-stable in [8551] to facilitate compatibility of plugins with both 0.12 and 0.11.

comment:9 by Remy Blank, 10 years ago

Resolution: fixed
Status: newclosed

And updated the API change documentation in TracDev/ApiChanges/0.12@6.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Remy Blank.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.