Opened 11 years ago
Closed 11 years ago
#22114 closed Bug (fixed)
URLField form adds trailing slash to pathless URLs
Reported by: | Owned by: | nobody | |
---|---|---|---|
Component: | Forms | Version: | 1.6 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
On line 678 in forms/fields.py, the URLfield.to_python() method adds a trailing slash to all pathless URLs (e.g. http://www.example.com becomes http://www.example.com/, but http://www.example.com/blah.html is left alone). The comment for why it does this is "the path portion may need to be added before query params", but I have no idea what that has to do with arbitrarily adding a slash to the end of pathless URLs.
Considering that this makes a visible change to the user's desired input, and they have no way to prevent it, I would like to see this changed. Simply removing the offending line (and the if check it's inside) seems to work just fine.
Attachments (2)
Change History (15)
comment:1 by , 11 years ago
comment:2 by , 11 years ago
I don't know the backward-compatibility policies for Django, but a change like this sounds ideal for a major version bump. So could Django 1.7 potentially contain this fix?
comment:3 by , 11 years ago
A future version, yes - I don't know about 1.7 though.
https://docs.djangoproject.com/en/dev/misc/api-stability/
So, to do away with this, we'd have to:
- provide an alternative function that works correctly
- raise a deprecation warning
- maintain the existing functionality over the full length of the deprecation cycle
but I am not at all sure how this would be achieved here.
comment:4 by , 11 years ago
Yeah, that would suck. You'd basically need to define models.URLField2 and forms.URLField2 (or something like that). Though, from my cursory reading of the API Stability page, this seems like a viable "Backwards incompatible change" for a minor version release. It's such a tiny difference.
I wonder if it would be worthwhile, at least, to mention the bug and workaround for it in the docs for URLField. As far as I know, there are currently two ways to work around this:
1) Define a custom field class that derives from CharField, doing the same thing as URLField but using a custom class for it's "form_class" setting in formfield(). That custom class is identical to forms.URLField, except for those offending lines.
2) Define that same forms.URLField-like class, and use it in the "formfield_overrides" setting of your ModelAdmin-derived class in your admin.py file, as seen in my answer here: http://stackoverflow.com/a/21944976/464318
comment:5 by , 11 years ago
Yes - please do mention the behaviour in the docs, and the longer-term question of what to do about the method's behaviour can be dealt with subsequently.
comment:6 by , 11 years ago
I think this should not be done...
http://www.example.com if is appended with trailing '/' will have no effect
But http://www.example.com/test will have . What if 'test' is a document and adding a trailing '/' to it will render it as dir
Current behavior is correct.
follow-up: 9 comment:7 by , 11 years ago
No, current behavior is not correct. I don't see the point in unconditionally transforming http://www.example.com
to http://www.example.com/
.
What sort of (serious) compatibility issues could be triggered by changing this?
by , 11 years ago
Attachment: | 22114-1.diff added |
---|
comment:8 by , 11 years ago
Has patch: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
UI/UX: | unset |
comment:9 by , 11 years ago
Replying to claudep:
No, current behavior is not correct. I don't see the point in unconditionally transforming
http://www.example.com
tohttp://www.example.com/
.
What sort of (serious) compatibility issues could be triggered by changing this?
I meant that '/' is not added in the second case that is correct... I misread the summary and thought it wanted trailing '/'
Also I think the diff is good.
comment:10 by , 11 years ago
Yay, a patch! Thank you for taking on this bug, claudep.
I'm curious about the change you made, though: why is it important to add a trailing slash onto a pathless URL that has query args?
by , 11 years ago
Attachment: | 22114-2.diff added |
---|
comment:11 by , 11 years ago
You're right, I probably read #11826 too quickly. I just uploaded a new patch.
comment:12 by , 11 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
For the release note (besides moving it to 1.8): "URLField.to_python no longer adds a trailing slash to pathless URLs." Otherwise, LGTM.
comment:13 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
I think you're right about how it should behave - but since it has behaved in this way for some years, a lot of code may have been written since then that relies on this behaviour, so there could be backward-compatibility issues.