Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#12945 closed (fixed)

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

Reported by: jklaiho Owned by: nobody
Component: Template system Version: master
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: UI/UX:

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 5 years ago.
12945.2.diff (9.0 KB) - added by SmileyChris 5 years ago.
12945.2.2.diff (9.3 KB) - added by SmileyChris 5 years ago.
12945.docfixes.diff (2.7 KB) - added by dmoisset 5 years ago.
Documentation fixes to acknowledge spaces the official separators in {% url %}

Download all attachments as: .zip

Change History (26)

comment:1 Changed 5 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by 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

comment:3 Changed 5 years ago by coleifer

  • Has patch set
  • Owner changed from nobody to coleifer
  • Status changed from new to assigned

patched with tests.

Changed 5 years ago by coleifer

comment:4 Changed 5 years ago by SmileyChris

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.

Changed 5 years ago by SmileyChris

Changed 5 years ago by SmileyChris

comment:5 Changed 5 years ago by DrMeers

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

comment:6 Changed 5 years ago by dmoisset

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

Changed 5 years ago by dmoisset

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

comment:7 Changed 5 years ago by ramusus

  • Cc ramusus@… added

comment:8 Changed 5 years ago by cmwslw

  • Cc cwalker32@… added

comment:9 Changed 5 years ago by anonymous

  • Cc alex@… added

comment:10 Changed 5 years ago by cmwslw

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 Changed 5 years ago by kmtracey

  • Owner changed from coleifer to kmtracey
  • Status changed from assigned to new

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 Changed 5 years ago by kmtracey

  • Owner changed from kmtracey to nobody

comment:13 Changed 5 years ago by tolano

  • Cc aribao@… added
  • Owner changed from nobody to tolano
  • Status changed from new to assigned

comment:14 Changed 5 years ago by tolano

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

comment:15 Changed 5 years ago by anonymous

  • Cc hvdklauw@… added

comment:16 Changed 5 years ago by russellm

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

(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 follow-up: Changed 5 years ago by 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.

comment:18 Changed 5 years ago by bronger

  • Cc bronger@… added

comment:19 in reply to: ↑ 17 Changed 5 years ago by kmtracey

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 Changed 5 years ago by bronger

This new "url_backwards_re" can hang. Try this:

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

comment:21 Changed 5 years ago by kmtracey

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

comment:22 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 deleted

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