Opened 10 years ago

Closed 8 years ago

#1278 closed enhancement (fixed)

MEDIA_URL needs to be accessible from templates

Reported by: matt Owned by: adrian
Component: Template system Version: master
Severity: minor Keywords:
Cc: adrian, SmileyChris Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I didn't think of the potential security implications of this until I wrote it up (think {% get_setting DATABASE_PASSWORD %}), but I think it could prove quite useful as a default templatetag.

class SettingNode(Node):
    def __init__(self, varname, setting):
        self.setting = setting
        self.varname = varname

    def render(self, context):
        context[self.varname] = self.setting
        return ""
        
#@register.tag
def get_setting(parser, token):
    """
    Retrieve a setting from the current settings file.  The optional as clause can control
    the variable to be added to the context, otherwise the setting asked for witll be added to
    the context.  For example::
        
        {% get_setting MEDIA_URL [as varname] %}
        
    If the setting does not exist in the current settings file, ``None`` will be returned.  The
    following example will return a bulleted list of installed apps::
    
        {% get_setting INSTALLED_APPS %}
        <ul>
        {% for app in INSTALLED_APPS %}
            <li>{{ app }}</li>
        {% endfor %}
        </ul>   
    """   
    bits = token.contents.split()
    if len(bits) == 2:
        varname = bits[1]
    elif len(bits == 4):
        varname = bits[3]
    else:
        raise TemplateSyntaxError, "'%s' takes either one or three arguments." % bits[0]
    try:
        setting = bits[1]
        imp = __import__('django.conf.settings', locals(), globals(), bits[1])
        retsetting = getattr(imp, bits[1])
    except AttributeError:
        retsetting = None
    return SettingNode(varname, retsetting)

get_setting = register.tag('get_setting', get_setting)

Attachments (3)

template_context_constants.patch (3.2 KB) - added by SmileyChris 9 years ago.
patch with tests
template_context_constants.2.patch (3.1 KB) - added by SmileyChris 9 years ago.
tiny fix
1278.diff (2.8 KB) - added by ubernostrum 9 years ago.
Patch adding a 'media' context processor and associated documentation

Download all attachments as: .zip

Change History (24)

comment:1 Changed 10 years ago by adrian

I don't think it should be a default tag, but maybe we can put it contrib for people who want it.

comment:2 Changed 10 years ago by matt

Here's a modified version that uses get_safe_settings from django.views.debug . It would probably be best to move get_safe_settings to somewhere in django.utils and call that from both the debug view and this templatetag.

from django.views.debug import get_safe_settings
class SettingNode(Node):
    def __init__(self, varname, setting):
        self.setting = setting
        self.varname = varname

    def render(self, context):
        context[self.varname] = self.setting
        return ""
        
#@register.tag
def get_setting(parser, token):
    """
    Retrieve a setting from the current settings file.  The optional as clause can control
    the variable to be added to the context, otherwise the setting asked for witll be added to
    the context.  For example::
        
        {% get_setting MEDIA_URL [as varname] %}
        
    If the setting does not exist in the current settings file, ``None`` will be returned.  The
    following example will return a bulleted list of installed apps::
    
        {% get_setting INSTALLED_APPS %}
        <ul>
        {% for app in INSTALLED_APPS %}
            <li>{{ app }}</li>
        {% endfor %}
        </ul>   
    """   
    bits = token.contents.split()
    if len(bits) == 2:
        varname = bits[1]
    elif len(bits) == 4:
        varname = bits[3]
    else:
        raise TemplateSyntaxError, "'%s' takes either one or three arguments." % bits[0]
    try:
        settings = get_safe_settings()
        retsetting = settings[bits[1]]
    except KeyError:
        retsetting = None
    return SettingNode(varname, retsetting)

get_setting = register.tag('get_setting', get_setting)

comment:3 Changed 9 years ago by adrian

  • priority changed from normal to lowest
  • Severity changed from normal to minor

comment:4 Changed 9 years ago by anonymous

  • priority changed from lowest to normal

This would be very useful when developing - getting rid of hardcoded media paths being the major example. It's easy to pass <project>.settings to views in render and use settings.MEDIA_URL, but you can't do this for generic views.

comment:5 Changed 9 years ago by adrian

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

Note that this tag can be written a lot more easily now with the simple_tag decorator.

And I'm marking it as a wontfix because if it goes into trunk, it'd be too easy for a template author to get sensitive settings such as the database password.

comment:6 Changed 9 years ago by Simon Litchfield <simon@…>

  • Cc adrian added
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Summary changed from Potentially useful get_setting templatetag to MEDIA_URL needs to be accessible from templates

As a new user, I was shocked and frustrated to find that MEDIA_URL wasn't available in templates. And that you needed to write into every context, or make a custom tag, or context_processor just to get it! It's a seriously painful unnecessary complication and stumbling block for new users. Now I've got RequestContext crap all through my code for no good reason at all. What happened to DRY, less code is better? 99% of templates need access to this setting.

Obviously we don't want template authors having access to all settings though. Goes without saying.

Can I suggest either we select a few fundamentally important settings for template authors, and make them available via {{ settings.MEDIA_URL }} or even simply {{ MEDIA_URL }}. Or perhaps better still, to make it more self-explanatory, maybe we define a list in settings.py --

Example --

TEMPLATE_SETTINGS = ['MEDIA_URL', 'GOOGLE_MAPS_KEY', 'SOME_OTHER_SETTING']

Or, if we prefer to be more specific --

TEMPLATE_CONTEXT_CONSTANTS = {'MEDIA_URL': MEDIA_URL, 'GOOGLE_MAPS_KEY': GOOGLE_MAPS_KEY ... }

comment:7 Changed 9 years ago by ubernostrum

Even if Django shipped a built-in context processor for retrieving some settings, you'd still need to use RequestContext to get them to show up in your template contexts; there's no way around that. I do think there's utility in deciding on a set of default settings like MEDIA_URL, etc. which are safe and which could have a built-in context processor.

Adding things to template contexts which are specific to the needs of your application is the job of custom context processors in your application; that's explicitly what context processors and RequestContext are designed for.

comment:8 Changed 9 years ago by Simon Litchfield <simon@…>

Agreed, but context processors and RequestContext are overly and needlessly clunky for settings like MEDIA_URL which, lets be realistic for a second, are needed in 99.9% of all half serious web sites.

This is definitely only one of those 'little things'; but, it's one of those little things that's important, particularly for new users, and needs to be done right. See my post above for some suggestions on how to handle.

Come on people, using MEDIA_URL is not an exception here at all, it's the rule; and a good practice that should be encouraged by the framework right from square one.

comment:9 Changed 9 years ago by adrian

  • Resolution set to wontfix
  • Status changed from reopened to closed

I don't buy the argument that 99.9% of all half serious Web sites "need" this setting. I haven't needed it in years.

Marking it as a wontfix, again.

Changed 9 years ago by SmileyChris

patch with tests

Changed 9 years ago by SmileyChris

tiny fix

comment:10 Changed 9 years ago by SmileyChris

For posterity, here is Simon's idea as a patch (which should have read "patch with docs").

Adrian, how do you link to your CSS in a template correctly without providing MEDIA_URL (or STATIC_URL, etc) to your templates or (ugh) hard-coding it?

comment:11 Changed 9 years ago by Simon Litchfield <simon@…>

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Yes yes, enlighten us, how do we link media without MEDIA_URL? I see the reason for wontfix last time was security. That's pretty obvious. Whats the reason this time?

comment:12 Changed 9 years ago by Simon Litchfield <simon@…>

Nice work Smiley, your patch rocks :-)

comment:13 Changed 9 years ago by adrian

  • Resolution set to wontfix
  • Status changed from reopened to closed

Hi Simon: My reasoning is, it's feature creep.

Please don't reopen this ticket. I've marked it as a wontfix.

comment:14 Changed 9 years ago by Simon Litchfield <simon@…>

  • Cc SmileyChris added

Hey Adrian. Sorry to be a pain! Lets assume for a second it's feature creep. The question remains. How do we link media without MEDIA_URL?

comment:15 Changed 9 years ago by SmileyChris

Simon, you may be better off taking this discussion to the django developers group.

comment:16 Changed 9 years ago by anonymous

I don't really see a need for a {% media_url %} either. The get_img_tag method figures out by itself where the static dir is. If you need other methods to figure it out, it's easy too.

comment:17 Changed 9 years ago by Simon Litchfield <simon@…>

Not talking about imagefields and the like here, this is for all your flat files like css, logos/interface images, stock js's, etc. Hard-coding those into templates doesn't make sense; the framework encourages pluggability and separated delivery.

Anyway agreed the _constants idea is creep; _processors already handles that.

Still think MEDIA_URL should be a core tag, or made available through a default context_processor though, or something like that; so newbies in particular are encouraged to use it straight up. Theres numerous blogs and such out there showing workarounds, and plenty of queries have come in over it...

Changed 9 years ago by ubernostrum

Patch adding a 'media' context processor and associated documentation

comment:18 Changed 8 years ago by Marc Fargas <telenieko@…>

  • Triage Stage changed from Unreviewed to Design decision needed

comment:19 Changed 8 years ago by russellm

  • Resolution wontfix deleted
  • Status changed from closed to reopened

Putting MEDIA_URL in RequestContext has been blessed by Adrian.

comment:20 Changed 8 years ago by Marc Fargas <telenieko@…>

  • Has patch set
  • Triage Stage changed from Design decision needed to Accepted

comment:21 Changed 8 years ago by russellm

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

(In [5379]) Fixed #1278 -- Added a context processor that puts MEDIA_URL in the context, and added that context processor to the default set. Thanks to Ubernostrum for the patch.

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