Opened 17 years ago

Closed 14 years ago

Last modified 13 years ago

#6218 closed (fixed)

Require trailing slash always on MEDIA_URL

Reported by: Michael Toomim <toomim@…> Owned by: Chris Heisel
Component: Core (Other) Version: dev
Severity: Keywords: easy-pickings
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

This is a follow-up patch to the resolution of ticket 6198.

Right now, MEDIA_URL requires a trailing slash if it contains a suffix beyond the domain name, and otherwise the slash is optional. This is an artifact of using urljoin() to combine MEDIA_URL with the url suffix. (Urljoin() was implements relative urls in RFC1808, slightly different than gluing paths together.)

The only real problem is that if you forget the trailing slash from MEDIA_URL, it fails in an odd way, by deleting the path after the last trailing slash. For instance, "http://localhost/dir" combined with "image1.png" will become "http://localhost/image1.png".

This patch replaces the use of "urljoin(MEDIA_URL, suffix)" with "MEDIA_URL + suffix" and updates the documentation. Now it would fail as "http://localhost/dirimage1.png" which is easier to debug.

Attachments (5)

dj.slash.patch (1.8 KB ) - added by Michael Toomim <toomim@…> 17 years ago.
Patch for base.py and settings.py template docs
ticket-6218.patch (2.1 KB ) - added by Chris Heisel 14 years ago.
Ensures MEDIA_URL always ends with a / when set to a non-empty string
ticket-6218.2.patch (4.9 KB ) - added by Chris Heisel 14 years ago.
6218.diff (7.0 KB ) - added by Chris Beaven 14 years ago.
t6218b.patch (7.2 KB ) - added by Chris Heisel 14 years ago.
Smiley's patch, but updated to use PendingDeprecationWarning

Download all attachments as: .zip

Change History (31)

by Michael Toomim <toomim@…>, 17 years ago

Attachment: dj.slash.patch added

Patch for base.py and settings.py template docs

comment:1 by Malcolm Tredinnick, 17 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

If the trailing slash is always required, then we should just add it in if it's missing. If there are cases where it can legitimately be missing and adding it in always is wrong, then the current behaviour is correct and this proposed patch would be wrong (I doubt there is a case where adding the trailing slash will be wrong, though).

So, either way, I don't think this patch solves a problem we have. A patch to append the trailing slash, if needed, and then use urljoin() would be better.

comment:2 by Michael Toomim <toomim@…>, 17 years ago

There are three alternative designs:

(1) Always add a trailing slash if missing
(2) Always require the user to add the trailing slash
(3) Sometimes add a trailing slash, relying on the semantics of urljoin()

(1) is what I wrote code for in ticket 6198. But we decided that it wasn't worth the hassle, and should just go with (2) instead. This ticket implements (2).

(3) is difficult to debug, and not very useful. urljoin() has weird semantics. It only inserts a trailing slash if there is no subdirectory in MEDIA_URL, which is not the common case. In other cases, it deletes the last component of the path, which is just weird.

I recommend either going with (1) or (2). Either is fine with me.

comment:3 by Malcolm Tredinnick, 17 years ago

I don't understand what you mean by option (3), but it doesn't really matter. "Do the right thing" is better than "document around the wrong thing" or "do the wrong thing", which is what we're doing now (and would be perpetuated by this patch), particularly since it's fully backwards compatible.

Go with option (1).

Attach the patch here ... all the bouncing between #6198 and this ticket is making my head spin. I can't see what's tricky about what you wanted to do the first time and it looks like the right thing to do.

comment:4 by Chris Beaven, 17 years ago

Keywords: easy-pickings added

comment:5 by Leo Shklovskii, 17 years ago

Owner: changed from nobody to Leo Shklovskii
Status: newassigned

comment:6 by Leo Shklovskii, 16 years ago

Owner: Leo Shklovskii removed
Status: assignednew

comment:7 by Chris Heisel, 14 years ago

Owner: set to Chris Heisel

by Chris Heisel, 14 years ago

Attachment: ticket-6218.patch added

Ensures MEDIA_URL always ends with a / when set to a non-empty string

comment:8 by Chris Heisel, 14 years ago

Patch needs improvement: unset
Status: newassigned

Attaching a patch with tests, ensuring that it gets the trailing / when set

comment:9 by Chris Heisel, 14 years ago

Needs documentation: set
Patch needs improvement: set

facepalm, forgot documentation and to test the settings.configure workflow...

by Chris Heisel, 14 years ago

Attachment: ticket-6218.2.patch added

comment:10 by Chris Heisel, 14 years ago

Patch needs improvement: unset

Updated patch to include docs, and tests for the settings.configure workflow. If someone wants to review my patch I'd be much obliged

comment:11 by Eric Holscher, 14 years ago

Looks good to me. There's a typo in the docstring for "wether", should be "whether". Otherwise, as long as the setattr implementation is acceptable, the patch looks RFC.

comment:12 by Chris Beaven, 14 years ago

Owner: changed from Chris Heisel to Chris Beaven
Status: assignednew

Magically fixing the setting is not the right behavior. This could silently break existing projects that have the MEDIA_URL setting with no slash.

A better fix would be to raise ImproperlyConfigured in the case of MEDIA_URL not ending in a slash.

comment:13 by toomim, 14 years ago

SmileyChris, please explain your complaint: what behavior are you referring to as "Magically fixing the setting" and what is an example of an existing project this could break?

by Chris Beaven, 14 years ago

Attachment: 6218.diff added

comment:14 by Chris Beaven, 14 years ago

Owner: Chris Beaven removed
Triage Stage: AcceptedDesign decision needed

If a project had the following setting:
MEDIA_URL = 'http://media.foo.com'

And a template which looks like:
<link rel="stylesheet" href="{{ MEDIA_URL }}/css/main.css" type="text/css" />

Then this change will silently alter their css reference to http://media.foo.com//css/main.css

In any case, I've put up a patch of how I'd prefer to see this issue resolved.

comment:15 by Eric Holscher, 14 years ago

This approach also will break pre-existing installs, but just in a more loud manner. This might be preferable, but still seems like a backwards incompat change.

comment:16 by Chris Heisel, 14 years ago

Good point SmileyChris, I didn't think about the hardcoded / in templates, but if I understand the original issue, paths to uploaded media are still broken. I see the value in raising the issue up and letting the user address it, but I was operating under Malcom's admonition to "Go with option (1)"

comment:17 by Chris Beaven, 14 years ago

Needs documentation: unset
Patch needs improvement: set

The current agreed (with core committers at the 2010 sprint) behavior is to only raise a PendingDeprecation warning in 1.3 and loudly break in 1.4

Anyone who wants to change this behavior in the patch is welcome to take it. I'm off to the airport to start my 24h+ flight plan home...

comment:18 by Chris Beaven, 14 years ago

Triage Stage: Design decision neededAccepted

comment:19 by toomim, 14 years ago

Thanks SmileyChris

comment:20 by Chris Heisel, 14 years ago

Owner: set to Chris Heisel
Status: newassigned

Thanks SmileyChris! I'll work up a patch.

by Chris Heisel, 14 years ago

Attachment: t6218b.patch added

Smiley's patch, but updated to use PendingDeprecationWarning

comment:21 by Chris Heisel, 14 years ago

Patch needs improvement: unset

comment:22 by Chris Beaven, 14 years ago

Owner: changed from Chris Heisel to Jannis Leidel
Status: assignednew

Throwing this jezdez's way for a bit of attention, since he's been knee-deep in media-file related stuff at the moment.

comment:23 by Jannis Leidel, 14 years ago

milestone: 1.3
Patch needs improvement: set

This looks good, only needs to be expanded to the new STATIC_URL setting. Not sure if it's sensible to introduce a BaseSettings object just for this purpose.

comment:24 by Jannis Leidel, 14 years ago

Owner: changed from Jannis Leidel to Chris Heisel

Reassigning just for the sake of not-stepping-on-toes :)

comment:25 by Jannis Leidel, 14 years ago

Resolution: fixed
Status: newclosed

(In [15130]) Fixed #6218 -- Made MEDIA_URL and STATIC_URL require a trailing slash to ensure there is a consistent way to combine paths in templates. Thanks to Michael Toomim, Chris Heisel and Chris Beaven.

comment:26 by Jacob, 13 years ago

milestone: 1.3

Milestone 1.3 deleted

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