extends, defined block multiple time #696

CitySim · 2016-03-11T16:27:17Z

file1.html

{% block test %}
  file1
{% endblock %}

file2.html

{% extends "file1.html" %}
{% if true %}
  {% block test %}
    file2 block1
  {% endblock %}
{% else %}
  {% block test %}
    file2 block2
  {% endblock %}
{% endblock %}

expected: file2 block 1
actual: file2 block 2

the precompiled js-file for file2.html contains the function for the test-block twice with the same name so the last defined instance of the block get called regardless of what case the if falls into.

The text was updated successfully, but these errors were encountered:

carljm · 2016-03-14T23:58:20Z

On further review, Jinja2 doesn't support conditional blocks either, and I'm not sure it's feasible to support. If somebody showed up with an implementation that didn't cause other problems, I wouldn't turn it down, but for now I consider this to be working as designed, not a bug. I did add a more explicit error in this case, instead of just always using the latter definition.

* 2.x: Bump version to 2.5.0-dev.2. Update version in Changelog, too. Release 2.4.1. Backport not escaping SafeString; it's a bug. Update changelog. Merge pull request #701 from legutierr/master Merge pull request #623 from atian25/filters-escape Throw an error if a block is defined multiple times. Refs #696. Update changelog. Grammar fixes. Merge pull request #691 from dkebler/file-ext Update changelog. Merge branch 't576'

pocesar · 2016-03-24T04:33:15Z

this now throws but doesn't implement self, makes it impossible to render the same block two times (and used to work)

ronnix · 2016-03-30T09:30:33Z

Same problem, I used this to render a block twice, and now it breaks.

danneu · 2016-04-19T03:00:08Z

This wasn't a bug. It was a breaking change. Ouch.

carljm · 2016-04-19T03:13:26Z

Sorry for the upgrade surprise, everyone. If I'd thought it through more carefully, I'd have delayed this fix to 3.0.

I still think the previous behavior was a bug. There's no reasonable interpretation of multiple definitions of the same block that applies to all possible inheritance situations, and the prior behavior was accidental and arbitrary ("last one wins"), silently dropping one of the definitions in some cases. So IMO this change is going to be in 3.0 regardless, and you may want to start thinking about alternate approaches now. But I could consider a 2.5.1 release that reverts the change for now.

danneu · 2016-04-19T03:16:24Z

The bug allowed for same-file block inheritance (e.g. breaking apart a template into multiple blocks in the same file, each one executing in the same context of the file's "master" block) which seemed like handy and intended behavior.

carljm · 2016-04-19T16:07:28Z

@danneu Not sure I understand that use case; can you give an example?

danneu · 2016-04-27T14:15:56Z

@carljm Sure. I'm not trying to defend this usage, but it is something that did break for me.

I would basically use blocks instead of macros when I wanted to break apart a template file into bits (all implemented in the same file) that all could access the same context (without format arguments).

Since macros define arguments, I would reserve them for re-usable fragments across many templates since they were independently parameterized. I used same-file blocks here because they were already parameterized by the parent-block in the same file and only ever used in that file, so using macros would've been redundant and repetitive.

master.html

<!doctype html>
<head>
  <title>My Website</title>
</head>
<body>
  {% block embed %}{% endblock %}
</body>

child.html

{% extends 'layouts/master.html' %}

{% block embed %}
  <div class="row">
    template parameter: {{ someValue }}
    <div class="col-lg-3">
      {% block side %}{% endblock %}
    </div>
    <div class="col-lg-9">
      {% block main %}{% endblock %}
    </div>
  </div>
{% endblock %}

{% block side %}
  <div class="sidebar">
    This is the sidebar
    template parameter: {{ someValue }}
  </div>
{% endblock %}

{% block main %}
  <article>
    <h1>This is the article</h1>
    template parameter: {{ someValue }}
  </article>
{% endblock %}

All of my templates implement an embed block which master.html wraps. That of course works as Nunjucks intended.

But after whichever minor-level update broke the behavior of the child blocks side and main, I get the "multiple blocks defined" error. I fixed it by merely inlining the childblocks.

Is there another way I can achieve this behavior? I do understand that Nunjucks wants to avoid introducing new behavior beyond jinja2's (which I have no experience with). But I will admit that this seemed like sensible usage to me from reading the Nunjucks docs.

I basically wanted a {% macro %} (something I could implement in the same file) that had access to the context in which it was called since it was already parameterized by the parent block. For example, imagine if this was called {% fragment %}, the use-case might be more apparent.

Ideally there's already a way to achieve this and I'm just missing it.

carljm · 2016-04-27T22:46:44Z

@danneu I would do this using separate-file includes, if you don't want the context isolation provided by a macro. I'm not aware of a different way to do this in the same file.

I can see why you might have found this pattern useful, but it was never intended behavior. The thing that makes that clear (and still makes it feel like a bug to me) is that it depends on the order of the block definitions. If (in your example) you move either the side or main block above the embed block, then the empty definition is used instead, and the blocks will have no contents. If this were an intended feature of the block system, it should not be order dependent.

Blocks are for template inheritance, they aren't intended as lightweight macros.

Thanks for explaining the use case! I do still intend to restore this behavior for the rest of the 2.x series.

see mozilla/nunjucks#696

ArmorDarks · 2016-04-29T18:43:00Z

That change landed on our projects quite hard too.

It's such an odd surprise that Jinja2 actually doesn't support it.

Here is our usecase:

_layout.nj — base layout:

{% set pageSidebar = pageSidebar|default(false) %}

{##
 # @param {bool} pageSidebar Display sidebar unless it set to `false`
 #}

{% if pageSidebar %}

  <div class='grid grid+'>

    <div class='grid__item 9/12'>
      {% block main %}{% endblock %}
    </div>{# /grid__item #}

    <aside class='site-sidebar grid__item 3/12'>
      {% block sidebar %}{% endblock %}
    </aside>{# /site-sidebar #}{# /grid__item #}

  </div>{# /grid #}

{% else %}

  {% block main %}{% endblock %}

{% endif %}

And page, which uses it:

{% extends '_layout.nj' %}


{# --------- #}
{% set pageSidebar = true %}
{# --------- #}

{% block sidebar %}
  Sidebar content
{% endblock %}

{% block main %}
  Main content
{% endblock %}

Now I'm thinking about ways to elegantly reimplement it...

CSS-only way definitely no-go way, since disabled by display: none chunk of html will stay and will be indexed by search bots.

Heavy usage of if statements on certain elements doesn't seem to be elegant solution too.

Solution like that

{% if pageSidebar %}
  {% block sidebar %}
    Sidebar content
  {% endblock %}
{% endif %}

is out of question, because it's poorly designed one and very easy to break, if one of devs will forget to wrap sidebar into if statement. That should be inherited from parent, not defined by child.

We just need self object. which will resolve this issue and allow reusage of block without overriding anything accidentaly: http://jinja.pocoo.org/docs/dev/templates/#child-template

In fact, that change shouldn't to land before self actually will be implemented.

carljm · 2016-04-29T18:51:24Z

@ArmorDarks I'd love to see an implementation of self.

Using multiple block definitions within if like that also kinda works for some use cases, but is kinda broken. When you use it with empty blocks in a parent template it works, but if you try it in a child template with blocks with contents, I think you'll find that the second definition is always used, regardless of which way the if evaluates. So again, I think if one tries to see this as a feature, at best it is a half-broken feature that can easily bite you with unexpected behavior.

For your use case what I would typically do is have two different base templates; the child template can conditionally extend one or the other.

ArmorDarks · 2016-04-29T19:00:57Z

@carljm

Using multiple block definitions within if like that also kinda works for some use cases, but is kinda broken.

Do you mean

{% if pageSidebar %}
  {% block sidebar %}
    Sidebar content
  {% endblock %}
{% endif %}

or

{% set pageSidebar = pageSidebar|default(false) %}

{##
 # @param {bool} pageSidebar Display sidebar unless it set to `false`
 #}

{% if pageSidebar %}

  <div class='grid grid+'>

    <div class='grid__item 9/12'>
      {% block main %}{% endblock %}
    </div>{# /grid__item #}

    <aside class='site-sidebar grid__item 3/12'>
      {% block sidebar %}{% endblock %}
    </aside>{# /site-sidebar #}{# /grid__item #}

  </div>{# /grid #}

{% else %}

  {% block main %}{% endblock %}

{% endif %}

?

btw, since we already have #164, I think that issue can be closed, since solution to it is same — self variable

carljm · 2016-04-29T19:09:59Z

I meant the first, but I might be wrong. It might be only in the case of the parent template where it doesn't work; the second value defined in the parent template would always be inherited to the child template for that block, regardless of if/else (because in the inheritance situation nunjucks does not actually render the parent, it scans its AST for all blocks and renders those, so the if/else is disregarded). Anyway, I'd need to investigate to get the details right, but I'm sure that in some cases having blocks within if/else will have very surprising results, due to the AST scan.

This ticket is still open because I want to revert this change for the remainder of the 2.x series.

shrpne · 2017-09-28T12:39:57Z

would be great to implement block() function from twig https://twig.symfony.com/doc/2.x/tags/extends.html#child-template

bewards · 2019-08-14T21:34:45Z

How was the scenario below resolved? I'm still getting Block "main" defined more than once, within my layout. I need the "authenticated" experience to contain a div.grid-container markup wrapped around the main nunjuck block.

  {% if isPublic %}

  {% include "includes/headers/_header.html" %}
  <main>{% block main %}{% endblock %}</main>
  {% include "includes/footers/_footer-public.html" %}

  {% else %}
  
  {% include "includes/headers/_header-auth.html" %}
  <div class="grid-container">
    {% include "includes/headers/_header-sidebar.html"  %}
    <main>{% block main %}{% endblock %}</main>
    {% include "includes/footers/_footer-auth.html" %}
  </div>
  {% endif %}

muriware · 2019-10-04T09:50:11Z

I agree that this should be resolved.
In the mean time, a way around for some of the examples presented above:

@CitySim

From

{% if true %}
  {% block test %}
    file2 block1
  {% endblock %}
{% else %}
  {% block test %}
    file2 block2
  {% endblock %}
{% endblock %}

To

{% block test %}
  {% if true %}
    file2 block1
  {% else %}
    file2 block2
  {% endif %}
{% endblock %}

@ArmorDarks

From

{% if pageSidebar %}

  <div class='grid grid+'>
    <div class='grid__item 9/12'>
      {% block main %}{% endblock %}
    </div>{# /grid__item #}
    <aside class='site-sidebar grid__item 3/12'>
      {% block sidebar %}{% endblock %}
    </aside>{# /site-sidebar #}{# /grid__item #}
  </div>{# /grid #}

{% else %}

  {% block main %}{% endblock %}

{% endif %}

To

{% if pageSidebar %}
  <div class='grid grid+'>
    <div class='grid__item 9/12'>
{% endif %}

      {% block main %}{% endblock %}

{% if pageSidebar %}
    </div>{# /grid__item #}
    <aside class='site-sidebar grid__item 3/12'>
      {% block sidebar %}{% endblock %}
    </aside>{# /site-sidebar #}{# /grid__item #}
  </div>{# /grid #}
{% endif %}

@bewards

From

  {% if isPublic %}

  {% include "includes/headers/_header.html" %}
  <main>{% block main %}{% endblock %}</main>
  {% include "includes/footers/_footer-public.html" %}

  {% else %}

  {% include "includes/headers/_header-auth.html" %}
  <div class="grid-container">
    {% include "includes/headers/_header-sidebar.html"  %}
    <main>{% block main %}{% endblock %}</main>
    {% include "includes/footers/_footer-auth.html" %}
  </div>
  
{% endif %}

To

{% if isPublic %}
  {% include "includes/headers/_header.html" %}
{% else %}
  {% include "includes/headers/_header-auth.html" %}
  <div class="grid-container">
    {% include "includes/headers/_header-sidebar.html"  %}
{% endif %}

  <main>{% block main %}{% endblock %}</main>

{% if isPublic %}
  {% include "includes/footers/_footer-public.html" %}
{% else %}
    {% include "includes/footers/_footer-auth.html" %}
  </div>
{% endif %}

bewards · 2019-10-04T13:23:51Z

@jmurinello confirmed that this works, just makes the formatting of end tags confusing - I'll be using this for now, thanks

muriware · 2019-10-05T02:53:37Z

just makes the formatting of end tags confusing

@bewards, I agree. For that reason I would suggest refactoring.

Like @carljm mentioned before:

... what I would typically do is have two different base templates; the child template can conditionally extend one or the other.

As such:

includes/main/_public.nj

{% include "includes/headers/_header.html" %}
<main>{% block main %}{% endblock %}</main>
{% include "includes/footers/_footer-public.html" %}

includes/main/_private.nj

{% include "includes/headers/_header-auth.html" %}
<div class="grid-container">
  {% include "includes/headers/_header-sidebar.html"  %}
  <main>{% block main %}{% endblock %}</main>
  {% include "includes/footers/_footer-auth.html" %}
</div>

Then:

{% if isPublic %}
  {% extends "includes/main/_private.html" %}
{% else %}
  {% extends "includes/main/_private.html" %}
{% endif %}

CitySim mentioned this issue Mar 11, 2016

test case for #696 #697

Closed

carljm added the bug label Mar 12, 2016

carljm added a commit that referenced this issue Mar 14, 2016

Throw an error if a block is defined multiple times. Refs #696.

e2efe90

carljm added a commit that referenced this issue Mar 14, 2016

Throw an error if a block is defined multiple times. Refs #696.

c1ad297

carljm closed this as completed Mar 14, 2016

carljm reopened this Apr 27, 2016

mrhyde added a commit to LotusTM/Kotsu that referenced this issue Apr 28, 2016

[nj] commented out conditional block handling

217c67a

see mozilla/nunjucks#696

ArmorDarks mentioned this issue Apr 29, 2016

self variable #741

Closed

ArmorDarks mentioned this issue Apr 29, 2016

self variable is not defined #164

Open

This was referenced Nov 14, 2017

some nunjucks snippets have {{- some just have {{ alphagov/govuk-frontend#344

Closed

Make macros available to all views alphagov/govuk-prototype-kit-private-beta#7

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extends, defined block multiple time #696

extends, defined block multiple time #696

CitySim commented Mar 11, 2016

carljm commented Mar 14, 2016

pocesar commented Mar 24, 2016

ronnix commented Mar 30, 2016

danneu commented Apr 19, 2016

carljm commented Apr 19, 2016

danneu commented Apr 19, 2016 •

edited

Loading

carljm commented Apr 19, 2016

danneu commented Apr 27, 2016 •

edited

Loading

carljm commented Apr 27, 2016

ArmorDarks commented Apr 29, 2016 •

edited

Loading

carljm commented Apr 29, 2016

ArmorDarks commented Apr 29, 2016

carljm commented Apr 29, 2016

shrpne commented Sep 28, 2017

bewards commented Aug 14, 2019 •

edited

Loading

muriware commented Oct 4, 2019

bewards commented Oct 4, 2019

muriware commented Oct 5, 2019

extends, defined block multiple time #696

extends, defined block multiple time #696

Comments

CitySim commented Mar 11, 2016

carljm commented Mar 14, 2016

pocesar commented Mar 24, 2016

ronnix commented Mar 30, 2016

danneu commented Apr 19, 2016

carljm commented Apr 19, 2016

danneu commented Apr 19, 2016 • edited Loading

carljm commented Apr 19, 2016

danneu commented Apr 27, 2016 • edited Loading

master.html

child.html

carljm commented Apr 27, 2016

ArmorDarks commented Apr 29, 2016 • edited Loading

carljm commented Apr 29, 2016

ArmorDarks commented Apr 29, 2016

carljm commented Apr 29, 2016

shrpne commented Sep 28, 2017

bewards commented Aug 14, 2019 • edited Loading

muriware commented Oct 4, 2019

bewards commented Oct 4, 2019

muriware commented Oct 5, 2019

danneu commented Apr 19, 2016 •

edited

Loading

danneu commented Apr 27, 2016 •

edited

Loading

ArmorDarks commented Apr 29, 2016 •

edited

Loading

bewards commented Aug 14, 2019 •

edited

Loading