Code

Opened 8 years ago

Closed 22 months ago

Last modified 16 months ago

#2384 closed defect (fixed)

Changing TEMPLATE_STRING_IF_INVALID breaks Admin Interface display of model list

Reported by: robbie@… Owned by: adrian
Component: contrib.admin Version: master
Severity: normal Keywords:
Cc: cmawebsite@… Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Model functions (as opposed to fields) used in "list_display" for the Admin interface lack the attribute "class_attrib". If TEMPLATE_STRING_IF_INVALID is changed from the default "" the new string is inserted at the end of the column's <th> tag, causing the model's list table to break on rendering.

Affects Admin interface template:

contrib/admin/templates/admin/change_list_results.html

Attachments (0)

Change History (4)

comment:1 Changed 8 years ago by russellm

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

(In [3901]) Fixes #2384,#2566 -- Clarify the role that TEMPLATE_STRING_IF_INVALID plays in the template system, and the problems that can occur if it is used on a production site.

comment:2 Changed 22 months ago by gcc

  • Easy pickings unset
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • UI/UX unset

Well, that doesn't actually fix the problem, it just documents it.

In https://github.com/django/django/blob/master/django/contrib/admin/templatetags/admin_list.py#L109, result_headers yields a text block without a class_attrib value. But the template expects it to be present. We could just have it send an empty one.

comment:3 Changed 22 months ago by lukeplant

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

We basically gave up attempting to fix this bug. Because no-one ever uses non-default values of TEMPLATE_STRING_IF_INVALID in production, no-one ever catches bugs caused by non-default values, so the bug always comes back, in the same place or a different place. Attempting to fix it will just waste more time.

comment:4 Changed 16 months ago by CollinAnderson

  • Cc cmawebsite@… added

I would be interested in revisiting the policy on TEMPLATE_STRING_IF_INVALID. This is an optional setting that can make the template language much more strict. It has shown me lots of template bugs on my websites that I didn't realize existed, and I trust it enough to even use it in production, despite the recommendation. I would recommend to people to use it more, if only the django admin didn't rely on silent invalid variable errors.

We could use a non-default value in our admin tests to be sure these types of errors do not crop up again.

Is it worth fixing the admin? Yes. As far as I can tell, the admin is actually very close to being 100% free of relying on invalid variables. Our original policy on TEMPLATE_STRING_IF_INVALID was put in place back in the old-admin days, but things are better now. As far as I've seen on live websites it's really just this header.class_attrib problem. I have USE_I18N = False set, so the LANGUAGE_CODE variable is also invalid. On a live production website, those are the only two invalid variables I have run into.

It's easy to fix these errors, just wrap them in {% if header.class_attrib %}.

Here's what I use as my "string" in my settings. Note the workaround for this ticket:

class InvalidVar(object):
    def __mod__(self, s):
        assert s.var in ('LANGUAGE_CODE', 'header.class_attrib'), s.var
        return ''
    def __contains__(self, x):
        return x == '%s'
TEMPLATE_STRING_IF_INVALID = InvalidVar()
Last edited 16 months ago by CollinAnderson (previous) (diff)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.