Opened 14 years ago

Last modified 12 months ago

#12772 new New feature

Allow loading template tags by fully qualified python module path

Reported by: Patryk Zawadzki Owned by:
Component: Template system Version: 1.2-beta
Severity: Normal Keywords:
Cc: marc.tamlyn@…, paluho@…, ramusus@…, brian@…, skylar.saveland@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Currently templatetags are magically searched for in the list of installed apps. This leads to all kind of problems when debugging code, starting from being forced to use find to locate tag libraries and ending with global namespace collisions.

The attached patch adds the possibility to import tags by fully qualified module path by first trying to make an absolute import and only then falling back to searching inside installed apps.

This also allows people to import tag libraries that are not parts of any application (so common tags can be kept together without the need of adding a fake app).

Also: "Explicit is better than implicit." :)

Attachments (4)

django-fully-qualified-template-tags.patch (1.5 KB ) - added by Patryk Zawadzki 14 years ago.
Proposed change
django-fully-qualified-template-tags.diff (1.5 KB ) - added by Skylar Saveland 14 years ago.
name file .diff, diff from the root of the repo, not django subdir
django-fully-qualified-template-tags-2.patch (2.6 KB ) - added by Patryk Zawadzki 14 years ago.
Modified patch
django-12772.patch (2.6 KB ) - added by Patryk Zawadzki 14 years ago.
Working patch

Download all attachments as: .zip

Change History (35)

by Patryk Zawadzki, 14 years ago

Proposed change

comment:1 by Johannes Dollinger, 14 years ago

Resolution: duplicate
Status: newclosed

Duplicate of #2539.

comment:2 by Patryk Zawadzki, 14 years ago

Resolution: duplicate
Status: closedreopened

It's not a duplicate.

#2539 is adding even more magic to the template loader.

My proposal here is removing the magic and eventually deprecating the old syntax. The "templatetags" dir should not be magic.

in reply to:  2 comment:3 by Johannes Dollinger, 14 years ago

Replying to patrys:

It's not a duplicate.

#2539 is adding even more magic to the template loader.

My proposal here is removing the magic and eventually deprecating the old syntax. The "templatetags" dir should not be magic.

Personally, I like your proposal better (one should get rid of app_label and flat namespaces everywhere).
But since #2539 is also about adding some kind of namespace support to template libraries, it makes sense to have a single ticket.

comment:4 by ramusus, 14 years ago

Cc: ramusus@… added

comment:5 by Alex Gaynor, 14 years ago

Triage Stage: UnreviewedDesign decision needed

comment:6 by paluh, 14 years ago

Cc: paluho@… added; ramusus@… removed

comment:7 by paluh, 14 years ago

Cc: ramusus@… added

comment:8 by unbracketed, 14 years ago

Cc: brian@… added

by Skylar Saveland, 14 years ago

name file .diff, diff from the root of the repo, not django subdir

comment:9 by Skylar Saveland, 14 years ago

Cc: skylar.saveland@… added
Needs documentation: set
Needs tests: set

comment:10 by Skylar Saveland, 14 years ago

Patch needs improvement: set

In revision 13020, the patch makes the test suite fail.

http://dpaste.com/hold/186990/

lib = import_library(taglib_module)

...

by Patryk Zawadzki, 14 years ago

Modified patch

comment:11 by Patryk Zawadzki, 14 years ago

Added a workaround around the workaround that was already there (the one trying to detect if the parent module contained the taglib).

comment:12 by Skylar Saveland, 14 years ago

The test suite fails with the new patch:

http://dpaste.com/hold/187378/

Also, it would be nice if you would use the .diff extension and run diff from the root of the django dir as specified:

http://docs.djangoproject.com/en/dev/internals/contributing/#patch-style

comment:13 by Skylar Saveland, 14 years ago

From testing with my test_project, I find that using the new style is working but, using the old style dumps errors.

so, using the old-syle with trunk, I can

{% load test_ttags %} and all is well.

With your patch, I can {% load test_app.templatetags.test_ttags %}

But, if I try to do it the old way,

'test_ttags' is not a valid tag library: ImportError raised loading test_ttags: No module named test_ttags

by Patryk Zawadzki, 14 years ago

Attachment: django-12772.patch added

Working patch

comment:14 by Patryk Zawadzki, 14 years ago

Attached a better patch, this time using full paths.

.patch is the usual extension for patches and trac handles that fine.

comment:15 by Julien Phalip, 13 years ago

Severity: Normal
Type: New feature

comment:16 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:17 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:18 by Aymeric Augustin, 11 years ago

Status: reopenednew

comment:19 by Jacob, 11 years ago

Triage Stage: Design decision neededAccepted

comment:20 by Patryk Zawadzki, 11 years ago

Owner: changed from nobody to Patryk Zawadzki
Status: newassigned

comment:22 by Diederik van der Boor, 11 years ago

I'm a bit worried about this change. What happens when someone manages to upload a .py file somewhere (e.g. in the media folder)?
With this patch, will it be possible to load that code?

Last edited 11 years ago by Diederik van der Boor (previous) (diff)

comment:23 by Patryk Zawadzki, 11 years ago

I assume the media folder is not a proper Python package so no, not unless you also manage to put an __init__.py on all the levels above the media folder and add it to sys.path. Also notice that this is only usable if you can edit the templates which means you already have access to the code.

Last edited 11 years ago by Patryk Zawadzki (previous) (diff)

comment:24 by Patryk Zawadzki, 9 years ago

Ping :)

comment:25 by Tim Graham, 9 years ago

I am not entirely convinced either way about the security concerns. For example, the documentation says we have settings.ALLOWED_INCLUDE_ROOTS because "This is a security measure, so that template authors can’t access files that they shouldn’t be accessing. It seems this opens up the same sort of issue where template authors can load arbitrary Python packages which shouldn't (but may) have side effects. It would be helpful to run this by the mailing list and see if a consensus emerges.

After that (assuming this isn't rejected), the patch needs to be updated to apply cleanly to master and then the Trac flags update so the patch appears in the review queue.

comment:26 by Carl Meyer, 9 years ago

There's a significant difference between settings.ALLOWED_INCLUDE_ROOTS and arbitrary imports. If we didn't have the former, a template author could access literally any file on the filesystem, as text. The latter only allows accessing files that are on sys.path within a valid Python package, and even then only in a limited way - by interpreting them as Python code. The former is much worse as a security risk.

That said, it has never been entirely clear (to me, anyway) whether "completely untrusted template author" is a use-case that Django intends to support out of the box. I doubt the template language currently meets that bar very well, so I think we'd have some work to do if the answer is "yes". But I do agree that if the answer to that is "yes", this patch should probably not go in.

Apart from the security concerns: while I fully agree that the flattened app-label namespace is silly and problematic, it feels strange to me to have Python import semantics reflected even more directly into the template engine. I prefer the Jinja2 approach, where the things that you can import from templates are other templates containing macros; if you want Python functions directly in the context, you have to put them there explicitly in Python code, you don't "import" Python modules from templates. The current DTL approach is less explicit than the Jinja2 approach, but at least with the current DTL approach the Python developer still has to explicitly nominate which modules should be available to the templates.

So I think I hover somewhere around +/-0 on this patch.

comment:27 by Tim Graham, 9 years ago

Created a thread to try to get some consensus on this: https://groups.google.com/d/topic/django-developers/4KEr_86Q3nA/discussion

comment:28 by Marc Tamlyn, 9 years ago

Cc: marc.tamlyn@… added

While I approve of magic removal, I'm not convinced I like the idea of importing by python path.

A possible solution to both any security concerns and namespace collisions would be to have TEMPLATE_TAG_LOADERS - one with the automatic behaviour which could be removed if you want to, and one which is configured via a mapping of library names to dotted paths to python files.

So current (default) behaviour:
TEMPLATE_TAG_LOADERS = ('django.template.tag_loaders.AppDirectoriesLoader',)

Magic removal version:

TEMPLATE_TAG_LOADERS = ('django.template.tag_loaders.ConfiguredLoader',)
TEMPLATE_TAG_LIBRARIES = {
    'admin.urls': 'django.contrib.admin.templatetags.admin_urls',
    # ... other built in libraries
    'my_tags': 'my_project.my_custom_templatetags',
}

These settings would actually not be yet more top level settings, but more configuration settings within Aymeric's template engines branch, which would make them a bit more palatable.

Not 100% sure this is a good idea, but I think it is worth considering. Most people would continue to use the magical version we have, but it would now be explicit, overridable magic.

comment:29 by Tim Graham, 9 years ago

See also #2539 for discussion on the problem of namespace collisions.

comment:30 by Collin Anderson, 9 years ago

Isn't there already a {% load mylibrary from myapp %} syntax?

I do feel like there's a ton of boilerplate for creating template tags. It would be nice to cut down on this:

mkdir templatetags
touch templatetags/__init__.py
vi templatetags/thing_to_name.py
from django import template
register = template.Library()
@register.simple_tag

Could we allow any python module to register template tags, and then it's up to you to be sure that gets imported before you use your template? So, any python module could do something like this?

register = template.Library('name_of_library')

Though, it still doesn't solve name collisions.

comment:31 by Patryk Zawadzki, 12 months ago

Owner: Patryk Zawadzki removed
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top