#16074 closed Bug (fixed)
Class-based views clash get_context_data
Reported by: | Evandro Myller | Owned by: | Tobias McNulty |
---|---|---|---|
Component: | Generic views | Version: | dev |
Severity: | Normal | Keywords: | cbv |
Cc: | eMyller@…, Carlos Palol, Reinout van Rees, Danilo Bargen | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
One of the best advantages of class-based views is the possibility to use "mixins". So now I'm trying to have a view that lists some database objects and puts an ordinary search form in the page. All I expect to do is the following:
class MyView(ListView, FormView): model = Model template_name = 'my_template.html' paginate_by = 12 form_class = SearchForm
But the way Django generic views collects data for the context completely misses such goal. We have .get_context_data (among other methods) clashing, so we never get both form and object_list keys.
One suggestion I give is to let the views feed a self.context_data
dict, so it could be far easier to access/modify the context and we wouldn't have such problem.
Reference: https://convore.com/django-community/django-13s-get_context_data/
Attachments (1)
Change History (20)
comment:1 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 14 years ago
Does "every get_context_data
implementation" point to the ones in the CBV's core only? If so, that'd be a pretty clever solution. There could also be a base class -- I think -- with the code above as a _base_get_context_data
-like method.
comment:3 by , 14 years ago
@emyller - I mean every CBV class that Django has (which needs it i.e. which can't call super at the moment because it is not sure there is a super), because those are the only ones we have control over. In theory, other people might have to apply the same fix to their mixins. This is a basic problem with inheritance in Python, and not something we can fix in general.
A base class for just this fix seems like overkill. Yes, it would reduce code duplication, but it would increase the number of layers, which can really make things harder to understand.
comment:4 by , 14 years ago
Having looked at this article, I think a better way to do this is to add the super()
calls, but for every class that defines get_context_data
and currently inherits from object
, make it inherit from a base class like this instead:
class GetContextDataRoot(object): def get_context_data(self): return {}
It doesn't actually have to be the same base class in each case, it just needs this method, and no other functionality that might be wanted for other reasons. The existence of a base class like this means that:
- It doesn't get into problems calling
get_context_data
on object. - The use of
super()
means that other classes can be inserted into the MRO, so no data gets lost.
So, if we have SubClass1
and SubClass2
that both inherit from GetContextDataRoot
(or an equivalent class), and use super(), and then I write my own that inherits from both SubClass1
and SubClass2
, I get an MRO like this:
MyClass
-> SubClass1
-> SubClass2
-> GetContextDataRoot
Which is what I want.
comment:6 by , 13 years ago
Cc: | added |
---|---|
UI/UX: | unset |
comment:8 by , 13 years ago
note that once a root object is in place to handle get_context_data for any inheritence tree, it should be easy to then fix #16744 in the base method
comment:9 by , 13 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:11 by , 13 years ago
Cc: | added |
---|
comment:12 by , 13 years ago
Has patch: | set |
---|---|
Version: | 1.3 → SVN |
comment:13 by , 13 years ago
comment:14 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:15 by , 13 years ago
Cc: | added |
---|
This is an important change, many mixin implementations might depend on this in the future. If this does not go into 1.4, it should be in 1.5 for sure.
comment:16 by , 13 years ago
Apart from the patch from the pull request, I had to modify the get_context_data of CustomTemplateView in tests/regressiontests/generic_views/views.py
to make the tests pass. But I think it was weirdly coded anyway.
Last call before I commit the patch.
comment:17 by , 13 years ago
get_context_data in TemplateView (django/views/generic/base.py) can probably be removed as all it does is call super (copying SmileyChris's comment from the pull request).
comment:19 by , 13 years ago
The commit has __future__
imports which are not needed anymore since trunk is python2.6+.
Getting views to modify a
context_data
dictionary instead of usingget_context_data
would be a backwards incompatible change. And it still wouldn't fix the actual problem, which is that these methods that provide context data don't callsuper()
.So the best fix is to add the appropriate
super()
calls. If we fix that, we have the problem thatobject
doesn't haveget_context_data
so will barf. Given the hierarchies that we have (with mixins that don't inherit from View), the only solution I can see is that everyget_context_data
implementation callssuper()
, but if it can't be sure that it has a superclass withget_context_data
defined, it does this (usingFormMixin
as an example):