[Django] #22114: URLField form adds trailing slash to pathless URLs
Django
---------------------------------+--------------------
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 1
---------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22114>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
Changes (by EvilDMP):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:1>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:2>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:3>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:4>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:5>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (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
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:6>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 1
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:7>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Changes (by claudep):
* has_patch: 0 => 1
* ui_ux: 1 => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:8>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by anubhav9042):
Replying to [comment:7 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 '/'
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:9>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:10>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------------------
Comment (by claudep):
You're right, I probably read #11826 too quickly. I just uploaded a new
patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:11>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: 1.6
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timo):
* stage: Accepted => Ready for checkin
Comment:
For the release note (besides moving it to 1.8): "URLField.to_python no
longer adds a trailing slash to pathless URLs." Otherwise, LGTM.
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:12>
Django
Reporter: coredumperror@… | Owner: nobody
Type: Bug | Status: closed
Component: Forms | Version: 1.6
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
Has patch: 1 | checkin
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"d320863878f097b5cbf33ace12e3adb5e02f27e0"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/22114#comment:13>