Code

Opened 7 years ago

Closed 3 years ago

#3579 closed (invalid)

remove admin's reliance on TEMPLATE_STRING_IF_INVALID set to empty string

Reported by: Jeff Bauer <jbauer@…> Owned by: nobody
Component: Template system Version: master
Severity: Keywords: template admin
Cc: hv@…, marinho Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Refactor the way TEMPLATE_STRING_IF_INVALID works, rather than special-case it for the admin site.

It's often useful to have something like "BADDATA" assigned to TEMPLATE_STRING_IF_INVALID during development and testing, because it's easy to to scan for unforseen side effects, both visually and by a text search.

Attachments (5)

3579.patch.txt (1.0 KB) - added by buriy <burchik@…> 7 years ago.
First piece
all.patch (3.6 KB) - added by buriy <burchik@…> 7 years ago.
All fixes: two for action="{{ form_url }}", two for <th{{some.stuff}}>
all.2.patch (3.6 KB) - added by buriy <burchik@…> 7 years ago.
All fixes: two for action="{{ form_url }}", two for <th{{some.stuff}}>
all.3.patch (4.0 KB) - added by buriy <burchik@…> 7 years ago.
All fixes: two for action="{{ form_url }}", two for <th{{some.stuff}}>, one for <td{{some.stuff}}
3579_4.patch (2.5 KB) - added by buriy 6 years ago.
Added template conditionals for sometimes-non-existing variables in admin, r8445.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

Not sure what you mean by special casing for the admin, could you please explain in more detail.

comment:2 Changed 7 years ago by Jeff Bauer <jbauer@…>

  • Resolution invalid deleted
  • Status changed from closed to reopened

Sorry, I was referring to this thread: http://tinyurl.com/2nxu98

A better way to phrase the issue?

Refactor the way TEMPLATE_STRING_IF_INVALID works, without special-casing it for the admin site.

comment:3 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Triage Stage changed from Unreviewed to Design decision needed

Could you please point out where the special-casing exists in the admin.

comment:4 Changed 7 years ago by Jeff Bauer <jbauer@…>

http://www.djangoproject.com/documentation/templates_python/#how-invalid-variables-are-handled

"Many templates, including those in the Admin site, rely upon the silence of the template system when a non-existent variable is encountered."

comment:5 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Component changed from Core framework to Template system
  • Triage Stage changed from Design decision needed to Accepted

Ok, this was just pointed out to me. Feel free to provide suggestions and/or patches. Marking accepted as per Adrian's comments in the mailing list post.

comment:6 Changed 7 years ago by Gary Wilson <gary.wilson@…>

comment:7 Changed 7 years ago by Gary Wilson <gary.wilson@…>

  • Summary changed from decouple TEMPLATE_STRING_IF_INVALID to remove admin's reliance on TEMPLATE_STRING_IF_INVALID set to empty string

Changed 7 years ago by buriy <burchik@…>

First piece

Changed 7 years ago by buriy <burchik@…>

All fixes: two for action="{{ form_url }}", two for <th{{some.stuff}}>

Changed 7 years ago by buriy <burchik@…>

All fixes: two for action="{{ form_url }}", two for <th{{some.stuff}}>

Changed 7 years ago by buriy <burchik@…>

All fixes: two for action="{{ form_url }}", two for <th{{some.stuff}}>, one for <td{{some.stuff}}

comment:8 Changed 7 years ago by buriy <burchik@…>

That's all. All code was looked for similar patterns.

comment:9 Changed 7 years ago by buriy <burchik@…>

related but not closed tickets:

http://code.djangoproject.com/ticket/3073 [hack for output, giving empty form_url]
http://code.djangoproject.com/ticket/2583 [no patch, only long bug description]

comment:10 Changed 7 years ago by buriy <burchik@…>

  • Has patch set

committers, anyone? week passed, no reaction.

comment:11 Changed 7 years ago by mtredinnick

Please demonstrate some patience. You may have noticed that this isn't the only ticket in Trac and it isn't the highest priority item at the moment. When we have time, it will be reviews and you'll get some feedback on what remains to be done.

comment:12 Changed 7 years ago by ubernostrum

#5532 was a duplicate.

comment:13 Changed 6 years ago by guettli

  • Cc hv@… added

comment:14 Changed 6 years ago by marinho

  • Cc marinho added

Changed 6 years ago by buriy

Added template conditionals for sometimes-non-existing variables in admin, r8445.

comment:15 Changed 6 years ago by buriy

  • milestone set to 1.0 maybe

comment:16 Changed 6 years ago by mtredinnick

  • milestone changed from 1.0 maybe to post-1.0

comment:17 follow-up: Changed 6 years ago by marinho

  • milestone changed from post-1.0 to 1.0 maybe

I'm not understanding why this ticket is being considered to post-1.0 if it is (I think) the cause to bugged add-user form and others strange behaviours everytime a variable isn't valid in the template.

For example: a simple /admin/auth/user/add/ is rendering this code:

<form action="&lt;font color=&#39;red&#39;&gt;form_url&lt;/font&gt;" method="post" id="user_form">
<div>

Shouldn't this enought reason to consider this ticket?

Maybe would make it safe string or disabling this "helper" (that isn't helping nothing).

comment:18 Changed 6 years ago by mtredinnick

  • milestone changed from 1.0 maybe to post-1.0

Making a change like this now (in feature freeze) would be destabilising, at a minimum.

Seriously, when a core maintainer triages something as post-1.0, please don't decide that you should change that. we have a mailing list if you want to ask for a reconsideration, but this isn't even borderline. It's an enhancement request that can easily be added afterwards.

comment:19 in reply to: ↑ 17 Changed 6 years ago by buriy

Replying to marinho:

For example: a simple /admin/auth/user/add/ is rendering this code:

<form action="&lt;font color=&#39;red&#39;&gt;form_url&lt;/font&gt;" method="post" id="user_form">
<div>

When TEMPLATE_STRING_IF_INVALID is empty (by default), there's no such problem.
Also it is said in the docs that

While TEMPLATE_STRING_IF_INVALID can be a useful debugging tool,
it is a bad idea to turn it on as a ‘development default’.

Many templates, including those in the Admin site, rely upon the silence
of the template system when a non-existent variable is encountered.
If you assign a value other than '' to TEMPLATE_STRING_IF_INVALID, you
will experience rendering problems with these templates and sites.

at http://www.djangoproject.com/documentation/templates_python/#how-invalid-variables-are-handled

Admin interface templates are relying on this behavior now in few last places. Patch for them is attached.

comment:20 Changed 6 years ago by marinho

Thank you Yuri.

Sorry, Malcolm, you both are right :)

comment:21 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:22 Changed 3 years ago by lukeplant

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

I think it is now too late to put this genie back in the bottle, and it isn't worth the effort trying to fix the admin or other contrib apps. Given that #3073 was marked as invalid I'm going to do the same here. Please bring to django-devs if you think it is worth the effort to fix this.

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.