BlockNode unsafely manages context

[Sean Stoops]

To allow the use of {{ block.super }}, the BlockNode uses context.push() to add itself to the context at position zero. After the scope of the context has been rendered, the BlockNode then assumes it is still position zero and removes itself with context.pop().

    def render(self, context):
        context.push()
        # Save context in case of block.super().
        self.context = context
        context['block'] = self
        result = self.nodelist.render(context)
        context.pop()
        return result

The problem with this arises when any template tag call inside a {% block ... %} tag modifies the context. Anything added to the context assumes position zero, thus when the BlockNode attempts to clean up with context.pop(), it will remove whatever was last added to the context and possibly leave the block object behind.

To resolve this issue, instead of using the offending context.pop(), I propose storing a reference to the object the BlockNode adds to the context, then removing that object from the context by reference with context.dicts.remove(reference). This method ensures the block object is removed from the context and that all other context objects will be available in the next {% block ... %} occurrence.

Patch attached.

[Sean Stoops]

First patch

[Chris Beaven]

I'm not sure whether Django should worry about cleaning up after tags which aren't doing the right thing (adding a context layer without removing it again).
Then again, that's not an invalid thing to do, so maybe: design decision needed.

[Sean Stoops]

I figure since the official documentation demonstrates how to update the context, then django should do it's best to not break that functionality. I've seen no mention anywhere saying that tags should clean up after themselves.

​http://docs.djangoproject.com/en/dev/howto/custom-template-tags/#setting-a-variable-in-the-context

I personally view this as more of a bugfix than an enhancement. I find it rather silly that the block node blindly removes position 0 in the first place.

One scenario I find it almost vital that variables can be set in one block node, and carry across to others, is on a query-heavy page (with template tags doing the heavy querying). I don't want to have to repeat the same tag call in every block node that needs the results; I'd like to just run it once and roll with it. We (my dev team) have many hundred templates dependent on this patch and have seen no ill effects. Though I do agree, proper testing should still be written if this bugfix/enhancement is deemed necessary.

[Chris Beaven]

I think you're misinterpreting what context.pop() actually does - it doesn't pop the most recent element entered ('block'), it pops off the whole context layer it created when context.push() was run.

Setting a variable in the context (refering to the documentation link you provided) doesn't create a new context layer, it just adds to the current context layer.

So if your really point of concern is that the lifespan of context attributes created inside of a block tag only have the lifespan of that block, then that is a different issue (it's not "unsafely managing" anything at least). And perhaps there should be a public method for adding to the base context layer - your tags can technically do it already by adding the "global" variable to context.dicts[len(context.dicts)-1].

[Sean Stoops]

Ah I think I see where the disconnect is on this issue; my fault. I understand how all the push/pop logic works, but our issue has always been we've used context.update({}) (useful when setting multiple items) instead of going through __setitem__. You're right in that if you just use dictionary access, it will add the variable to the current context layer, then the block node will remove that entire layer at the conclusion of the block.

However, should the block node further "scope" the context? I mean, if you add several more context layers from within a block node, should the block node remove all layers that have been added since the start of the block ...

context.dicts = context.dicts[context.dicts.index(stored_reference):]

This would prevent the issue of the block assuming position it's 0 if the context is mismanaged by a user and in essence create a truer "scope".

From here, I would agree that having the public method for adding to the base context layer might not be a bad idea. It might also be convenient to note in documentation (docstring/sphynx docs) the difference between using __setitem__ and update and/or clarification on how block nodes manage context layers like a true scope. I could see how this might bite a user wanting to treat the context object like a typical dictionary.

I suppose this is back to: design decision needed.

[Chris Beaven]

Well I'm glad we got to the same page :)

Yeah, I had that issue with using context.update() too when I started - could you open another ticket for the doc clarification please?

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Aymeric Augustin]

Re-qualifying as a doc fix since the reporter didn't open another ticket for that and the discussion has stalled.

[Tim Graham]

Closing given the lack of activity here and because I can't understand what documentation should be added.