Opened 8 years ago

Closed 6 years ago

Last modified 4 years ago

#6587 closed (fixed)

remove django.templates __path__ hacking

Reported by: oyvind Owned by: andrewbadr
Component: Template system Version: master
Severity: Keywords: path hacking remove
Cc: cgrady@…, mathieu.damours@…, sdeasey@…, andrewbadr.etc@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

As mentioned in VersionOneFeatures

I made a attempt at fixing it, posting git patch

Attachments (16)

new_template_lib_loader.diff (2.8 KB) - added by oyvind 8 years ago.
new template library loader
new_template_lib_loader_2.diff (3.3 KB) - added by oyvind 8 years ago.
better error messages, no conflicting library names, better naming of variables
new_template_lib_loader_3.diff (3.4 KB) - added by oyvind 8 years ago.
Do not walk subdirs in os.walk
new_template_lib_loader_4.diff (4.5 KB) - added by oyvind 8 years ago.
get_library no longer uses django.templatetags.module
new_template_lib_loader_5.diff (5.1 KB) - added by oyvind 8 years ago.
More use of import, tests pass
new_template_lib_loader_6.diff (4.5 KB) - added by oyvind 8 years ago.
populate app_label in templatetags/init..py, import_library is own function
new_template_lib_loader_7.diff (5.0 KB) - added by oyvind 7 years ago.
better naming of variables, some code fixes, added some docstrings
new_template_lib_loader_8.diff (4.6 KB) - added by oyvind 7 years ago.
All tests pass
new_template_lib_loader_9.diff (4.5 KB) - added by oyvind 7 years ago.
removed os import
new_template_lib_loader_10.diff (70.0 KB) - added by oyvind 7 years ago.
fixed import to get the correct module, moved defaulttags and defaultfilters to avoid circular imports, fixes #6579
new_template_lib_loader_11.diff (134.6 KB) - added by oyvind 7 years ago.
forgot to remove some files in the commit, again
new_template_lib_loader_12.diff (135.7 KB) - added by oyvind 7 years ago.
new loader using imp, may need some cleaup, and decide if add_to_builtins should change, and if a import should use a list instead of the getattr solution
new_template_lib_loader_13.diff (135.6 KB) - added by oyvind 7 years ago.
removed import using getattr
new_template_lib_loader_14.diff (135.0 KB) - added by oyvind 7 years ago.
Final patch, i hope :)
template_libs_t6587_r12288_v1.diff (145.9 KB) - added by andrewbadr 6 years ago.
t6587-r12291.1.diff (4.8 KB) - added by russellm 6 years ago.
RC1 of template load refactoring

Download all attachments as: .zip

Change History (49)

Changed 8 years ago by oyvind

new template library loader

Changed 8 years ago by oyvind

better error messages, no conflicting library names, better naming of variables

Changed 8 years ago by oyvind

Do not walk subdirs in os.walk

Changed 8 years ago by oyvind

get_library no longer uses django.templatetags.module

comment:1 Changed 8 years ago by oyvind

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

all tests in runtests.py templates now pass

comment:2 Changed 8 years ago by Simon Greenhill <dev@…>

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:3 Changed 8 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

A good start. Needs a little more tweaking, though.

Line 19 in the new version of templatetags/__init__.py can't be right. It looks like you're trying to replicate Python's import behaviour, but that's not the way to do it. I would be cautious of anything trying to do with this work without using at least some functions from Python's imp module. See below (the "Finally,..." paragraph) for more here: I feel a bit suspicious about the whole approach in this part of the code.

Lines 8 and 9 in the same file cry out for a better solution. One possible, probably for another ticket, is to add a "tags to load always" list in settings. This would help people who are poking into the builtin tags as well. We could add django.templatetags.* to that list.

Even simpler, and probably sufficient for the purposes of this ticket is to iterate over ['django'] + list(settings.INSTALLED_APPS). That avoids having some special list we have to remember to update if we add more tags in there.

Finally, the approach in get_library_names() looks complex. You're doing a lot of computation there in the hope of getting one hit on the returned dictionary when you try to load a tag. Is it possible to let Python do more of the work here. Loop over the list of directories (<app_name>+'.templatetags') and try appending what is being loaded to the end of that and importing. As soon as you don't get an import error, stop with success. That will also mean if you try to load the same template tags in multiple templates in the the same process, Python will just reuse the one already in memory, instead of walking all those directories again. Push less of the work down into get_library_names() and try to use Python's builtin import facilities (esp. __import__() a bit more.

comment:4 Changed 8 years ago by Collin Grady <cgrady@…>

  • Cc cgrady@… added

Changed 8 years ago by oyvind

More use of import, tests pass

Changed 8 years ago by oyvind

populate app_label in templatetags/init..py, import_library is own function

Changed 7 years ago by oyvind

better naming of variables, some code fixes, added some docstrings

Changed 7 years ago by oyvind

All tests pass

comment:5 Changed 7 years ago by telenieko

#3349 talks about ImportError in template loading.

comment:6 Changed 7 years ago by oyvind

This patch shows the tried modules, should the message be less specific, and ask about apps in INSTALLED_APPS as in #3349?

Changed 7 years ago by oyvind

removed os import

comment:7 Changed 7 years ago by telenieko

I prefer to know the tried modules, just pointed out #3349 in case it gets invalidated whe this one gets in.
But I doubt it now as I changed #3349 summary (as the patch fixed other places with obscure errors).

comment:8 Changed 7 years ago by oyvind

  • Owner changed from nobody to oyvind

comment:9 Changed 7 years ago by oyvind

  • Patch needs improvement unset
  • Status changed from new to assigned

comment:10 Changed 7 years ago by ramiro

See also #6579 regarding the use of [''] as the 4th. parameter to ___init__() (with some notes from the Python developers advising against its use.)

Changed 7 years ago by oyvind

fixed import to get the correct module, moved defaulttags and defaultfilters to avoid circular imports, fixes #6579

Changed 7 years ago by oyvind

forgot to remove some files in the commit, again

Changed 7 years ago by oyvind

new loader using imp, may need some cleaup, and decide if add_to_builtins should change, and if a import should use a list instead of the getattr solution

Changed 7 years ago by oyvind

removed import using getattr

Changed 7 years ago by oyvind

Final patch, i hope :)

comment:11 Changed 7 years ago by mtredinnick

  • Owner changed from oyvind to mtredinnick
  • Status changed from assigned to new

comment:12 Changed 7 years ago by Alex

  • milestone changed from 1.0 beta to post-1.0

1.0 beta has shipped and this was not fixed, will be handled post-1.0.

comment:13 Changed 7 years ago by mtredinnick

  • milestone changed from post-1.0 to 1.0

I realise people are only trying to help, but can we have a little restraint with triaging tickets off the feature list? Particularly when a maintainer is currently the assigned person. This is still on the 1.0 track. I can do it in a more-or-less backwards-compatible way, with luck.

comment:14 Changed 7 years ago by mtredinnick

  • milestone 1.0 deleted
  • Owner mtredinnick deleted
  • Patch needs improvement set

comment:15 Changed 7 years ago by anonymous

  • milestone set to post-1.0

comment:16 Changed 7 years ago by ramiro

#6579 was related and marked as duplicate.

comment:17 Changed 7 years ago by akaihola

#9413 is a symptom of this and has been marked as duplicate.

comment:18 Changed 7 years ago by matehat

Just a suggestion: if an app (say called 'chunkies') needs only one templatetag module, it would be better to put its content in the 'templatetags/_ _init_ _.py' module instead of forcing it into a new file, say 'templatetags/chunkiestags.py'. This way, we would be able to load it with {% load chunkies %} instead of {% load chunkiestags %}. I looked at the patch and it would only require a short change in 'django/template/_ _init_ _.py'

    if module.endswith("%s.%s" % (library_name, 'templatetags')):
        taglib_module = module
    else:
        taglib_module = str('%s.%s' % (module, library_name))

comment:19 Changed 7 years ago by matehat

  • Cc mathieu.damours@… added

comment:20 Changed 7 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:21 Changed 7 years ago by jacob

  • milestone set to 1.1 beta

comment:22 Changed 6 years ago by mtredinnick

  • milestone 1.1 beta deleted

No actual progress on this since 1.0 (yeah, we suck). We'll get to it one day, but not this time around.

comment:23 Changed 6 years ago by groks

  • Cc sdeasey@… added

comment:24 Changed 6 years ago by mrts

  • milestone set to 1.2

Re-targeting for 1.2.

comment:25 Changed 6 years ago by andrewbadr

  • Owner set to andrewbadr

can take a look for 1.2

comment:26 follow-up: Changed 6 years ago by andrewbadr

Oyvind, or anyone: how do I use the latest patch? What's up with the /dev/null stuff?

comment:27 in reply to: ↑ 26 Changed 6 years ago by ramiro

Replying to andrewbadr:

What's up with the /dev/null stuff?

Download the patch using the 'Original Format' link at the bottom of the page and examine it with your preferred text editor. The fact you see /dev/null files is because of an interaction between a) Behavior of some tools (like Git, Mercurial, ...?) that generate diffs containing a /dev/null path for the original file in the header when a new file has been added and b) Trac patch HTML renderer that picks and shows that specific header piece.

Patch should apply cleanly using GNU patch though.

Changed 6 years ago by andrewbadr

comment:28 Changed 6 years ago by andrewbadr

Here's a version of new_template_lib_loader_14.diff updated to work against trunk. This is my first time using git for patches so hopefully I didn't mess anything up.

Two important notes:

  • The defaulttags module isn't just moved; it is patched and moved at the same time. There is a one-line difference in the "load" function.
  • The templatetag admindocs break. I'll be getting to that later tonight. It was previously pretty tightly coupled to the internals, so I'll assume that's ok for the new version if I can't find a better solution.

It would be great if a committer could confirm that this patch's general approach is correct.

comment:29 Changed 6 years ago by andrewbadr

If you are looking at this patch...

  • Should get_templatetags_modules be using import_module or something besides a manual import? I don't know too much about that stuff.
  • The division of code feels wrong. I want to at least move get_templatetags_modules from django.templatetags to django.template (the only place it's used) and maybe even rename it to "get_templatelibrary_modules" (matching the terminology used in django.template). At that point, the django/templatetags folder would only contain libraries, so I'd want to move the directory to django/template/templatetags. (The first change has a stronger case.) Thoughts?

comment:30 Changed 6 years ago by russellm

@andrewbadr - Yes, you should be making better use of importlib.import_module. Essentially, any code where you are using imp directly and splitting on '.' - replace it with a call to import_module('full.qualified.module.name')

You need to be careful about moving big blocks of code around, though. The template tests currently fail, for example, because "from django.templatetags.defaultfilters import stringfilter" no longer works. I have plenty of code that imports and uses templates filters as functions. These tests may still have been passing for you if you had a stale .pyc file. django.template may not be the best place for this code, but its a location with historical significance; it can't be changed lightly.

I'll upload an updated patch shortly, which I'll probably commit tomorrow when my head is a little clearer. Other than the import_module and code relocation issues, I'm happy with the approach that has been taken.

Changed 6 years ago by russellm

RC1 of template load refactoring

comment:31 Changed 6 years ago by andrewbadr

  • Cc andrewbadr.etc@… added

Before committing, note the admindoc breakage I mentioned above and that Øyvind should get credit for writing the patch (I just translated it to trunk). I can work on the admindoc issue tomorrow.

comment:32 Changed 6 years ago by russellm

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

(In [12295]) Fixed #6587 -- Removed nasty path hacking in templatetag loading. Thanks to Øyvind Satvik and Andrew Badr for their work on this patch.

comment:33 Changed 4 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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