Opened 7 years ago

Closed 7 years ago

#28001 closed Cleanup/optimization (fixed)

Investigate/update comment about context popping in ForNode.render()

Reported by: Baptiste Mispelon Owned by: kapil garg
Component: Template system Version: 1.11
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The code for ForNode.render() [1] has a bit of code with a comment that reads:

# The loop variables were pushed on to the context so pop them
# off again. This is necessary because the tag lets the length
# of loopvars differ to the length of each set of items and we
# don't want to leave any vars from the previous loop on the
# context.

I believe this sentence is incorrect, because {% for ... %} raises an error when there's a mismatch between the variable count and the number of items retrieve during iteration (this behavior is tested explicitly [2]).

I did a bit of digging and this comment was correct when it was added (16269c4d0a5d2e61c7555fec438440abee9be9f5) but the error-raising feature was added after that.

When removing the context.pop() just after that comment, the test suite still passes which indicates that either the code is unnecessary, or it's untested.

[1] https://github.com/django/django/blob/70197241017575b60973b038c8f68dcb18526110/django/template/defaulttags.py#L213-L219
[2] https://github.com/django/django/blob/70197241017575b60973b038c8f68dcb18526110/tests/template_tests/syntax_tests/test_for.py#L157

Change History (9)

comment:1 by Tim Graham, 7 years ago

Has patch: set
Summary: Possible unnecessary code if ForNode.render()Remove obsolete context popping in ForNode.render()
Triage Stage: UnreviewedAccepted

Confirmed that's unneeded since 3bbebd06adc36f31877a9c0af6c20c5b5a71a900 (a test failed before that change with the context.pop() removed).
PR

comment:2 by kapil garg, 7 years ago

I think the comment needs to be changed but the code should remain there because if there are multiple variables and we are pushing them onto the context then we should pop them off because they are not of any use in next iteration. If we don't pop those variables, the context will keep on growing without any significance.
Its not that this is obsolete line of code, it works now as an optimization to save stack memory.
The comment should be changed to

"""
# The loop variables were pushed on to the context so pop them
# off again. This is necessary to prevent context stack from growing
# insignificantly.
"""

comment:3 by Tim Graham, 7 years ago

Has patch: unset
Summary: Remove obsolete context popping in ForNode.render()Investigate/update comment about context popping in ForNode.render()

There are also some failing admin tests the cause of which isn't obvious. It would be nice to investigate that and add a test in template_tests so this isn't tested only in a contrib app.

comment:4 by kapil garg, 7 years ago

Needs tests: set
Owner: changed from nobody to kapil garg
Status: newassigned

comment:5 by kapil garg, 7 years ago

The reason admin tests were failing because 'forms/widgets/attrs.html' uses 2 variables in for loop which implies that those variables are pushed onto stack.

admin's 'split_datetime.html' template uses 'with' tag to push extra content to context which is 'widget = widget.subwidgets.0' for date and 'widget = widget.subwidgets.1' for time.
Note here that now the context has a different variable with name 'widget'.
This calls 'attrs.html' after inclusions of 'text.html and input.html' and 'attrs.html' pushes its variables onto the context.
Now when "with" tag exits it pops the context which just pops "attrs.html" variables and the 'widget' variable is still there which was used as a local context.

When 'split_datetime.html' calls second 'with' node it gets the previous 'with' node's widget variable which has no 'subwidgets' and thus ends in being widget value to None.

A None value is passed on and the next include tag in 'with' node try to get a 'None' Template

So the final conclusion is : Whatever goes onto the context gets popped out when its of no use and thus we need that line of code.

comment:6 by kapil garg, 7 years ago

Version: 1.101.11

comment:7 by kapil garg, 7 years ago

Also , It is to note that 'None' template name actually passed the engine and the loaders tried to load that template even the template_name was None.
The error I got was 'IsADirectoryError' because loaders appended nothing to template directory and tried to load that directory.
So i think we need to add a check in engine for invalid template names.

comment:8 by kapil garg, 7 years ago

comment:9 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: assignedclosed

In dbfcedb:

Fixed #28001 -- Updated comment and tested context popping in ForNode.render().

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