Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

render_table tag overwrites "table" variable from outer context #547

Closed
thunderk opened this issue Mar 7, 2018 · 8 comments · Fixed by #550
Closed

render_table tag overwrites "table" variable from outer context #547

thunderk opened this issue Mar 7, 2018 · 8 comments · Fixed by #550

Comments

@thunderk
Copy link

thunderk commented Mar 7, 2018

The render_table tag binds its first argument (the Table object) to a table variable, which is then used in child template for rendering.
The problem is that this seems to overwrite any previous table variable set in the parent context.

Here is a small example, with a django-tables2 Table object in mytable template variable:

{% with "My poor variable" as table %}
<p>{{ table }}</p>
{% render_table mytable %}
<p>{{ table }}</p>
{% endwith %}

The output of this template is:

My poor variable
[the mytable rendered]
<django_tables2.Table object at 0x...>

Tested with version 1.17.1

@thunderk thunderk changed the title render_table tag overrides "table" variable from outer context render_table tag overwrites "table" variable from outer context Mar 7, 2018
@jieter
Copy link
Owner

jieter commented Mar 7, 2018

The comments here suggest the added entry is popped off. Maybe something changed in how django handles this and there clearly is no test checking for it.

I don't think we really need the original context, except for request, so we might be able to use a fresh context. It would really help if you are able to provide a failing test case for the test suite to solve this bug.

@jieter
Copy link
Owner

jieter commented Mar 8, 2018

@thunderk I tried reproducing the issue with this test, but it seems to pass without changes.

    def test_does_not_touch_context(self):
        '''
        Make sure the tag does not change the context of the template the tag is called from
        https://github.com/jieter/django-tables2/issues/547
        '''

        table = CountryTable([], template_name='dummy.html')
        template = Template(
            '{% load django_tables2 %}'
            '{% with "foo" as table %}{{ table }} {% render_table mytable %} {{ table }}{% endwith %}'
        )

        html = template.render(Context({
            'request': build_request(),
            'mytable': table,
        }))

        self.assertEqual(html, 'foo dummy template contents\n foo')

Can you spot the difference between your case and my test?

@jieter
Copy link
Owner

jieter commented Mar 8, 2018

Fyi, this is a commit with change which would fix this problem. But the test also passes without the fix.
d1d95a6

@thunderk
Copy link
Author

thunderk commented Mar 9, 2018

Sorry about the delay, but I'm struggling to reproduce the bug on a small case (it happens in our quite large app, with several levels of template inheritance and custom tags).
In our case, it happens when rendering a table with a TemplateColumn. Removing this column fixes the behavior. But the template used in the column does not bind a 'table' variable, so I don't see why it should interfere.
I will still try to provide you with a simple example.

@thunderk
Copy link
Author

thunderk commented Mar 9, 2018

Ok, got it, the table has to be non-empty to trigger the bug:

    def test_does_not_touch_context(self):
        '''
        Make sure the tag does not change the context of the template the tag is called from
        https://github.com/jieter/django-tables2/issues/547
        '''
        class TestTable(Table):
            col = TemplateColumn(template_name='test_template_column.html')
        table = TestTable([{}])
        template = Template(
            '{% load django_tables2 %}'
            '{% with "foo" as table %}{{ table }} {% render_table mytable %} {{ table }}{% endwith %}'
        )

        html = template.render(Context({
            'request': build_request(),
            'mytable': table,
        }))

        self.assertEqual(html[:4], 'foo ')
        self.assertEqual(html[-4:], ' foo')

@jieter
Copy link
Owner

jieter commented Mar 9, 2018

@thunderk Can you verify #550 solves your use case?

pip install https://github.com/jieter/django-tables2/archive/prevent-context-mutation.zip

@thunderk
Copy link
Author

@jieter Yes, case solved ! Thank you.

@jieter
Copy link
Owner

jieter commented Mar 12, 2018

released 1.21.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants