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

[JK Laiho]

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.

2. 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.

[coleifer]

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

[coleifer]

patched with tests.

[Chris Beaven]

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.

[Simon Meers]

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

[Daniel F Moisset]

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.

[Daniel F Moisset]

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

[Cory Walker]

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.

[Karen Tracey]

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?

[Russell Keith-Magee]

(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.

[Torsten Bronger]

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.

[Karen Tracey]

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>

[Torsten Bronger]

This new "url_backwards_re" can hang. Try this:

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

[Karen Tracey]

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

[Jacob]

Milestone 1.2 deleted