Require trailing slash always on MEDIA_URL

[Michael Toomim <toomim@…>]

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.

[Michael Toomim <toomim@…>]

Patch for base.py and settings.py template docs

[Malcolm Tredinnick]

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.

[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.

[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.

[Chris Heisel]

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

[Chris Heisel]

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

[Chris Heisel]

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

[Chris Heisel]

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

[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.

[Chris Beaven]

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.

[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?

[Chris Beaven]

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.

[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.

[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)"

[Chris Beaven]

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...

[toomim]

Thanks SmileyChris

[Chris Heisel]

Thanks SmileyChris! I'll work up a patch.

[Chris Heisel]

Smiley's patch, but updated to use PendingDeprecationWarning

[Chris Beaven]

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

[Jannis Leidel]

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.

[Jannis Leidel]

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

[Jannis Leidel]

(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.

[Jacob]

Milestone 1.3 deleted