Opened 12 years ago

Closed 21 months ago

#18105 closed Cleanup/optimization (fixed)

Investigate possible misuse of Context

Reported by: Aymeric Augustin Owned by: nobody
Component: Template system Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

This was reported in the comments of #17229, but I'm moving it to its own ticket for clarity.


Changeset r17894 produced several errors in the Django test suite. In fact, it reveals issues where Context instances are initialized with a Context argument instead of a simple dict.
Examples:

  • In browser:django/trunk/django/contrib/auth/tests/urls.py, several render_to_response calls have the Context instance as second argument, which seems to be plain bugs (uncaught until now)
  • In browser:django/trunk/django/contrib/admin/templatetags/admin_modify.py, the prepopulated_fields_js inclusion tag returns a Context instance instead of a dict.

The question is now whether Context initialization should accept other Context instance as the __init__ dict_ parameter (and then this changeset should be fixed) or if we should enforce dict_ as a dict and fix places in Django code when it is not the case. I'd be in favour of the latter, but this might need some more investigation.

Attachments (2)

18105-1.diff (4.0 KB ) - added by Claude Paroz 12 years ago.
Do not accept context instance as second render_to_response parameter
18105-2.diff (3.9 KB ) - added by Claude Paroz 12 years ago.
Properly construct context when inclusion tag returns a context

Download all attachments as: .zip

Change History (8)

comment:1 by Claude Paroz, 12 years ago

Component: UncategorizedTemplate system
Type: UncategorizedCleanup/optimization

After re-reading the docs about Context, I think things are clear: Context takes a dict as optional init parameter. So for me, passing a Context to another Context init is a mistake and should be fixed (see my patch on #18103).

comment:2 by Aymeric Augustin, 12 years ago

I'm in favor of this change. We may need a deprecation path, though.

Last edited 12 years ago by Aymeric Augustin (previous) (diff)

by Claude Paroz, 12 years ago

Attachment: 18105-1.diff added

Do not accept context instance as second render_to_response parameter

by Claude Paroz, 12 years ago

Attachment: 18105-2.diff added

Properly construct context when inclusion tag returns a context

comment:3 by Claude Paroz, 12 years ago

Has patch: set
Triage Stage: UnreviewedAccepted

I have separated in two distinct patches the problematic uses of Context initialization from a context instance in Django code.

At this point and considering the correction you committed in r17896, it's still possible to intialize a Context from another Context, even with these patches.

comment:4 by Tim Graham, 10 years ago

Patch needs improvement: set

comment:5 by Aymeric Augustin, 9 years ago

733178830072caeca3c054a220808b4c557faec4 changed render_to_response so it wouldn't rewrap a Context inadvertently passed in the dictionary argument in another Context. That's the opposite of the route taken by first patch above and I'm convinced it's a better approach given how widespread render_to_response is and how easy forgetting context_instance= is.

The second patch still warrants consideration. I had a look and I don't think the code inside if isinstance(_dict, BaseContext) is correct: it adds values such as autoescape and current_app to the context when they're intended to be attributes of the context object.

Last edited 7 years ago by Tim Graham (previous) (diff)

comment:6 by Mariusz Felisiak, 21 months ago

Resolution: fixed
Status: newclosed

As far as I'm aware the second part was fixed by e53495ba3352c2c0fdb6178f2b333c30cb6b5d46.

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