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

[Baptiste Mispelon]

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

[Tim Graham]

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

[kapil garg]

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

[Tim Graham]

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.

[kapil garg]

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.

[kapil garg]

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.

[kapil garg]

​PR

[Tim Graham <timograham@…>]

In dbfcedb:

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