Opened 3 years ago

Last modified 8 months ago

#18105 new Cleanup/optimization

Investigate possible misuse of Context

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

Download all attachments as: .zip

Change History (7)

comment:1 Changed 3 years ago by claudep

  • Component changed from Uncategorized to Template system
  • Type changed from Uncategorized to Cleanup/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 3 years ago by aaugustin

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

Last edited 3 years ago by aaugustin (previous) (diff)

Changed 3 years ago by claudep

Do not accept context instance as second render_to_response parameter

Changed 3 years ago by claudep

Properly construct context when inclusion tag returns a context

comment:3 Changed 3 years ago by claudep

  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted

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 14 months ago by timo

  • Patch needs improvement set

comment:5 Changed 8 months ago by aaugustin

[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