simple_tag does not allow kwargs with dashes in them

[Ilya Semenov]

Consider a simple tag which is supposed to emit HTML code:

from django.template import Library
from django.utils.html import escape
register = Library()

@register.simple_tag()
def generate_html_tag(element, **kwargs):
        attrs = ''.join(' %s="%s"' % (k,escape(v)) for k,v in kwargs.items())
        return "<%s%s/>" % (element, attrs)

and this usage:

{% generate_html_tag "img" src=image.url %}

This works pretty well until we decide to add a data-xxx attribute:

{% generate_html_tag "img" src=image.thumbnail.url data-original-src=image.url %}

which makes the template engine crash with the exception:

Traceback:
File "/venv/lib/python2.7/site-packages/django/core/handlers/base.py" in get_response
  115.                         response = callback(request, *callback_args, **callback_kwargs)
File "/Users/semenov/work/onor/src/onor/views.py" in index
  4. 	return render(request, 'index.html')
File "/venv/lib/python2.7/site-packages/django/shortcuts/__init__.py" in render
  53.     return HttpResponse(loader.render_to_string(*args, **kwargs),
File "/venv/lib/python2.7/site-packages/django/template/loader.py" in render_to_string
  170.         t = get_template(template_name)
File "/venv/lib/python2.7/site-packages/django/template/loader.py" in get_template
  146.     template, origin = find_template(template_name)
File "/venv/lib/python2.7/site-packages/django/template/loader.py" in find_template
  135.             source, display_name = loader(name, dirs)
File "/venv/lib/python2.7/site-packages/django/template/loader.py" in __call__
  43.         return self.load_template(template_name, template_dirs)
File "/venv/lib/python2.7/site-packages/django/template/loader.py" in load_template
  49.             template = get_template_from_string(source, origin, template_name)
File "/venv/lib/python2.7/site-packages/django/template/loader.py" in get_template_from_string
  157.     return Template(source, origin, name)
File "/venv/lib/python2.7/site-packages/django/template/base.py" in __init__
  125.         self.nodelist = compile_string(template_string, origin)
File "/venv/lib/python2.7/site-packages/django/template/base.py" in compile_string
  153.     return parser.parse()
File "/venv/lib/python2.7/site-packages/django/template/base.py" in parse
  274.                     compiled_result = compile_func(self, token)
File "/venv/lib/python2.7/site-packages/django/template/loader_tags.py" in do_extends
  215.     nodelist = parser.parse()
File "/venv/lib/python2.7/site-packages/django/template/base.py" in parse
  274.                     compiled_result = compile_func(self, token)
File "/venv/lib/python2.7/site-packages/django/template/loader_tags.py" in do_block
  190.     nodelist = parser.parse(('endblock',))
File "/venv/lib/python2.7/site-packages/django/template/base.py" in parse
  274.                     compiled_result = compile_func(self, token)
File "/venv/lib/python2.7/site-packages/django/template/base.py" in generic_tag_compiler
  1015.                               defaults, takes_context, name)
File "/venv/lib/python2.7/site-packages/django/template/base.py" in parse_bits
  985.                     "keyword argument(s)" % name)

Exception Type: TemplateSyntaxError at /
Exception Value: 'generate_html_tag' received some positional argument(s) after some keyword argument(s)

This is because django.template.base.kwarg_re is quite restrictive and doesn't allow dashes in kwargs names:

kwarg_re = re.compile(r"(?:(\w+)=)?(.+)")

This is a real life use case and we have to apply monkey patches to overcome the problem. Moreover, other projects reuse Django's token_kwargs/parse_bits logic and then fail in the same way: ​https://github.com/matthewwithanm/django-imagekit/pull/253

Obviously, this is easily fixable by adjusting the regular expression:

kwarg_re = re.compile(r"(?:([\w+-])=)?(.+)")

I'm not sure if the tag arguments syntax was restricted this way on purpose or not. You see, it would at least make sense if it repeated Python's own syntax for keyword arguments (something like [a-z_]\w+), but currently as it's simply \w+ it allows keyword arguments like 2=2 (but doesn't allow foo-bar=1) so in my opinion it's overly restrictive in one sense and insufficiently restrictive in other sense.

[Kevin Christopher Henry]

One way or another, this looks like a bug.

On the one hand, the permissiveness of kwarg_re invites invalid function signatures. {% my_tag 9_lives=True %} is valid template code, but def my_tag(9_lives=False) is invalid Python. On the other hand, there's no particular reason why named template arguments should have to be valid Python identifiers, and semenov's use case seems reasonable to me.

In retrospect, I think it might have been better to pass named arguments to simple_tag functions as plain dictionaries, instead of unpacking them into function parameters. But given what we have now, I can think of two ways to proceed:

1. Only allow valid Python identifiers to be named arguments in simple_tag tags. If users want something else they'll have to forgo using simple_tag. This isn't backwards compatible, though, and is more restrictive than we might like.

2. Allow named arguments to be invalid Python identifiers, acknowledging that template tag authors can simply use **kwargs in their function signatures. In this case, I agree with semenov that we should be more permissive in what we accept; in particular, I don't see any reason to outlaw dashes. This is backwards compatible and more functional, though it seems hackish that only the **kwargs style of function signature can be used with named arguments that aren't Python identifiers.

It looks like this same regular expression is used for other tags with keyword arguments (with, include, blocktrans, url) so it's probably worth thinking about any implications for those cases as well.

[Christopher Babiak]

I think it's possible to create a custom template tag where you could allow dashes rather than using simple_tag(), which seems like a better solution to me than changing simple_tag(). Even if not, though, introducing syntax (dashes) that could be confused as trying to do subtraction within tags or variables is not a good idea. True, {% my_tag 9_lives=True %} as valid template code is not optimal and not pythonic, but that is not a good argument to make 9-lives (which is worse because it could be misconstrued as attempting to subtract lives from 9) acceptable.

{{ foo-bar }} is bad template variable syntax, and it *should* break if you tried to name a variable foo-bar. Likewise, {% foo-bar %} is bad template tag syntax. There are also further-reaching implications to changing kwarg_re than just simple_tag(). As marfire notes, it is used by many of the default tags: with, include, blocktrans, and url. Creating a work-around just so the kwarg_re that applies to simple_tag() allows dashes does not seem like a good idea, especially if having dashes can be achieved through a custom template tag... and if it should be avoided in the first place, anyway.

I think this ticket should be changed to "won't fix".

[Kevin Christopher Henry]

The template language doesn't appear to have been designed to be as restrictive as you are calling for. For example, {% foo-bar %} is a perfectly valid template tag name. {{ my_var|my-filter }} and {% my_tag my-arg %} are also valid.

And {% my_tag my-kwarg=5 %} is valid if the template tag author doesn't use simple_tag(). The question is why it shouldn't also be possible to use simple_tag() in this case. The original reason, presumably, was to ensure that keyword arguments could be listed in the function signature (as noted, though, kwarg_re fails to ensure that).

To me it seems better to treat these keyword arguments like other strings in the template language (allowing -, etc.); in addition to being more consistent, this would improve the usefulness of simple_tag(), as in semenov's use case. This solution also appears to be backwards compatible.

[Russell Keith-Magee]

I'm inclined to agree with @cbabs on this one. I can't see any compelling reason why a dash is helpful as a token in kwarg on a template tag. The fact that they're legal for the name of the template tag itself strikes me as an accident of parsing history, not strong design. Introducing dashes into kwarg syntax seems to me like something that would be syntactically ambiguous, and would open up a treasure trove if annoying edge case bugs.

Marking won't fix; if anyone is particularly driven to argue the counter position, please open a thread on Django-dev.