Opened 3 years ago
Closed 3 years ago
#21077 closed Bug (wontfix)
simple_tag does not allow kwargs with dashes in them
Description
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:
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.
Change History (6)
comment:1 Changed 3 years ago by
comment:2 Changed 3 years ago by
comment:3 Changed 3 years ago by
comment:4 Changed 3 years ago by
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".
comment:5 Changed 3 years ago by
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.
comment:6 Changed 3 years ago by
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.
One way or another, this looks like a bug.
On the one hand, the permissiveness of
kwarg_reinvites 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_tagfunctions as plain dictionaries, instead of unpacking them into function parameters. But given what we have now, I can think of two ways to proceed:
simple_tagtags. 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.
**kwargsin 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
**kwargsstyle 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.