Opened 8 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 Chris Cogdon, 8 years ago

Owner: changed from nobody to Chris Cogdon
Status: newassigned

comment:2 by Chris Cogdon, 8 years ago

Has patch: set
Triage Stage: UnreviewedAccepted

Pull request: https://github.com/django/django/pull/5892

All tests pass. Documentation compiles.

comment:3 by Chris Cogdon, 8 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?

One possibility is we can detect when both template_name and template_field_name are defined and raise a deprecation warning, and then fix the stack in a later version. If that's preferable, I'll make the change, and would we want it deprecated for 1.10 or 1.11 ?

Last edited 8 years ago by Chris Cogdon (previous) (diff)

comment:4 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:5 by Tim Graham, 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.

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