include_block template tag for improved StreamField template inclusion

[gasman]

As proposed in #1743 (comment) - this PR implements an {% include_block my_block %} template tag, which works equivalently to Django's {% include 'my_template.html' %} tag (including constructs like {% include_block my_block with classname='foo' only %}).

This improves on the current convention of rendering StreamField blocks using variable tags e.g. {{ my_block }}, as it allows the child template to inherit the template context - this is necessary for the {% pageurl %} tag to work (and also provides access to the page variable, which is frequently requested - e.g. to allow building StreamField-based forms that post back to the current page).

As part of this change, the render and render_basic methods on Block now accept a context kwarg, which introduces a backwards incompatibility (since third-party libraries may have overridden these methods with the old method signature) - I've put compatibility shims in place to mitigate this.

Fixes #1743, #2469.

[m1kola]

At first glance looks good, but I want come back to it with fresh eyes tomorrow.

Thanks Matthew!

[m1kola]

It seems to me that we need to add a similar tag for the Jinja2 template engine.

[m1kola]

If we need an access to a request (to build a pagination object for a block, for example), we need to override the render method instead of creating the pagination object in the get_context method. Is that right?

[m1kola]

Other than questions above, looks good to me.

Thank you @gasman!

[m1kola]

Hm. If 3-rd party block raises TypeError (I don't know why they may do that :)), render method will be called twice even if block have proper signature. It will be hard to debug for 3-rd party developers.

    def render(self, value, context=None):
        # do something that raises a TypeError

Probably we need to use inspect. Something like this:

import inspect


class Foo:
    def render(self, value):
        print("I'm alive")
    def _render_with_context(self, value, context=None):
        if 'context' in inspect.getargspec(self.render).args:
            return self.render(value, context=context)
        else:
            result = self.render(value)
            # Warn and bla-bla-bla


test = Foo()
test._render_with_context(1, {"two": 2})

[gasman]

I don't think it's worth making our code more complicated (and less Pythonic...) just to handle this very obscure special case.

This would only be a problem if the render method has side effects - otherwise, it doesn't matter that we call it twice. A render method that throws TypeError and has side effects would have to be doing something very hacky and non-standard indeed - if they have a hard time debugging it as a result, I think that's fair punishment ;-)

[m1kola]

just to handle this very obscure special case.

It may be just typo or "4-th party code" can riseTypeError... For example, I've tried to add a pagination to a block. It's a bad solution because I pass request object into my block through templates, but it's a good example when it will be hard to figure out what's going on.

    def render(self, value, context=None):
        request = context.get('request') if context else None
        if not request:
            raise Exception('Sad trombone sound')

        pages = context.get('pages')

        per_page = settings.DEFAULT_PER_PAGE
        paginator = Paginator(pages, per_page)
        page_number = request.GET.get('page')

        try:
            pages = paginator.page(page_number)
        except PageNotAnInteger:
            # Hint: TypeError will be raised somewhere in Django's Paginator
            pages = paginator.page(1)
        except EmptyPage:
            pages = paginator.page(paginator.num_pages)

        context = dict(context)
        context['pages'] = pages

        return super().render(value, context)

[gasman]

Ah, yes, I see. So, the problem isn't really about render being unexpectedly called twice - it's that it's missing the context on the second occasion (and that's the one you'll see in the debug output). That's a fair point - I'll fix that with your suggested inspect approach. Thanks!

[m1kola]

Yes, I haven't expected to see Exception: Sad trombone sound and it took some time to figure out why I've got this exception instead of TypeError that was thrown by the paginator.

inspect - it's just the first idea that came to my head. If you know more Pythonic way, feel free to use it ;)

[gasman]

If we need an access to a request (to build a pagination object for a block, for example), we need to override the render method instead of creating the pagination object in the get_context method. Is that right?

Hmm, good point! I can see how it would be useful to have access to the parent context inside get_context. I think that's better handled in a separate PR though, as it's quite a bit more complicated than updating render / render_basic - we can't just change the signature to get_context(self, value, context=context) because we also need to consider what happens when old code calls super(MyBlock, self).get_context(value). (Writing the documentation to explain what it means to pass a context to get_context is going to be quite difficult too!)

[m1kola]

I think that's better handled in a separate PR though

Absolutely agree.

Also you can implement a Jinja2 template tag in a separate PR, if you wish. But I think that we should release Jinja2 template tag in the same time as Django's one.

[gasman]

Rebased, and updated to check the method signatures of render and render_basic using inspect.getargspec, as suggested.

[gasman]

I agree that we ought to support Jinja2 templates here too - I was meaning to mention that in the ticket description, but forgot...

I've just been reading the docs for Jinja2's include / import tags, and my first impression is that they differ significantly from Django's approach. Given that {% include_block %} is designed to closely follow the syntax of {% include %}, I feel that it would probably be better implemented by someone who's more familiar with Jinja2 idioms, and has a clear idea of what a Jinja2 version of {% include_block %} should look like. (@timheap, would that person be you? :-) )

[m1kola]

@gasman I'm not sure that I more familiar with Jinja2 idioms, but I've used it for a long time.

include tags of both template engines are not so different. The only significant difference, in my opinion, is how Jinja2 allows you to control which variables will be passed into the context. You can specify with context (by default) or without context in include tag. If you want to pass only specified variables, you have to use with tag. It will be something like this:

{% with my_var=something %}
    {% include 'my_template.html' %}
{% endwith %}

It seems to me that implementation of a Jinja2 version of our include_block will be simpler than implementation of a Django version because with tag does almost all business. As far as I remember, with goes out of the box with Jinja2, but it disabled by default (btw, I don't think that we need to care if it disabled).

I will back to this PR again in the morning.

P.S.: Damn! We need another solution for Python 3.5 (see tests).

[gasman]

Fix for Python 3.5 added. I'm afraid it's not pretty... I look forward to Wagtail 1.8, when we can remove this pile of hacks :-)

[mx-moth]

Adding an {% include_block %} tag should be possible. I think it would only need to support the with/without context, and would not have to support naming templates or extra kwargs:

{% include_block block %}{# Includes context by default #}
{% include_block block with context %}{# Explicitly include context #}
{% include_block block without context %}{# Does not use context #}

Template authors can use {% with %} or `{% set %} if they need to provide extra context.

[m1kola]

Merged in dbc4c9b (release notes in b79069b).

I've created 2 issues:

• A Jinja2 version of include_block template tag #2823 for a Jinja2 version of {% include_block %}
• Allow access to the parent context inside get_context of StreamField blocks #2824 for parent context inside get_context

Thanks @gasman and @timheap!