#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 claudep 15 months ago.
22114-2.diff (7.1 KB) - added by claudep 15 months ago.

Download all attachments as: .zip

Change History (15)

comment:1 Changed 15 months ago by EvilDMP

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 15 months ago by coredumperror

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 Changed 15 months ago by EvilDMP

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 Changed 15 months ago by coredumperror

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 Changed 15 months ago by EvilDMP

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 Changed 15 months ago by anubhav9042

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

Version 0, edited 15 months ago by anubhav9042 (next)

comment:7 follow-up: Changed 15 months ago by 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?

Changed 15 months ago by claudep

comment:8 Changed 15 months ago by claudep

  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted
  • UI/UX unset

comment:9 in reply to: ↑ 7 Changed 15 months ago by anubhav9042

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 15 months ago by anubhav9042 (previous) (diff)

comment:10 Changed 15 months ago by coredumperror

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?

Changed 15 months ago by claudep

comment:11 Changed 15 months ago by claudep

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

comment:12 Changed 14 months ago by timo

  • Triage Stage changed from Accepted to 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 Changed 14 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

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