Opened 5 years ago

Closed 3 years ago

#17795 closed Cleanup/optimization (wontfix)

kwargs not passed on by django.views.generic.edit import ProcessFormView

Reported by: ed.crewe@… Owned by: Adrien Lemaire
Component: Generic views Version: master
Severity: Normal Keywords:
Cc: lemaire.adrien@…, bmispelon@…, polmuz Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Jannis Leidel)

Although these do get attached to self.kwargs for use within the class, this causes problems for decorators of the get_context_data method.

kwargs which you may expect to be available for use by decorators you create are unset by this mixin.

NB: In my case this was related to testing user object permissions - hence the need to have the context data

Patching as below fixes it:

    A mixin that processes a form on POST.                                                                                                                                                
    """
    def get(self, request, *args, **kwargs):
        form_class = self.get_form_class()
        form = self.get_form(form_class)
--      return self.render_to_response(self.get_context_data(form=form))
++      kwargs['form'] = form
++      return self.render_to_response(self.get_context_data(**kwargs))

Attachments (3)

t17795.diff (5.9 KB) - added by Adrien Lemaire 5 years ago.
Add kwargs to context in all generic base classes get() + test
example_code.py (3.6 KB) - added by ed.crewe@… 5 years ago.
example decorator needing objects and monkeypatches to provide this
t17795.2.diff (12.9 KB) - added by Adrien Lemaire 5 years ago.
kwargs for get_context_data() result, and factory tests

Download all attachments as: .zip

Change History (24)

comment:1 Changed 5 years ago by anonymous

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Same issue in

dates.BaseDateListView.get
list.BaseListView.get
detail.BaseDetailView.get

comment:2 Changed 5 years ago by Jannis Leidel

Description: modified (diff)

Updated ticket description.

comment:3 Changed 5 years ago by Jannis Leidel

Description: modified (diff)

comment:4 Changed 5 years ago by Jannis Leidel

Triage Stage: UnreviewedAccepted

comment:5 Changed 5 years ago by Adrien Lemaire

Cc: lemaire.adrien@… added
Owner: changed from nobody to Adrien Lemaire

comment:6 Changed 5 years ago by Adrien Lemaire

Has patch: set
Needs tests: set

Changed 5 years ago by Adrien Lemaire

Attachment: t17795.diff added

Add kwargs to context in all generic base classes get() + test

comment:7 Changed 5 years ago by Adrien Lemaire

Needs tests: unset
Patch needs improvement: set

@ed.crewe , I couldn't reproduce your problem, I don't understand how you're testing your user object permissions. I always do it through get_object or dispatch, and didn't get such problem.

I wrote a test that does basically the same thing, by adding a kwarg in an overrided get() instead of get_context_data, and insure that this new kwarg isn't lost in ProcessFormView.get(). I don't like my test, so if you could give me your use case, I'll replace the test by a proper one.

I'm wondering if I should do the same with post ? Again, I need to understand the use case before doing so.

Changed 5 years ago by ed.crewe@…

Attachment: example_code.py added

example decorator needing objects and monkeypatches to provide this

comment:8 Changed 5 years ago by anonymous

Hi,

I have attached a file with the monkey patches I am using of class views - and an example decorator that demonstrates the issue.

So why it is desirable for get_context_data to include the views object(s) in the kwargs it is passed - rather than just getting them from the view attributes, ie. self.object - so that standard decorators for the context data method can be used which need access to a views object(s) or possibly other attributes.

In this case an object permissions test decorator.

Thanks,
Ed

comment:9 Changed 5 years ago by Adrien Lemaire

Patch needs improvement: unset

I think we're good now. I wrote responsible tests thanks to @acdha idea of using RequestFactory for that case.

I realize that there is no test for BaseDateListView, so I omitted that one, let me know if I should write tests for the archive views.

comment:10 Changed 5 years ago by Julien Phalip

Patch needs improvement: set

The patch looks good. If you could add tests for BaseDateListView that would be great. Also some minor comments:

  • Verify the actual value of the 'foo' context variables instead of just verifying their existence.
  • Instead of updating kwargs (e.g.: kwargs['form'] = form) before passing it to self.get_context_data(), use named arguments, i.e.: self.get_context_data(form=form, **kwargs), mainly for consistency with the rest of the codebase.

Changed 5 years ago by Adrien Lemaire

Attachment: t17795.2.diff added

kwargs for get_context_data() result, and factory tests

comment:11 Changed 5 years ago by Adrien Lemaire

Patch needs improvement: unset

Done. I just needed to write a test for one of DateBaseListView subclasses, I wrote one for ArchiveIndexViewTests.

actual value of 'foo' is verified.
BaseDetailView.get() now consistent with the rest of the code.

comment:12 Changed 5 years ago by Julien Phalip

Triage Stage: AcceptedReady for checkin

Great, thanks!

comment:13 Changed 3 years ago by Baptiste Mispelon

Cc: bmispelon@… added

For reference, commit f04bb6d798b07aa5e7c1d99d700fa6ddc7d39e62 added this behavior to TemplateView.

I'm wondering if it'd be a good idea to just add this behavior in ContextMixin instead of re-implementing it in each view.

comment:14 Changed 3 years ago by Tim Graham

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

I'm not sure if this requires any changes per the last comment, but the patch no longer applies cleanly.

comment:15 Changed 3 years ago by polmuz

Cc: polmuz added
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin
Version: 1.4-beta-1master

Hi, I created a PR that merges cleanly using the patch provided. I think it's ready for checking again given that adding the behavior in ContextMixin doesn't seem to be straightforward.

https://github.com/django/django/pull/1631

comment:16 Changed 3 years ago by Tim Graham

Needs documentation: set
Triage Stage: Ready for checkinAccepted

This behavior change needs a mention in the release notes and probably the docs themselves.

comment:17 Changed 3 years ago by Marc Tamlyn

I want to -1 this change. URL keyword arguments are not context data. IMO the behaviour of TemplateView is wrong here, and has bitten me before where I've had to change my urlconf.

For a concrete exple of why this is bad, consider the situation where I have a URL pointing to a form view which renders different forms depending on the url. Then I call that URL kwarg 'form' and this code breaks.

The correct place to add things to the context is in get context data, not by arbitrarily adding all URL kwargs to the context in all views.

This patch would break existing running code for me.

As an aside, note that view is put into the context now, so if you really want to you can access the URL kwargs as view.kwargs

comment:18 Changed 3 years ago by Marc Tamlyn

This is related to #19878

comment:19 Changed 3 years ago by loic84

@mjtamlyn: +1, it bit me too. It's a problem with TemplateView which if I remember well, did that for backward compat with the old direct_to_template.

It'd be great to deprecate that...

comment:20 Changed 3 years ago by Marc Tamlyn

That's what #19878 is about. the details of the deprecation are tricky.

comment:21 Changed 3 years ago by Tim Graham

Resolution: wontfix
Status: newclosed

Marking as won't fix given the backwards compatibility concerns.

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