Opened 4 years ago

Closed 4 years ago

#25839 closed Bug (wontfix)

RequestContext does not apply context processors [regression]

Reported by: direx Owned by: nobody
Component: Documentation Version: 1.8
Severity: Normal Keywords: RequestContext, regression
Cc: Aymeric Augustin Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Prior to Django 1.8 django.template.RequestContext always had the context from all of the TEMPLATE_CONTEXT_PROCESSORS applied. In Django 1.8 this is no longer the case, which breaks the API. As this has neither been announced nor documented anywhere I'd consider this a critical regression.

The following view code no longer works with the default TEMPLATE_CONTEXT_PROCESSORS setting:

from django.template import RequestContext

def my_view(request):
    context = RequestContext(request)
    print context['user']
    #...

Actually user should be put there by django.contrib.auth.context_processors.auth. It is also present in templates rendered by render(), but not in RequestContext. Of course this does not only apply to the user context variable, but to all context variables set by context processors.

Attachments (1)

test_25839.py (685 bytes) - added by Tim Graham 4 years ago.

Download all attachments as: .zip

Change History (12)

comment:1 Changed 4 years ago by Tim Graham

Cc: Aymeric Augustin added
Triage Stage: UnreviewedAccepted

Bisected to 37505b6397058bcc3460f23d48a7de9641cd6ef0. The test I used to bisect is attached.

Changed 4 years ago by Tim Graham

Attachment: test_25839.py added

comment:2 Changed 4 years ago by Carl Meyer

As part of the multiple-template-engines feature, Django 1.8 refactored the Django template language to remove the direct coupling to global settings, instead making it possible to instantiate multiple independently-configured Engine instances (and configure multiple such instances via the new TEMPLATES setting). Because of this, it's no longer possible for RequestContext to automatically fetch the list of configured context processors from settings (the TEMPLATES setting can define multiple instances of the DTL with different lists of context processors, so how could RequestContext possibly know which list of context processors to use?). This was handled by adding a new context-manager method RequestContext.bind_template which temporarily binds a RequestContext to a template instance, in the process applying all the context processors from that template's engine configuration.

This solution is fully backwards-compatible for the only documented usage of a RequestContext, which is to pass it into a template render and access its values in the template. It's true that it's not backwards-compatible for the usage you demonstrate, directly accessing values from the context as a dictionary without rendering a template, but this is not a documented or supported mode of use for RequestContext. If you need this usage, you must now first bind the context to a specific template:

from django.template import engines

engine = engines['default']
tpl = engine.get_template('some/template.html')
ctx = RequestContext(request)
with ctx.bind_template(tpl):
    print(ctx['user'])

It may be possible to restore the old behavior for this usage using Engine.get_default() (it would only work if there's only a single instance of the DTL configured in settings); personally I don't think that's worth doing, or that this use of RequestContext should be supported.

Alternatively, we could add a warning to the 1.8 release notes about this, but this usage is so unusual that I'm not even sure that's worth doing.

comment:3 Changed 4 years ago by Aymeric Augustin

Severity: Release blockerNormal

Thanks Carl, you've explained the situation more clearly than I would have been able to. This is the intended behavior. It does not break the public API.

comment:4 Changed 4 years ago by direx

By saying that this is not part of the public API or not a supported usage of RequestContext you are making this a little too easy for you.

After looking at the Django 1.7 documentation these are facts:

  1. RequestContext is a special Context class. One of the differences „is that it automatically populates the context with a few variables, according to your TEMPLATE_CONTEXT_PROCESSORS setting.“ (quote from documentation)
  1. Another quote from the documentation: „Most of the time, you’ll instantiate Context objects by passing in a fully-populated dictionary to Context(). But you can add and delete items from a Context object once it’s been instantiated, too, using standard dictionary syntax“. Note the standard dictionary syntax here.

So I don't see why the behavior described in this bug should not be considered part of the public API. The usage as dictionary is actually even documented and encouraged (also in 1.8, section Playing with Context objects). So I'd still consider this a public API breakage.

comment:5 Changed 4 years ago by Aymeric Augustin

Can you submit a patch?

comment:6 Changed 4 years ago by Aymeric Augustin

(In case that's not clear enough, saying "you are making this a little too easy for you" to a person who works in their free time guarantees their non-cooperation. My involvement with this issue stops here.)

comment:7 Changed 4 years ago by Carl Meyer

Component: Template systemDocumentation

That's a good point; I looked through the RequestContext docs but didn't notice that dictionary-style access was documented higher up in the Context docs. I think that does make it a backwards-incompatible change to a public API that should have been documented in the release notes.

However, I still don't think it's a valuable enough usage to fix at this point. If we had run across this case while building multiple-template-engines, I think we'd have documented it as a minor backwards-incompatibility, not attempted to accommodate it -- the usage just doesn't belong in a multiple-template-engines world.

I'd be willing to look at a proposed patch, but I think a patch will need to introduce too much complexity. Specifically, a patch to reintroduce the old behavior would need to introduce a scheme for lazy-loading of the "default" context processors (that is, the ones configured for the "default" DTL instance -- which doesn't exist if there's more than one DTL instance configured) that takes place only if someone tries to dictionary-access a RequestContext that hasn't been bound to a template. This would be an entirely new code path that exists only to support this usage. I don't think that added complexity is justified by the limited-to-nonexistent need for this usage (note that the docs don't ever recommend it for any real purpose, just for "playing around" to understand Context objects).

So I think this ticket should be fixed the same way we'd have fixed it if we'd noticed it before the release of 1.8: by making a note of the issue in the backwards-incompatible changes section of the 1.8 release notes, and perhaps also clarifying the RequestContext docs to note that values from context processors are only available in a RequestContext once it has been bound to a template (which Django does automatically when you render a template using a RequestContext).

comment:8 Changed 4 years ago by direx

Replying to aaugustin:

(In case that's not clear enough, saying "you are making this a little too easy for you" to a person who works in their free time guarantees their non-cooperation. My involvement with this issue stops here.)

I don't get your point. I was neither rude, nor was my comment meant to be offensive. I am also reporting bugs in my free time, hoping to help other users who might stumble upon the same issue. Sorry if you got me wrong there.

Replying to carljm:

So I think this ticket should be fixed the same way we'd have fixed it if we'd noticed it before the release of 1.8: by making a note of the issue in the backwards-incompatible changes section of the 1.8 release notes, and perhaps also clarifying the RequestContext docs to note that values from context processors are only available in a RequestContext once it has been bound to a template (which Django does automatically when you render a template using a RequestContext).

To me this entire issue is actually not a biggie. Some third-party apps I am using rely on that behavior, but I can fix that. So I'd agree with you here that adding it to the list of backwards-incompatible changes and maybe a small note in the docs would probably be sufficient.

comment:9 Changed 4 years ago by Aymeric Augustin

It took me on the order of magnitude of one week of work to come up with the patch discussed here in order to maximize backwards compatibility — most likely because I'm a poor developer. Regardless I didn't try to cut corners and this was certainly not "easy for me". I'm sorry it didn't work for you.

comment:10 Changed 4 years ago by Carl Meyer

@direx Thanks for reporting the issue, and following up with your observations about what behaviors were documented! I'm glad we're agreed on the best path forward.

FWIW I didn't take the "too easy" comment personally on an individual level, but rather on the project level, which includes everyone commenting here. As a project, the initial response here was to disavow any responsibility at all for this change on a backwards-compatibility front, and I do indeed (in retrospect) think that was a bit "making it too easy," purely in a what-do-we-need-to-do-now-about-this-issue-as-a-project sense. Fortunately I don't think the documentation-based resolution is "making it too hard" either :-) I certainly didn't see the comment as a criticism of the (large amount of excellent) work that went into the initial multiple-template-engines change; unintentional minor regressions happen, even with much smaller feature additions than that one.

@direx, I'm curious if you could point me to the third-party app(s) making use of this behavior? I doubt it will change my mind on what we should do, but I am curious to see how they found it useful.

comment:11 Changed 4 years ago by Tim Graham

Resolution: wontfix
Status: newclosed

As 1.8 is nearly a year old and we only have a single report of the issue, I say it's not worth documenting unless someone offers a patch. If so, please reopen. Thanks!

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