Code

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#16074 closed Bug (fixed)

Class-based views clash get_context_data

Reported by: emyller Owned by: tobias
Component: Generic views Version: master
Severity: Normal Keywords: cbv
Cc: eMyller@…, carlospalol, reinout, dbrgn 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)

16074-1.diff (10.5 KB) - added by claudep 2 years ago.
Pull request + minor changes

Download all attachments as: .zip

Change History (20)

comment:1 Changed 3 years ago by lukeplant

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Getting views to modify a context_data dictionary instead of using get_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 call super().

So the best fix is to add the appropriate super() calls. If we fix that, we have the problem that object doesn't have get_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 every get_context_data implementation calls super(), but if it can't be sure that it has a superclass with get_context_data defined, it does this (using FormMixin as an example):

class FormMixin(object):
    def get_context_data(self, **kwargs):
        try:
            m = super(FormMixin, self).get_context_data
        except AttributeError:
            return kwargs
        d = m()
        d.update(kwargs)
        return d

comment:2 Changed 3 years ago by emyller

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 Changed 3 years ago by lukeplant

@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 Changed 3 years ago by lukeplant

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:

  1. It doesn't get into problems calling get_context_data on object.
  2. 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:5 Changed 3 years ago by emyller

Good idea, seems to be pretty straightforward.

comment:6 Changed 3 years ago by carlospalol

  • Cc carlospalol added
  • UI/UX unset

comment:7 Changed 3 years ago by ptone

Ref dupe: #16931

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

comment:8 Changed 2 years ago by ptone

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

  • Owner changed from nobody to tobias
  • Status changed from new to assigned

comment:10 Changed 2 years ago by ptone

Note this closely related issue: #17242

comment:11 Changed 2 years ago by reinout

  • Cc reinout added

comment:12 Changed 2 years ago by ptone

  • Has patch set
  • Version changed from 1.3 to SVN

comment:13 Changed 2 years ago by reinout

The version changed from 1.3 to svn, but shouldn't that be 1.4? #16744, which was supposed to go into 1.4 iirc, is waiting for this ticket to be fixed.

I kinda like #16744 to be in 1.4, as that makes explaining class based views much easier and clearer in that new django book that I'm writing :-)

comment:14 Changed 2 years ago by anonymous

  • Triage Stage changed from Accepted to Ready for checkin

comment:15 Changed 2 years ago by dbrgn

  • Cc dbrgn 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.

Changed 2 years ago by claudep

Pull request + minor changes

comment:16 Changed 2 years ago by claudep

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

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:18 Changed 2 years ago by claudep

  • Resolution set to fixed
  • Status changed from assigned to closed

In [17875]:

Fixed #16074 -- Added ContextMixin to class-based generic views to handle get_context_data. Thanks emyller, Luke Plant, Preston Holmes for working on the ticket and patch.

comment:19 Changed 2 years ago by brutasse

The commit has __future__ imports which are not needed anymore since trunk is python2.6+.

EDIT: Actually, 2.6 only raises a DeprecationWarning so I'm not sure my comment applies. Sorry for the noise.

Last edited 2 years ago by brutasse (previous) (diff)

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.