Opened 4 years ago

Last modified 22 months ago

#18105 new Cleanup/optimization

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 4 years ago.
Do not accept context instance as second render_to_response parameter
18105-2.diff (3.9 KB) - added by Claude Paroz 4 years ago.
Properly construct context when inclusion tag returns a context

Download all attachments as: .zip

Change History (7)

comment:1 Changed 4 years ago by Claude Paroz

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 Changed 4 years ago by Aymeric Augustin

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

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

Changed 4 years ago by Claude Paroz

Attachment: 18105-1.diff added

Do not accept context instance as second render_to_response parameter

Changed 4 years ago by Claude Paroz

Attachment: 18105-2.diff added

Properly construct context when inclusion tag returns a context

comment:3 Changed 4 years ago by Claude Paroz

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 Changed 2 years ago by Tim Graham

Patch needs improvement: set

comment:5 Changed 22 months ago by Aymeric Augustin

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

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