Opened 10 years ago

Closed 10 years ago

#22114 closed Bug (fixed)

URLField form adds trailing slash to pathless URLs

Reported by: coredumperror@… 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)

22114-1.diff (6.8 KB ) - added by Claude Paroz 10 years ago.
22114-2.diff (7.1 KB ) - added by Claude Paroz 10 years ago.

Download all attachments as: .zip

Change History (15)

comment:1 by Daniele Procida, 10 years ago

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.

comment:2 by Robert Rollins, 10 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 Daniele Procida, 10 years ago

A future version, yes - I don't know about 1.7 though.

https://docs.djangoproject.com/en/dev/misc/api-stability/

https://docs.djangoproject.com/en/dev/internals/release-process/#internal-release-deprecation-policy

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 Robert Rollins, 10 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 Daniele Procida, 10 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 ANUBHAV JOSHI, 10 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.

Last edited 10 years ago by ANUBHAV JOSHI (previous) (diff)

comment:7 by Claude Paroz, 10 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 Claude Paroz, 10 years ago

Attachment: 22114-1.diff added

comment:8 by Claude Paroz, 10 years ago

Has patch: set
Triage Stage: UnreviewedAccepted
UI/UX: unset

in reply to:  7 comment:9 by ANUBHAV JOSHI, 10 years ago

Replying to claudep:

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?

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.

Last edited 10 years ago by ANUBHAV JOSHI (previous) (diff)

comment:10 by Robert Rollins, 10 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 Claude Paroz, 10 years ago

Attachment: 22114-2.diff added

comment:11 by Claude Paroz, 10 years ago

You're right, I probably read #11826 too quickly. I just uploaded a new patch.

comment:12 by Tim Graham, 10 years ago

Triage Stage: AcceptedReady 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 Claude Paroz <claude@…>, 10 years ago

Resolution: fixed
Status: newclosed

In d320863878f097b5cbf33ace12e3adb5e02f27e0:

Fixed #22114 -- Stopped adding trailing slashes in URLField.to_python

Thanks coredumperror at gmail.com for the report and Tim Graham
for the review.

Note: See TracTickets for help on using tickets.
Back to Top