req.href can generate empty URLs

[Remy Blank]

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().

2. 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.

[ThurnerRupert]

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

[Christian Boos]

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.

[Remy Blank]

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?

[Remy Blank]

Proposed fix

[Remy Blank]

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?

[Christian Boos]

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

[Remy Blank]

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.

[Remy Blank]

Simpler patch applied in [8550].

[Remy Blank]

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.

[Remy Blank]

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