[Django] #28001: Possible unnecessary code if ForNode.render()
Django
------------------------------------------------+------------------------
Reporter: Baptiste Mispelon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.10
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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
--
Ticket URL: <https://code.djangoproject.com/ticket/28001>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
--------------------------------------+------------------------------------
Reporter: Baptiste Mispelon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Confirmed that's unneeded since 3bbebd06adc36f31877a9c0af6c20c5b5a71a900
(a test failed before that change with the `context.pop()` removed).
[https://github.com/django/django/pull/8273 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28001#comment:1>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
"""
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28001#comment:2>
Django
--------------------------------------+------------------------------------
Reporter: Baptiste Mispelon | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28001#comment:3>
Django
Reporter: Baptiste Mispelon | Owner: kapil
Type: | garg
Cleanup/optimization | Status: assigned
Component: Template system | Version: 1.10
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by kapil garg):
* owner: nobody => kapil garg
* status: new => assigned
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/28001#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: kapil
Type: | garg
Cleanup/optimization | Status: assigned
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28001#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: kapil
Type: | garg
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by kapil garg):
* version: 1.10 => 1.11
--
Ticket URL: <https://code.djangoproject.com/ticket/28001#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: kapil
Type: | garg
Cleanup/optimization | Status: assigned
Component: Template system | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Comment (by 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28001#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: kapil
Type: | garg
Cleanup/optimization | Status: assigned
Component: Template system | Version: 1.11
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Comment (by kapil garg):
[https://github.com/django/django/pull/8294/ PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/28001#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Baptiste Mispelon | Owner: kapil
Type: | garg
Component: Template system | Version: 1.11
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"dbfcedb499944f31444d347aa6c389303c6cf22e" dbfcedb]:
{{{
#!CommitTicketReference repository=""
revision="dbfcedb499944f31444d347aa6c389303c6cf22e"
Fixed #28001 -- Updated comment and tested context popping in
ForNode.render().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28001#comment:9>