[Django] #17085: Deprecate "add_to_builtins"
Django
-------------------------------------------+------------------------
Reporter: carljm | Owner: nobody
Type: New feature | Status: new
Component: Template system | Version: SVN
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------+------------------------
`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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17085>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
---------------------------------+--------------------------------------
Type: New feature | Status: new
Component: Template system | Version: SVN
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/17085#comment:1>
Django
---------------------------------+------------------------------------
Type: New feature | Status: new
Component: Template system | Version: SVN
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by ptone):
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17085#comment:2>
Django
---------------------------------+------------------------------------
Type: New feature | Status: new
Component: Template system | Version: SVN
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17085#comment:3>
Django
Reporter: carljm | Owner: nobody
Type: New feature | Status: closed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* status: new => closed
* resolution: => duplicate
Comment:
Closing this as dupe of #17093 since dealing with the add_to_builtins
problem makes the most sense in the context of that refactor.
--
Ticket URL: <https://code.djangoproject.com/ticket/17085#comment:4>
Django
Reporter: carljm | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* cc: aaugustin (added)
* status: closed => new
* resolution: duplicate =>
* stage: Accepted => Unreviewed
Comment:
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 [https://github.com/jezdez/django-
hosts/blob/c12a343473ab8633e09b6bc716b67e0eaafc76e7/django_hosts/apps.py#L23
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/17085#comment:5>
Django
---------------------------------+------------------------------------
Reporter: carljm | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* stage: Unreviewed => Accepted
Comment:
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...)
--
Ticket URL: <https://code.djangoproject.com/ticket/17085#comment:6>
Django
---------------------------------+------------------------------------
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by aaugustin):
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/4657 implements this.
--
Ticket URL: <https://code.djangoproject.com/ticket/17085#comment:7>
Django
Reporter: carljm | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/17085#comment:8>
Django
-------------------------------------+-------------------------------------
Type: New feature | Status: closed
Component: Template system | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Preston Timmons <prestontimmons@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"655f52491505932ef04264de2bce21a03f3a7cd0" 655f5249]:
{{{
#!CommitTicketReference repository=""
revision="655f52491505932ef04264de2bce21a03f3a7cd0"
Fixed #17085, #24783 -- Refactored template library registration.
* Converted the ``libraries`` and ``builtins`` globals of
``django.template.base`` into properties of the Engine class.
* Added a public API for explicit registration of libraries and builtins.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/17085#comment:9>