Edgewall Software
Modify

Opened 11 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, 11 years ago

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

comment:2 by Christian Boos, 11 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, 11 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'.
to as closed The owner will be changed from Remy Blank to the specified user.

Add Comment


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