Closed kbayliss closed 7 months ago
This relates to commit #854e9b0 on django.template.base.NodeList.render()
, where rendered child nodes are no longer converted to str
before they're joined together. This means that child nodes which render as None
cause the error to be thrown when included in a join.
As noted on that commit, the output of render_annotated()
shouldn't need to be converted to str
, because Node.render() must return a string. So something needs changing inside django-pattern-library to make sure this is enforced.
See issue #166 & related PR https://github.com/torchbox/django-pattern-library/pull/188/ where the same change was fixed for parts of this project.
It looks to me like this previous fix was incomplete - the value of default_html
is checked against a constant for UNSPECIFIED
, but this still passes if it's None
, triggering the error.
@thibaudcolas What should the correct behaviour here be?
default_html
isn't None
default_html
is None
None
is sometimes supplied as the default_html
value and change thatFirstly, it looks like there's a mistake in the code in override_tag()
. The default_html is not UNSPECIFIED
condition will always be true because UNSPECIFIED
is never assigned as the value of default_html
. It should be the default variable of the kwarg, but it's not!
That check was originally intended to prevent falsey values being disregarded so that, for example, you could pass default_html=None
or default_html=False
and have it respected in the output. In retrospect that's unnecessary/bad. The return value here always need to be string, more so since the changes in Django that @alxbridge mentions above.
My proposed fix would be to:
default_html
kwarg so it's actually UNSPECIFIED
str(default_html)
. Quick, fixes the problem, but potentially changes the rendered output in existing pattern library implementations? It feels like it's papering over something we shouldn't allow, rather than fixing the problem, but it's the least disruptive approach.TypeError
if default_html
isn't a string. More "correct", but also more likely to cause existing pattern libraries to break (loudly, helpfully!). I don't know how much of a concern this is, and we could just make sure we flag the change in the changelog and don't release it as a minor/patch version.@bcdickinson Please see the PR linked above, which aims to implement your proposed fix. I've made the handling for non-string default_html
dependent on the version of Django used, so this shouldn't be a breaking change for anyone.
Issue Summary
Using with latest Django causes a
TypeError
(traceback below) that prevents the pattern library from rendering patterns.Technical details
Traceback
``` 11:09:06 web.1 | Traceback (most recent call last): 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/core/handlers/exception.py", line 56, in inner 11:09:06 web.1 | response = get_response(request) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/core/handlers/base.py", line 197, in _get_response 11:09:06 web.1 | response = wrapped_callback(request, *callback_args, **callback_kwargs) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/views/decorators/cache.py", line 62, in _wrapped_view_func 11:09:06 web.1 | response = view_func(request, *args, **kwargs) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/views/generic/base.py", line 103, in view 11:09:06 web.1 | return self.dispatch(request, *args, **kwargs) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/views/generic/base.py", line 142, in dispatch 11:09:06 web.1 | return handler(request, *args, **kwargs) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/utils/decorators.py", line 46, in _wrapper 11:09:06 web.1 | return bound_method(*args, **kwargs) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/views/decorators/clickjacking.py", line 36, in wrapped_view 11:09:06 web.1 | resp = view_func(*args, **kwargs) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/pattern_library/views.py", line 95, in get 11:09:06 web.1 | rendered_pattern = render_pattern(request, pattern_template_name) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/pattern_library/utils.py", line 227, in render_pattern 11:09:06 web.1 | return render_to_string(template_name, request=request, context=context) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/loader.py", line 62, in render_to_string 11:09:06 web.1 | return template.render(context, request) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/backends/django.py", line 61, in render 11:09:06 web.1 | return self.template.render(context) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 175, in render 11:09:06 web.1 | return self._render(context) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/test/utils.py", line 111, in instrumented_test_render 11:09:06 web.1 | return self.nodelist.render(context) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 1005, in render 11:09:06 web.1 | return SafeString("".join([node.render_annotated(context) for node in self])) 11:09:06 web.1 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 11:09:06 web.1 | File "/venv/lib/python3.11/site-packages/django/template/base.py", line 1005, in