Code

Opened 7 years ago

Closed 2 years ago

Last modified 2 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: aaugustin
Component: Template system Version: master
Severity: Normal Keywords:
Cc: danring, 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@…> 7 years ago.
patch-breaking-namedendblocks-in-regressiontests-templates.diff (1.1 KB) - added by danring 5 years ago.
patch breaks regressiontests.templates 'namedendblocks0[234]'
django_elif_docs.diff (817 bytes) - added by cwebber 4 years ago.
Docs for the new {% elif %} tag
django_elif.diff (8.8 KB) - added by cwebber 4 years ago.
Django {% elif %} tag support (now with docs in the same patch)
fix3100.patch (9.0 KB) - added by mk 4 years ago.
3100-alternate.diff (1.3 KB) - added by SmileyChris 3 years ago.
An alternative to adding a parse_until_args_allowed argument
3100-backwards-compatible.patch (2.1 KB) - added by aaugustin 2 years ago.

Download all attachments as: .zip

Change History (29)

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

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

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

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

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

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

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

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

  • Needs tests set
  • Patch needs improvement set
  • Resolution duplicate deleted
  • Status changed from closed to reopened
  • Summary changed from [patch]: Support for arguments on intermediate tag tokens (ex. {% if arg %}{% elif arg %}{% endif %}) to 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 5 years ago by julien

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

comment:6 Changed 5 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

Changed 5 years ago by danring

patch breaks regressiontests.templates 'namedendblocks0[234]'

comment:7 Changed 5 years ago by danring

  • Cc danring 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 4 years ago by adrian

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

comment:9 Changed 4 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 4 years ago by cwebber

  • Needs tests unset
  • Patch needs improvement unset

Changed 4 years ago by cwebber

Docs for the new {% elif %} tag

comment:11 Changed 4 years ago by cwebber

  • Triage Stage changed from Accepted to Ready for checkin

Added docs for the {% elif %} tag.

Changed 4 years ago by cwebber

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

comment:12 Changed 4 years ago by mitar

  • Cc mmitar@… added

comment:13 Changed 4 years ago by mtredinnick

  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 4 years ago by mk

  • 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 4 years ago by mk

comment:15 Changed 3 years ago by lrekucki

  • Severity changed from normal to Normal
  • Type changed from enhancement to New feature

Changed 3 years ago by SmileyChris

An alternative to adding a parse_until_args_allowed argument

comment:16 Changed 2 years ago by foxwhisper

  • Easy pickings unset
  • UI/UX unset

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

Cal

comment:17 Changed 2 years ago by aaugustin

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

comment:18 Changed 2 years ago by aaugustin

  • Owner changed from adrian to aaugustin
  • Status changed from reopened to new

comment:19 Changed 2 years ago by aaugustin

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

Last edited 2 years ago by aaugustin (previous) (diff)

comment:20 Changed 2 years ago by aaugustin

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 2 years ago by aaugustin

comment:21 Changed 2 years ago by aaugustin

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

In [17186]:

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

comment:22 Changed 2 years ago by aaugustin

In [17187]:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.