Opened 14 years ago

Closed 14 years ago

Last modified 12 years ago

#12945 closed (fixed)

url tag breaks on spaces between delimiting commas since [12503]

Reported by: JK Laiho Owned by: nobody
Component: Template system Version: dev
Severity: Keywords: url tag pycamp2010
Cc: ramusus@…, cwalker32@…, alex@…, aribao@…, hvdklauw@…, bronger@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The parsing of the arguments to the {% url %} tag was changed in [12503] to account for commas etc. appearing in literal strings, fixing #12072.

Unfortunately, the same changeset broke the use of spaces between the commas that delimit arguments, producing a TemplateSyntaxError: Malformed arguments to url tag.

Admittedly, spaces in the url tag have never been "officially sanctioned", given that the documentation example for the url tag looks like this:

{% url path.to.some_view arg1,arg2,name1=value1 %}

But in a lot of real-world code (including all of our projects), it's common to use spaces between the commas, because it looks cleaner and is easier to read:

{% url path.to.some_view arg1, arg2, name1=value1 %}

I'd wager that the previously working form with spaces is common enough out in the wild for this change to bite a lot of people pretty hard. While the documentation example doesn't use spaces, it doesn't explicitly state that they are forbidden, either, and typing them comes naturally given the tendency to separate commas with spaces in Python code.

I tested and found that downgrading Django to r12502 allowed for spaces between commas again.

So, the possibilities:

  1. Stick with the change, forcing everybody to remove any spaces they may have in their url tags, but document the change as backwards-incompatible. Given the non-specific error message, this will cause a lot of head-scratching, especially since looking at the deeply nested code in URLNode (and the hairy regex preceding it) doesn't make the problem immediately clear. This can be alleviated by documenting it as backwards-incompatible, of course.
  1. Make the regex more permissive, allowing at least a single space after each comma.

IMHO option 2 would be the least painful way forward. I don't believe there's any convincing reason to be strict about this, given the downsides in the real world.

Attachments (4)

12945.diff (4.2 KB ) - added by coleifer 14 years ago.
12945.2.diff (9.0 KB ) - added by Chris Beaven 14 years ago.
12945.2.2.diff (9.3 KB ) - added by Chris Beaven 14 years ago.
12945.docfixes.diff (2.7 KB ) - added by Daniel F Moisset 14 years ago.
Documentation fixes to acknowledge spaces the official separators in {% url %}

Download all attachments as: .zip

Change History (26)

comment:1 by Alex Gaynor, 14 years ago

Triage Stage: UnreviewedAccepted

comment:2 by coleifer, 14 years ago

reverse() does not handle both args & kwargs - just one or the other. I've submitted a ticket: http://code.djangoproject.com/ticket/12946

This issue is of course still valid, just wanted to give a heads up

comment:3 by coleifer, 14 years ago

Has patch: set
Owner: changed from nobody to coleifer
Status: newassigned

patched with tests.

by coleifer, 14 years ago

Attachment: 12945.diff added

comment:4 by Chris Beaven, 14 years ago

Technically I'm proposing my patch as an alternative to that committed in [12503], but fixing this issue too.

I'd rather not see band-aid patches for the current {% url %} tag's comma separated argument format, when no other template tags use commas. My suggestion is that we use spaces like everywhere else but keep backwards compatibility just like the {% cycle %} tag did. This is what my patch does.

by Chris Beaven, 14 years ago

Attachment: 12945.2.diff added

by Chris Beaven, 14 years ago

Attachment: 12945.2.2.diff added

comment:5 by Simon Meers, 14 years ago

Note that some built-in templates such as registration/password_reset_email.html are affected by this, and currently broken.

comment:6 by Daniel F Moisset, 14 years ago

Keywords: pycamp2010 added

If the "official" way to use arguments in the url tags is with spaces, documentation needs to be updated. The following patch:

  • Fixes the template mentioned by DrMeers (I checked the rest of the template and this one seems to be the only one with commas)
  • Fixes the docstring of the url tag to use spaces in the example
  • Updates the documentation to use spaces, and note that the old syntax is supported but deprecated.

by Daniel F Moisset, 14 years ago

Attachment: 12945.docfixes.diff added

Documentation fixes to acknowledge spaces the official separators in {% url %}

comment:7 by ramusus, 14 years ago

Cc: ramusus@… added

comment:8 by Cory Walker, 14 years ago

Cc: cwalker32@… added

comment:9 by anonymous, 14 years ago

Cc: alex@… added

comment:10 by Cory Walker, 14 years ago

Any reason why there hasn't been any activity on this in a while? Why not just add spaces in the password reset email template for now, so it is not completely broken. Then the developers can decide on whether or not spaces should be required. This is the only issue keeping me from updating my SVN past r12502.

comment:11 by Karen Tracey, 14 years ago

Owner: changed from coleifer to Karen Tracey
Status: assignednew

Removing the use of commas in the doc and shipped templates, so making spaces the official "right" way, yet still allowing for the old comma (with optional space) syntax, sounds good to me. Any objections?

comment:12 by Karen Tracey, 14 years ago

Owner: changed from Karen Tracey to nobody

comment:13 by Adrian Ribao, 14 years ago

Cc: aribao@… added
Owner: changed from nobody to Adrian Ribao
Status: newassigned

comment:14 by Adrian Ribao, 14 years ago

Owner: changed from Adrian Ribao to nobody
Status: assignednew

comment:15 by anonymous, 14 years ago

Cc: hvdklauw@… added

comment:16 by Russell Keith-Magee, 14 years ago

Resolution: fixed
Status: newclosed

(In [12889]) Fixed #12945 -- Corrected the parsing of arguments in {% url %} when the argument list has spaces between commas. This is a revised version of r12503, which was a fix for #12072. Thanks to SmileyChris for the patch, and to dmoisset for finding all the places in the docs that the old style syntax was used.

comment:17 by Torsten Bronger, 14 years ago

But now, something is broken.

{% url refdb.views.reference.edit citation_key=reference.citation_key database=database %}

works, but with

{% url refdb.views.reference.edit citation_key=reference.citation_key, database=database %}

and

{% url refdb.views.reference.edit citation_key=reference.citation_key,database=database %}

the testserver hangs indefninitely.

comment:18 by Torsten Bronger, 14 years ago

Cc: bronger@… added

in reply to:  17 comment:19 by Karen Tracey, 14 years ago

Replying to bronger:

But now, something is broken.

I cannot recreate anything like what you describe, using trunk r12921. For a view:

def twoargs(request, pk, arg):
    rsp_txt = 'request with pk=%s, arg=%s processed' % (pk, arg)
    return HttpResponse(rsp_txt)

included in urls.py:

    (r'^twoargs/(?P<pk>\d+)/(?P<arg>\w+)/$', 'ttt.views.twoargs'),

all of these variations in a template produce working correct links:

<a href="{% url ttt.views.twoargs pk=info.pk arg=info.arg %}">Link 1</a>
<a href="{% url ttt.views.twoargs pk=info.pk,arg=info.arg %}">Link 2</a>
<a href="{% url ttt.views.twoargs pk=info.pk, arg=info.arg %}">Link 3</a>
<a href="{% url ttt.views.twoargs info.pk  info.arg %}">Link 4</a>
<a href="{% url ttt.views.twoargs info.pk,info.arg %}">Link 5</a>
<a href="{% url ttt.views.twoargs info.pk, info.arg %}">Link 6</a>

comment:20 by Torsten Bronger, 14 years ago

This new "url_backwards_re" can hang. Try this:

import re
re.match(r'''(('[^']*'|"[^"]*"|[^,]+)=?)+$''', "citation_key=reference.citation_key,database=database")

comment:21 by Karen Tracey, 14 years ago

I opened #13275 to look into this problem with url_backwards_re.

comment:22 by Jacob, 12 years ago

milestone: 1.2

Milestone 1.2 deleted

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