Opened 13 years ago
Closed 10 years ago
#17085 closed New feature (fixed)
Deprecate "add_to_builtins" and turn builtins into a property of Engine
Reported by: | Carl Meyer | Owned by: | nobody |
---|---|---|---|
Component: | Template system | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Aymeric Augustin | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
add_to_builtins
modifies global process state. This is to be avoided whenever possible, and in this case there would be a much better alternative: provide an API on a Template object to inject templatetag libraries as "built-in" for that Template. This provides the same effect as add_to_builtins
but in a localized and explicit way. If the tag library should be effectively built-in for the whole project, just use a custom render_to_response
equivalent (or TemplateResponse
subclass) that uses this API to inject the library for each Template rendered.
Deprecating add_to_builtins
is another step on the path of cleaning up globally-stored settings-related state in Django, which needs to be done if we're going to clean up settings as process-global.
Change History (9)
comment:1 by , 13 years ago
Summary: | Deprecate "add_to_builtins" → Deprecate "add_to_builtins" and replace with API on Template |
---|
comment:2 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 13 years ago
This ticket was based on the false premise that add_to_builtins
was documented API - I really thought I'd seen it in a corner of the docs somewhere. Given that it's not, I think it should just be removed, and we don't need to add an API to replace it.
comment:4 by , 13 years ago
Resolution: | → duplicate |
---|---|
Status: | new → closed |
Closing this as dupe of #17093 since dealing with the add_to_builtins problem makes the most sense in the context of that refactor.
comment:5 by , 10 years ago
Cc: | added |
---|---|
Resolution: | duplicate |
Status: | closed → new |
Triage Stage: | Accepted → Unreviewed |
Reopening this since #17093 was closed and add_to_builtins()
still exists so maybe this is worth addressing?
I came across the issue in trying to test djangoproject.com with Django 1.8 and saw that django-hosts isn't compatible as it imports from django.template import add_to_builtins
which is no longer there due to the namespace cleanup.
comment:6 by , 10 years ago
Summary: | Deprecate "add_to_builtins" and replace with API on Template → Deprecate "add_to_builtins" and turn builtins into a property of Engine |
---|---|
Triage Stage: | Unreviewed → Accepted |
When I worked on #17093 I treated template tag and filter libraries (i.e. myapp/templatetags/*.py
) like Python modules. Each Python module is referenced in sys.modules
and can be imported as needed. Likewise, each library is referenced in django.template.base.libraries
and can be loaded in templates as needed.
To give some background on the issue, builtin libraries are simply added to django.template.base.builtins
as follows:
builtins = [] def add_to_builtins(module): builtins.append(import_library(module)) add_to_builtins('django.template.defaulttags') add_to_builtins('django.template.defaultfilters') add_to_builtins('django.template.loader_tags')
If we want to discourage adding libraries to builtins, this code can be simplified to:
builtins = [ import_library('django.template.defaulttags'), import_library('django.template.defaultfilters'), import_library('django.template.loader_tags'), ]
However it will still be trivial to add things to builtins by appending to this list if you want to, so this breaks third-party code without providing real advantages in terms of isolation. I'm against doing this.
Turning builtins
into a property of the Engine
class would be much more interesting.
That's easy because builtins is only used in
Parser.init which is only called from
Engine.compile_string`.
Then extra builtins could be configured like this:
TEMPLATES = [ { 'BACKEND': 'django.template.backends.django.DjangoTemplates', 'APP_DIRS': True, 'OPTIONS': { 'extra_builtins': [ 'foobar.templatetags', ], }, }, ]
With this solution, authors of pluggable apps will have the choice between using {% load ... %}
in their templates (and the only excuse for not doing so is laziness) or documenting that their users must configure some extra builtins in TEMPLATES. Developers of projects can easily add builtins for use within the project.
I understand that adding builtins sounds horrific from the perspective of a Python developer, but within a given project and in templates it seems acceptable:
- templates have tons of builtins already and it isn't a problem because the situation is more simple than with Python imports: you don't go read the source of templatetags, unless it shows up in the debug page
- as long as the extra builtins are defined within the same project, they're just a "Find-all" away (with the caveat that you can still shoot yourself into the foot by adding third-party tags in extra builtins...)
comment:7 by , 10 years ago
Has patch: | set |
---|
https://github.com/django/django/pull/4657 implements this.
comment:8 by , 10 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
add_to_builtins is not a documented function - but seems to have made it into use extensive in the wild.
its primary use currently in Django is either:
add the truly global builtin tags and filters in template/base.py
add template tags into builtin space as a convenience for tests
the API you propose sounds like a python level alternative to the load tag.
From the context of the template, this is magic and far less explicit than using the load tag
It looks like most of the wild use of add_to_builtins is to provide "convenience" by 3rd party apps to make their tags global. But I would say that they should be re-documented as requiring the load tag in the users base.html if they want it everywhere
I think a deprecation of add_to_builtins with a replacement of _add_to_builtins could be done to get the message. But I think we should be encouraging more explicitness in the template space, not enabling less.