Opened 2 years ago
Closed 2 years ago
#33830 closed Bug (fixed)
Variable lookup errors are logged rendering 'clearable_file_input.html'
Reported by: | Horst Schneider | Owned by: | Neeraj Kumar |
---|---|---|---|
Component: | contrib.admin | Version: | 4.0 |
Severity: | Normal | Keywords: | admin, template |
Cc: | Neeraj Kumar | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
It seems like the fix to #31536 raised a problem similar to #32681: Not checking whether the disabled
attribute actually exists on the attrs
of the 'clear' checkbox widget causes a VariableDoesNotExist
exception to be logged every time one of the patched clearable_file_input.html
templates is rendered with a checkbox that has no disabled
atrribute (i.e. is enabled):
[2022-07-06 10:06:03,452] DEBUG django.template base: Exception while resolving variable 'disabled' in template 'admin/widgets/clearable_file_input.html'. Traceback (most recent call last): File "/home/horst/some_project/venv/lib/python3.10/site-packages/django/template/base.py", line 875, in _resolve_lookup current = current[bit] KeyError: 'disabled' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/horst/some_project/venv/lib/python3.10/site-packages/django/template/base.py", line 885, in _resolve_lookup current = getattr(current, bit) AttributeError: 'dict' object has no attribute 'disabled' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/horst/some_project/venv/lib/python3.10/site-packages/django/template/base.py", line 891, in _resolve_lookup current = current[int(bit)] ValueError: invalid literal for int() with base 10: 'disabled' During handling of the above exception, another exception occurred: Traceback (most recent call last): File "/home/horst/some_project/venv/lib/python3.10/site-packages/django/template/base.py", line 898, in _resolve_lookup raise VariableDoesNotExist( django.template.base.VariableDoesNotExist: Failed lookup for key [disabled] in {'id': 'id_document'}
Change History (17)
comment:1 by , 2 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 2 years ago
Type: | Uncategorized → Bug |
---|
comment:3 by , 2 years ago
Although the problem is similar, the fix must be different here. We can not easily supply some default context from the outside here since the missing variable is in the attrs
attribute of the checkbox widget.
It is guaranteed by the base Widget
that attrs
will always be a dict
instance. But it is not guaranteed that the key disabled
will always be present on the attrs
, so we will mostly find it to be either missing or set to True
(could as well be set to False
, if the default is explicitly stated).
I guess a proper fix would be something along the lines of
<input type="checkbox" name="{{ widget.checkbox_name }}" id="{{ widget.checkbox_id }}" {% if 'disabled' in widget.attrs and widget.attrs.disabled %} disabled{% endif %} >
(First checking if key is present, then checking its truthiness).
Or is there a more idiomatic way to perform those checks?
follow-up: 7 comment:4 by , 2 years ago
follow-up: 6 comment:5 by , 2 years ago
@Horst could you investigate setting a default (False
) for disabled in ClearableFileInput for this case?
follow-up: 8 comment:6 by , 2 years ago
Replying to Carlton Gibson:
@Horst could you investigate setting a default (
False
) for disabled in ClearableFileInput for this case?
Setting it both to True
or False
explicitly in the attrs
does not trigger the VariableDoesNotExist
exception to be logged. Only the absence of the disabled
attribute on the attrs
altogether triggers it.
comment:7 by , 2 years ago
Replying to Carlton Gibson:
Either that or we'd need a fix to #28172 maybe? See also #28526 — I wonder if there isn't a cluster of duplicates here? 🤔
Not really duplicates, right? #28172 seems to be a very specific different issue with filters and #28526 addresses the whole concept of logging missing template variables in such a verbose fashion.
comment:8 by , 2 years ago
Replying to Horst Schneider:
Replying to Carlton Gibson:
@Horst could you investigate setting a default (
False
) for disabled in ClearableFileInput for this case?
Setting it both to
True
orFalse
explicitly in theattrs
does not trigger theVariableDoesNotExist
exception to be logged. Only the absence of thedisabled
attribute on theattrs
altogether triggers it.
This error already handled in Exception when we do not provide "disabled" in attrs.
raise VariableDoesNotExist("Failed lookup for key " "[%s] in %r", (bit, current)) # missing attribute
except Exception as e: template_name = getattr(context, 'template_name', None) or 'unknown' logger.debug( "Exception while resolving variable '%s' in template '%s'.", bit, template_name, exc_info=True, )
comment:9 by , 2 years ago
<input type="checkbox" name="{{ widget.checkbox_name }}" id="{{ widget.checkbox_id }}" {% if 'disabled' in widget.attrs and widget.attrs.disabled %} disabled{% endif %} >
This solution can work to stop the trigger for disabled
attribute in attrs.
comment:10 by , 2 years ago
Cc: | added |
---|
comment:11 by , 2 years ago
The issue with the {% if 'disabled' in widget.attrs and widget.attrs.disabled %} disabled{% endif %}
approach is that it makes the template somewhat verbose.
The thought is that was can avoid that by setting a default False
for the disabled attribute in ClearableFileInput
— If that works (i.e. doesn't trigger the VariableDoesNotExist
, shown by a new test similar to those added in e.g. 0a4a5e5bacc354df3132d0fcf706839c21afb89d, and doesn't break any existing tests) then that would be the preferred candidate for a fix.
follow-up: 14 comment:13 by , 2 years ago
Needs tests: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
Thanks for the patch Neeraj.
Can you add a test case using assertNoLogs()
?
If that works (i.e. doesn't trigger the VariableDoesNotExist, shown by a new test similar to those added in e.g. 0a4a5e5bacc354df3132d0fcf706839c21afb89d,
comment:14 by , 2 years ago
Replying to Carlton Gibson:
Thanks for the patch Neeraj.
Can you add a test case using
assertNoLogs()
?
If that works (i.e. doesn't trigger the VariableDoesNotExist, shown by a new test similar to those added in e.g. 0a4a5e5bacc354df3132d0fcf706839c21afb89d,
Added testcase using assertNoLogs()
comment:15 by , 2 years ago
Needs tests: | unset |
---|
comment:16 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
OK, yes — a fix to suppress that, similar to #32681 would be welcome. Thanks.