Opened 10 years ago

Closed 5 years ago

Last modified 5 years ago

#3100 closed New feature (fixed)

Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %})

Reported by: Eric Van Dewoestine <ervandew@…> Owned by: Aymeric Augustin
Component: Template system Version: master
Severity: Normal Keywords:
Cc: Daniel Ring, mmitar@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Patch based on development version 4051.

Currently the template functionality prevents creation of tags which have intermediate tags requiring arguments.

For instance:

{% myif arg %}
  first block
{% elif arg %}  <-- current version would fail to parse this correctly because of the arg
  second block
{% endmyif %}

The attached patch removes this restriction.

Attachments (7)

__init__.py.diff (992 bytes) - added by Eric Van Dewoestine <ervandew@…> 10 years ago.
patch-breaking-namedendblocks-in-regressiontests-templates.diff (1.1 KB) - added by Daniel Ring 7 years ago.
patch breaks regressiontests.templates 'namedendblocks0[234]'
django_elif_docs.diff (817 bytes) - added by cwebber 7 years ago.
Docs for the new {% elif %} tag
django_elif.diff (8.8 KB) - added by cwebber 7 years ago.
Django {% elif %} tag support (now with docs in the same patch)
fix3100.patch (9.0 KB) - added by Matthias Kestenholz 6 years ago.
3100-alternate.diff (1.3 KB) - added by Chris Beaven 5 years ago.
An alternative to adding a parse_until_args_allowed argument
3100-backwards-compatible.patch (2.1 KB) - added by Aymeric Augustin 5 years ago.

Download all attachments as: .zip

Change History (29)

Changed 10 years ago by Eric Van Dewoestine <ervandew@…>

Attachment: __init__.py.diff added

comment:1 Changed 10 years ago by Eric Van Dewoestine <ervandew@…>

Summary: Patch: Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %})[patch]: Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %})

comment:2 Changed 10 years ago by Eric Van Dewoestine <ervandew@…>

note patch is for file django/template/init.py.diff

comment:3 Changed 10 years ago by Eric Van Dewoestine <ervandew@…>

Resolution: duplicate
Status: newclosed

Just stumbled on http://code.djangoproject.com/ticket/3090 ... not sure which one is more or less correct, but as long as the referenced example in each is supported, I'm happy.

comment:4 Changed 7 years ago by Russell Keith-Magee

Needs tests: set
Patch needs improvement: set
Resolution: duplicate
Status: closedreopened
Summary: [patch]: Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %})Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %})

This may be a duplicate of #3090, but this ticket describes the problem much better. Reopening.

comment:5 Changed 7 years ago by Julien Phalip

See also #8296 for a similar issue but formulated differently.

comment:6 Changed 7 years ago by Alex Gaynor

Triage Stage: UnreviewedAccepted

Changed 7 years ago by Daniel Ring

patch breaks regressiontests.templates 'namedendblocks0[234]'

comment:7 Changed 7 years ago by Daniel Ring

Cc: Daniel Ring added

The patch just added breaks the regressiontests.templates 'namedendblocks0?' by failing to raise TemplateSyntaxError when {% endblock %} tags are out of order. (Since endblock tags are the only (core) tags where the endtag has an argument.)

So two possibilities are:

  1. remove those tests - seems okay to me, since the named-endblock tags are for readability anyway, rather than providing functionality; also, named endblocks are only used (at least in trunk) in contrib.gis javascripts.
  2. make additional changes in template.loader_tags.do_block to force strict checking of endblock tags there

Which would be the desired route? Or something altogether different?

comment:8 Changed 7 years ago by Adrian Holovaty

I'm fine with removing those named-end-block tests. I agree that named endblock tags are just for readability, anyway.

comment:9 Changed 7 years ago by cwebber

I removed those named-end-block tests and also added {% elif %} tag support (yay, {% elif %} tags is a major feature!)

comment:10 Changed 7 years ago by cwebber

Needs tests: unset
Patch needs improvement: unset

Changed 7 years ago by cwebber

Attachment: django_elif_docs.diff added

Docs for the new {% elif %} tag

comment:11 Changed 7 years ago by cwebber

Triage Stage: AcceptedReady for checkin

Added docs for the {% elif %} tag.

Changed 7 years ago by cwebber

Attachment: django_elif.diff added

Django {% elif %} tag support (now with docs in the same patch)

comment:12 Changed 6 years ago by Mitar

Cc: mmitar@… added

comment:13 Changed 6 years ago by Malcolm Tredinnick

Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Hmm... so this is two different things in one patch: (1) is allowing args on the intermediate tag, (2) changing if tag to include elif. We might want (1) without (2), hence our usual insistence on one feature per ticket.

That being said, the patch looks reasonable. However, the change to the tests to remove the unbalanced blocks test is bad. You are removing tests that check for something bad happening. If this patch doesn't let those tests still fail, the patch is broken. If it doesn't affect those tests, they should remain. Either way, this isn't ready for checkin yet. I'll add "needs tests" to the flags for this reason, in case somebody is looking for test related work. It doesn't need new tests, just those deleted "extra endblock" tests restored, please.

comment:14 Changed 6 years ago by Matthias Kestenholz

Needs documentation: set
Needs tests: unset

A differentiation between intermediate tags which accept arguments and tags who don't seems to be necessary to support this use case properly (without breaking backwards-compatibility or deleting tests).

The attached patch adds another argument to django.template.Parser.parse; parse_until is complemented by parse_until_args_allowed. The included if-tag supporting {% elif %} makes use of this. The previously deleted "extra endblock" tests have been restored and they pass, as they should.

The documentation about writing custom tags would have to be extended for this, though.

Changed 6 years ago by Matthias Kestenholz

Attachment: fix3100.patch added

comment:15 Changed 6 years ago by Łukasz Rekucki

Severity: normalNormal
Type: enhancementNew feature

Changed 5 years ago by Chris Beaven

Attachment: 3100-alternate.diff added

An alternative to adding a parse_until_args_allowed argument

comment:16 Changed 5 years ago by Cal Leeming

Easy pickings: unset
UI/UX: unset

Guys - any idea when this might make it into a release??

Cal

comment:17 Changed 5 years ago by Aymeric Augustin

#17348 was created to discuss the addition of {% elif %} — as pointed out by Malcolm these are two different (however related) features.

comment:18 Changed 5 years ago by Aymeric Augustin

Owner: changed from Adrian Holovaty to Aymeric Augustin
Status: reopenednew

comment:19 Changed 5 years ago by Aymeric Augustin

Here are some implementation details of the Parser class that I had to look up in the source in order to understand this ticket.

An instance of Parser holds a list of tokens — the output of Lexer.tokenize — in its tokens instance variable. Its parse method builds a list of nodes (nodelist) by repeatedly popping the first token, compiling it, and appending to nodelist appropriate nodes.

parse terminates:

  • when it reaches the end of the token stream;
  • when it receives a block token whose content is found in parse_until (when it was called with a parse_until argument).

parse_until is used by tags that use final (or intermediary) tags, like {% block ... %} {% endblock %}, or {% for ..%} {% empty %} {% endfor %}. However, the current implementation requires that the exact content of the final (or intermediary) tag is known by the time the initial tag is compiled. This makes it impossible to have variable arguments on the intermediary tag.

This feature is used to reject unmatched block/endblock tags: {% block foo %}{% endblock bar %}. I think this check should be delegated to the block tag itself. The alternative is to use Chris' technique — looking for an exact match by default, and for a prefix match for elements that end with a space — but it feels hackish.

Version 0, edited 5 years ago by Aymeric Augustin (next)

comment:20 Changed 5 years ago by Aymeric Augustin

Attached patch implement changes accept arguments on intermediary or final tags in a backwards compatible way.

This is exercised by the new implementation of the {% block %} tag. Namely, template test namedendblocks04 breaks if the changes in django/template/loader_tags.py are omitted.

Changed 5 years ago by Aymeric Augustin

comment:21 Changed 5 years ago by Aymeric Augustin

Resolution: fixed
Status: newclosed

In [17186]:

Fixed #3100 -- Added support for arguments on intermediate tag tokens.

comment:22 Changed 5 years ago by Aymeric Augustin

In [17187]:

Fixed #17348 -- Implemented {% elif %}. Refs #3100.

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