Opened 9 years ago

Closed 6 years ago

Last modified 5 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: master
Severity: Keywords: easy-pickings
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

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@…> 9 years ago.
Patch for base.py and settings.py template docs
ticket-6218.patch (2.1 KB) - added by Chris Heisel 6 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 6 years ago.
6218.diff (7.0 KB) - added by Chris Beaven 6 years ago.
t6218b.patch (7.2 KB) - added by Chris Heisel 6 years ago.
Smiley's patch, but updated to use PendingDeprecationWarning

Download all attachments as: .zip

Change History (31)

Changed 9 years ago by Michael Toomim <toomim@…>

Attachment: dj.slash.patch added

Patch for base.py and settings.py template docs

comment:1 Changed 9 years ago by Malcolm Tredinnick

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 Changed 9 years ago by Michael Toomim <toomim@…>

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 Changed 9 years ago by Malcolm Tredinnick

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 Changed 9 years ago by Chris Beaven

Keywords: easy-pickings added

comment:5 Changed 9 years ago by Leo Shklovskii

Owner: changed from nobody to Leo Shklovskii
Status: newassigned

comment:6 Changed 8 years ago by Leo Shklovskii

Owner: Leo Shklovskii deleted
Status: assignednew

comment:7 Changed 6 years ago by Chris Heisel

Owner: set to Chris Heisel

Changed 6 years ago by Chris Heisel

Attachment: ticket-6218.patch added

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

comment:8 Changed 6 years ago by Chris Heisel

Patch needs improvement: unset
Status: newassigned

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

comment:9 Changed 6 years ago by Chris Heisel

Needs documentation: set
Patch needs improvement: set

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

Changed 6 years ago by Chris Heisel

Attachment: ticket-6218.2.patch added

comment:10 Changed 6 years ago by Chris Heisel

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 Changed 6 years ago by Eric Holscher

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 Changed 6 years ago by Chris Beaven

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 Changed 6 years ago by toomim

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?

Changed 6 years ago by Chris Beaven

Attachment: 6218.diff added

comment:14 Changed 6 years ago by Chris Beaven

Owner: Chris Beaven deleted
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 Changed 6 years ago by Eric Holscher

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 Changed 6 years ago by Chris Heisel

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 Changed 6 years ago by Chris Beaven

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 Changed 6 years ago by Chris Beaven

Triage Stage: Design decision neededAccepted

comment:19 Changed 6 years ago by toomim

Thanks SmileyChris

comment:20 Changed 6 years ago by Chris Heisel

Owner: set to Chris Heisel
Status: newassigned

Thanks SmileyChris! I'll work up a patch.

Changed 6 years ago by Chris Heisel

Attachment: t6218b.patch added

Smiley's patch, but updated to use PendingDeprecationWarning

comment:21 Changed 6 years ago by Chris Heisel

Patch needs improvement: unset

comment:22 Changed 6 years ago by Chris Beaven

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 Changed 6 years ago by Jannis Leidel

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 Changed 6 years ago by Jannis Leidel

Owner: changed from Jannis Leidel to Chris Heisel

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

comment:25 Changed 6 years ago by Jannis Leidel

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 Changed 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

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