Fixed bug where empty placeholder would render as 'None' #5734
Conversation
This bug would appear when "placeholder" template tag appeared in a
template being rendered by a non-CMS view.
The render_page_placeholder function had a bare "return" instead of
returning an explicit empty string.
See also the code that would be executed for the same case in
django-cms 3.3.x (in cms_tags.py:render_tag in that release):
page = request.current_page
if not page or page == 'dummy':
if nodelist:
return nodelist.render(context)
return ''
|
Hello @cworth-gh, Having a We decided to "fail silently" when this change was introduced back in 3.0.0 because it would have caused a lot of failures for many people, now I'm not so sure we should continue to fail silently. Anyways, for the reasons above I'm ok to merge this ( @yakky thoughts? |
|
Hi @czpython, Thanks for the quick reply. In our use case, I don't think {% static placeholder %} will do what we want. What we have is a shared, base template that is used by both CMS and non-CMS views. We just expect the {% placeholder %} tags to render as empty for the non-CMS views. So, as of 3.3.x the fact that these are rendered empty, (and silently, even though not technically supported), is quite helpful for us. If there's something different we "should" be doing for a case like this, let me know. -Carl |
|
This is a valid case, forcing users to override their base template blocks to remove the {% placeholder %} tags would not be good. That said, I think for the future (thinking 3.5.0) we will no longer render a placeholder outside of the cms in edit mode. So when users switch to an app template that inherits from base template with placeholder tags, these placeholders won't show up in structure mode. |
|
@czpython I'm ok with the proposed change and ok for not needing a dedicated test |
|
Thanks @cworth-gh |
…5734) This bug would appear when "placeholder" template tag appeared in a template being rendered by a non-CMS view. The render_page_placeholder function had a bare "return" instead of returning an explicit empty string. See also the code that would be executed for the same case in django-cms 3.3.x (in cms_tags.py:render_tag in that release): page = request.current_page if not page or page == 'dummy': if nodelist: return nodelist.render(context) return ''
This bug would appear when "placeholder" template tag appeared in a
template being rendered by a non-CMS view.
The render_page_placeholder function had a bare "return" instead of
returning an explicit empty string.
See also the code that would be executed for the same case in
django-cms 3.3.x (in cms_tags.py:render_tag in that release):
Note: I'd like to contribute a test case for this as well. The existing test at cms.tests.test_rendering.RenderingTestCase.test_placeholder looks like
the closest thing for this. I'm just not sure what it would take to modify that
test case so that it would hit the conditional return modified by this code.
(Just changing self.test_page to None did not do the trick.) When I've
exercised this bug manually, I've done it by creating a non-CMS view that
renders with a template that contains a placeholder template tag.
Perhaps that would be an easy test to write for someone with more
experience writing Django tests. Any guidance you might be able to give
me on how to write that test would be appreciated.