Opened 7 years ago

Closed 4 years ago

Last modified 3 years ago

#6218 closed (fixed)

Require trailing slash always on MEDIA_URL

Reported by: Michael Toomim <toomim@…> Owned by: cmheisel
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@…> 7 years ago.
Patch for base.py and settings.py template docs
ticket-6218.patch (2.1 KB) - added by cmheisel 4 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 cmheisel 4 years ago.
6218.diff (7.0 KB) - added by SmileyChris 4 years ago.
t6218b.patch (7.2 KB) - added by cmheisel 4 years ago.
Smiley's patch, but updated to use PendingDeprecationWarning

Download all attachments as: .zip

Change History (31)

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

Patch for base.py and settings.py template docs

comment:1 Changed 7 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

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 7 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 7 years ago by mtredinnick

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 7 years ago by SmileyChris

  • Keywords easy-pickings added

comment:5 Changed 7 years ago by Leo

  • Owner changed from nobody to Leo
  • Status changed from new to assigned

comment:6 Changed 6 years ago by Leo

  • Owner Leo deleted
  • Status changed from assigned to new

comment:7 Changed 4 years ago by cmheisel

  • Owner set to cmheisel

Changed 4 years ago by cmheisel

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

comment:8 Changed 4 years ago by cmheisel

  • Patch needs improvement unset
  • Status changed from new to assigned

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

comment:9 Changed 4 years ago by cmheisel

  • Needs documentation set
  • Patch needs improvement set

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

Changed 4 years ago by cmheisel

comment:10 Changed 4 years ago by cmheisel

  • 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 4 years ago by ericholscher

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 4 years ago by SmileyChris

  • Owner changed from cmheisel to SmileyChris
  • Status changed from assigned to new

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 4 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 4 years ago by SmileyChris

comment:14 Changed 4 years ago by SmileyChris

  • Owner SmileyChris deleted
  • Triage Stage changed from Accepted to Design 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 4 years ago by ericholscher

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 4 years ago by cmheisel

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 4 years ago by SmileyChris

  • 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 4 years ago by SmileyChris

  • Triage Stage changed from Design decision needed to Accepted

comment:19 Changed 4 years ago by toomim

Thanks SmileyChris

comment:20 Changed 4 years ago by cmheisel

  • Owner set to cmheisel
  • Status changed from new to assigned

Thanks SmileyChris! I'll work up a patch.

Changed 4 years ago by cmheisel

Smiley's patch, but updated to use PendingDeprecationWarning

comment:21 Changed 4 years ago by cmheisel

  • Patch needs improvement unset

comment:22 Changed 4 years ago by SmileyChris

  • Owner changed from cmheisel to jezdez
  • Status changed from assigned to new

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 4 years ago by jezdez

  • milestone set to 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 4 years ago by jezdez

  • Owner changed from jezdez to cmheisel

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

comment:25 Changed 4 years ago by jezdez

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

(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 3 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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