Opened 11 years ago

Closed 9 years ago

#1278 closed enhancement (fixed)

MEDIA_URL needs to be accessible from templates

Reported by: matt Owned by: Adrian Holovaty
Component: Template system Version: master
Severity: minor Keywords:
Cc: Adrian Holovaty, Chris Beaven 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 Chris Beaven 10 years ago.
patch with tests
template_context_constants.2.patch (3.1 KB) - added by Chris Beaven 10 years ago.
tiny fix
1278.diff (2.8 KB) - added by James Bennett 10 years ago.
Patch adding a 'media' context processor and associated documentation

Download all attachments as: .zip

Change History (24)

comment:1 Changed 11 years ago by Adrian Holovaty

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 11 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 10 years ago by Adrian Holovaty

priority: normallowest
Severity: normalminor

comment:4 Changed 10 years ago by anonymous

priority: lowestnormal

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 10 years ago by Adrian Holovaty

Resolution: wontfix
Status: newclosed

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 10 years ago by Simon Litchfield <simon@…>

Cc: Adrian Holovaty added
Resolution: wontfix
Status: closedreopened
Summary: Potentially useful get_setting templatetagMEDIA_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 10 years ago by James Bennett

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 10 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 10 years ago by Adrian Holovaty

Resolution: wontfix
Status: reopenedclosed

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

patch with tests

Changed 10 years ago by Chris Beaven

tiny fix

comment:10 Changed 10 years ago by Chris Beaven

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 10 years ago by Simon Litchfield <simon@…>

Resolution: wontfix
Status: closedreopened

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 10 years ago by Simon Litchfield <simon@…>

Nice work Smiley, your patch rocks :-)

comment:13 Changed 10 years ago by Adrian Holovaty

Resolution: wontfix
Status: reopenedclosed

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 10 years ago by Simon Litchfield <simon@…>

Cc: Chris Beaven 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 10 years ago by Chris Beaven

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

comment:16 Changed 10 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 10 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 10 years ago by James Bennett

Attachment: 1278.diff added

Patch adding a 'media' context processor and associated documentation

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

Triage Stage: UnreviewedDesign decision needed

comment:19 Changed 9 years ago by Russell Keith-Magee

Resolution: wontfix
Status: closedreopened

Putting MEDIA_URL in RequestContext has been blessed by Adrian.

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

Has patch: set
Triage Stage: Design decision neededAccepted

comment:21 Changed 9 years ago by Russell Keith-Magee

Resolution: fixed
Status: reopenedclosed

(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