Code

Opened 7 years ago

Closed 17 months ago

#6271 closed New feature (fixed)

filter arguments with spaces break several template tags

Reported by: Rob Hudson <treborhudson@…> Owned by: aaugustin
Component: Template system Version: master
Severity: Normal Keywords: tplrf-fixed
Cc: serg@…, bmispelon@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by gwilson)

After [6956], something like this now breaks:

{% regroup object_list by created|date:"F Y" as grouped %}

I believe it now splits the quoted bits and the arg count is more than 6 in this case, so the templatetag errors out.

Attachments (4)

regroup_regression_test.diff (2.2 KB) - added by Rob Hudson <treborhudson@…> 7 years ago.
A test case for regroup with filters containing a space
6271.diff (9.5 KB) - added by gwilson 7 years ago.
smart_split.diff (2.5 KB) - added by SmileyChris 7 years ago.
A rewrite of smart_split generator to properly handle quoted phrases like we need
smart_split.2.diff (5.6 KB) - added by gwilson 7 years ago.

Download all attachments as: .zip

Change History (19)

Changed 7 years ago by Rob Hudson <treborhudson@…>

A test case for regroup with filters containing a space

comment:1 Changed 7 years ago by gwilson

  • Description modified (diff)
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement set
  • Summary changed from Changeset 6956 broke regroup tag if filter contains a space to filter arguments with spaces break several template tags
  • Triage Stage changed from Unreviewed to Accepted

The bigger problem here is that many of the template tags do not accept filtered arguments passed an argument with a space because they are simply doing token.contents.split().

Here is a patch that seems to fix things for regroup, ifchanged, and with. firstof, if, and ifequal/ifnotequal need some more (involved) work. I changed smart_split_re to allow text before and after the quote characters. Not sure if we want smart_split() to basically only be for parsing template tags, but it seems to be the only place it's used currently.

Changed 7 years ago by gwilson

Changed 7 years ago by SmileyChris

A rewrite of smart_split generator to properly handle quoted phrases like we need

comment:2 Changed 7 years ago by SmileyChris

I had been thinking about this recently too.

Just allowing text before and after quote characters still isn't good enough. If multiple filters are used which have arguments with spaces then it'll still fall over. Here's my rewrite of the smart_split method to handle it like we need.

comment:3 Changed 7 years ago by adrian

Note that I reverted [6956] in [6996].

comment:4 Changed 7 years ago by gwilson

  • Owner changed from nobody to gwilson
  • Status changed from new to assigned

comment:5 Changed 7 years ago by gwilson

  • Owner gwilson deleted
  • Status changed from assigned to new

So there seems to be a problem here in that smart_split is replacing escaped quotes and escaped backslashes with the quote and backslash respectively. The bits from the split are sometimes passed to django.template.compile_filter, which creates a FilterExpression object, which again looks for escaped quotes in filter arguments, and causes a parse error.

I'm thinking that we make smart_split just split and not replace any escaped quotes or backslashes. Then we make sure compile_filter is being used everywhere it needs to be used. Let compile filter do the processing of escaped quotes, translation strings, multiple filters, arguments with spaces, etc.

Anyway, here is a patch for smart_split that takes SmileyChris's regex changes, adds the smart_split tests from my first patch (plus a few more), and removes the unescaping of quotes and backslashes in smart_split...

Changed 7 years ago by gwilson

comment:6 Changed 6 years ago by anonymous

  • Cc serg@… added

comment:7 Changed 6 years ago by emulbreh

  • Keywords tplrf-fixed added

This would be fixed by the refactoring proposed in #7806.

comment:8 Changed 6 years ago by dc

#8902 has been marked as a duplicate of this ticket. It also contains small patch.

comment:9 Changed 6 years ago by mtredinnick

For this ticket to get to the point of being resolvable, it has to actually fix the template tags that have problems with spaces in their arguments. Just changing smart_split() is only part of the issue (since it's actually a change that doesn't fix the original problem, as the original problem isn't in the code any longer). We also need to fix the problem identified in #8902: those tags not using smart_split(), as well as add documentation.

So the currently latest patch is only the first 25.647% (approximately) of the fix. Yes, this may be looked at as an increase in scope of the ticket, but we need one ticket that is the holder for "spaces in arguments" and this one was just nominated to volunteer.

comment:10 Changed 6 years ago by dc

#10001 has been marked as a duplicate of this ticket. It also contains small patch for smart_split().

comment:11 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to New feature

Considering how old this "bug" is, fixing it properly today would be equivalent to a new feature, I think.

comment:12 Changed 2 years ago by aaugustin

  • Easy pickings unset
  • Owner set to aaugustin
  • UI/UX unset

comment:13 Changed 17 months ago by bmispelon

I "accidentally" fixed this while working on a different bug.

I have a pull request for this and #18260 at ​https://github.com/django/django/pull/751

comment:14 Changed 17 months ago by bmispelon

  • Cc bmispelon@… added

comment:15 Changed 17 months ago by Baptiste Mispelon <bmispelon@…>

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

In 069280a6893d0f496c8f15922807939c68459ec3:

Used token.split_contents() for tokenisation in template tags accepting variables.

Fixed #6271, #18260.

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.