Opened 6 years ago

Last modified 2 years ago

#11234 assigned Cleanup/optimization

BlockNode unsafely manages context

Reported by: brutimus Owned by: brutimus
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

20090529-block-scope.diff (544 bytes) - added by brutimus 6 years ago.
First patch

Download all attachments as: .zip

Change History (12)

Changed 6 years ago by brutimus

First patch

comment:1 Changed 6 years ago by brutimus

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 6 years ago by ramiro

  • Needs tests set

comment:3 Changed 6 years ago by SmileyChris

  • Triage Stage changed from Unreviewed to Design decision needed

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.

comment:4 Changed 6 years ago by brutimus

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.

comment:5 Changed 6 years ago by SmileyChris

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].

comment:6 Changed 6 years ago by brutimus

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.

comment:7 Changed 6 years ago by SmileyChris

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?

comment:8 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:9 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:11 Changed 2 years ago by aaugustin

  • Component changed from Template system to Documentation
  • Has patch unset
  • Needs tests unset
  • Triage Stage changed from Design decision needed to Accepted

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

Note: See TracTickets for help on using tickets.
Back to Top