Opened 11 years ago
Closed 22 months ago
#24128 closed Bug (fixed)
Admindocs doesn't account for template loaders
| Reported by: | Aymeric Augustin | Owned by: | Alexander Lazarević |
|---|---|---|---|
| Component: | contrib.admindocs | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Kris Avi, Sarah Abderemane | 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
TemplateDetailView only considers dirs and assumes that the filesystem loader is enabled. It doesn't account for other loaders such as app_directories.
The code changed a bit during the multiple-template-engines refactor but this bug existed before and still exists.
Change History (15)
comment:1 by , 11 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:2 by , 9 years ago
| Cc: | added |
|---|
comment:4 by , 9 years ago
| Needs tests: | unset |
|---|
comment:5 by , 9 years ago
| Patch needs improvement: | set |
|---|
comment:7 by , 22 months ago
| Owner: | changed from to |
|---|---|
| Status: | new → assigned |
I applied the patch and all tests pass on my machine.
comment:8 by , 22 months ago
As an example I get the following for http://localhost:8000/admin/doc/templates/base.html/ without the patch:
Template: "base.html"
Search path for template "base.html":
/home/laza/Projects/Django/empty/templates/base.html
and with the patch:
Template: "base.html"
Search path for template "base.html":
/home/laza/Projects/Django/django/django/contrib/admin/templates/base.html (does not exist)
/home/laza/Projects/Django/empty/templates/base.html
/home/laza/Projects/Django/django/django/contrib/admindocs/templates/base.html (does not exist)
/home/laza/Projects/Django/django/django/contrib/auth/templates/base.html (does not exist)
So that's what the fix is about? Right now I'm not sure how to write a test for this.
comment:9 by , 22 months ago
Just saw that there is also an old PR for the ticket: https://github.com/django/django/pull/7434
comment:11 by , 22 months ago
| Cc: | added |
|---|---|
| Patch needs improvement: | unset |
| Triage Stage: | Accepted → Ready for checkin |
comment:12 by , 22 months ago
| Patch needs improvement: | set |
|---|---|
| Triage Stage: | Ready for checkin → Accepted |
comment:13 by , 22 months ago
| Patch needs improvement: | unset |
|---|
I applied the recommendations on the PR
comment:14 by , 22 months ago
| Triage Stage: | Accepted → Ready for checkin |
|---|
It seems it is quite fixable with something like that:
That is probably not the most elegant solution. Tried to get loaders from all the engines and combine their location directories into one to send to enumerator.
... pass else: # This doesn't account for template loaders (#24128). # Fix for #24128 from django.template import engines directories = set(default_engine.dirs) # making set with default engine dirs parameter as initial set for engine in engines.all(): # going through all the engines for loader in engine.engine.template_loaders: # getting each template loader from engine directories.update(loader.get_dirs()) # updating set with new directories from loaders for index, directory in enumerate(directories): # new enumerator with all dirs from default and loaders # for index, directory in enumerate(default_engine.dirs): # commented out original enumerator # End of fix for #24128 template_file = os.path.join(directory, template) templates.append({ ...instead of
... pass else: # This doesn't account for template loaders (#24128). for index, directory in enumerate(default_engine.dirs): template_file = os.path.join(directory, template) templates.append({ ...