Opened 9 years ago
Last modified 8 years ago
#26007 assigned Bug
SingleObjectTemplateResponseMixin.get_template_names does not return names stack properly.
Reported by: | Chris Cogdon | Owned by: | Chris Cogdon |
---|---|---|---|
Component: | Generic views | Version: | 1.9 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
The docstring for SingleObjectTemplateResponseMixin.get_template_names() implies that a particular list of names is being returned. However, if SingleObjectTemplateResponseMixin.get_template_names() (the super) returns anything, then only the template_name will be returned.
In addition, the name obtained by using self.template_name_field is inserted at 0, implying that there is something to insert it in front of, which is impossible given the current implementation, and conflicts with the documentation.
By inference, it appears that the intended implementation is that the following names are returned, in this order:
- a template derived from the template_name_field field of the object.
- a template derived from the super (the template_name) attribute of the view
- a template derived from the object's model name or self.model's name, with the template_name_suffix appended.
Change will need some refactoring of the method, change to the docstring, and the documentation. I'll provide a pull request.
Change History (5)
comment:1 by , 9 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:2 by , 9 years ago
Has patch: | set |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:3 by , 9 years ago
Note: This patch introduces one potential backwards incompatibility:
In the current code, if both template_name and template_field_name is set, template_field_name is always ignored. With the patch, if template_field_name is declared AND there is a non-empty value in the referenced field, the template name from the field is used in preference to template_name.
I think the patch implements the intended behaviour (or why bother trying to insert(0, name)
, and compare the implementation in MultipleObjectTemplateResponseMixin.get_template_names
), and cases were someone has set both template_name and template_field_name is rare or non-existant, but given this possibility, we may want to defer the change to 1.10 with or without accelerated deprecation. Advise?
comment:4 by , 9 years ago
Patch needs improvement: | set |
---|
comment:5 by , 8 years ago
As noted on the PR, I don't see what implies that template_name_field
should be first? Even if the code implies some other behavior, it seems to me that the the documentation and docstring should take precedence.
Pull request: https://github.com/django/django/pull/5892
All tests pass. Documentation compiles.