Code

Opened 2 years ago

Closed 10 months ago

#17795 closed Cleanup/optimization (wontfix)

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

Reported by: ed.crewe@… Owned by: Fandekasp
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 jezdez)

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 Fandekasp 2 years ago.
Add kwargs to context in all generic base classes get() + test
example_code.py (3.6 KB) - added by ed.crewe@… 2 years ago.
example decorator needing objects and monkeypatches to provide this
t17795.2.diff (12.9 KB) - added by Fandekasp 2 years ago.
kwargs for get_context_data() result, and factory tests

Download all attachments as: .zip

Change History (24)

comment:1 Changed 2 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 2 years ago by jezdez

  • Description modified (diff)

Updated ticket description.

comment:3 Changed 2 years ago by jezdez

  • Description modified (diff)

comment:4 Changed 2 years ago by jezdez

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 2 years ago by Fandekasp

  • Cc lemaire.adrien@… added
  • Owner changed from nobody to Fandekasp

comment:6 Changed 2 years ago by Fandekasp

  • Has patch set
  • Needs tests set

Changed 2 years ago by Fandekasp

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

comment:7 Changed 2 years ago by Fandekasp

  • 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 2 years ago by ed.crewe@…

example decorator needing objects and monkeypatches to provide this

comment:8 Changed 2 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 2 years ago by Fandekasp

  • 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 2 years ago by julien

  • 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 2 years ago by Fandekasp

kwargs for get_context_data() result, and factory tests

comment:11 Changed 2 years ago by Fandekasp

  • 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 2 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

Great, thanks!

comment:13 Changed 15 months ago by bmispelon

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

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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

comment:15 Changed 10 months ago by polmuz

  • Cc polmuz added
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin
  • Version changed from 1.4-beta-1 to master

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

  • Needs documentation set
  • Triage Stage changed from Ready for checkin to Accepted

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

comment:17 Changed 10 months ago by mjtamlyn

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 10 months ago by mjtamlyn

This is related to #19878

comment:19 Changed 10 months 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 10 months ago by mjtamlyn

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

comment:21 Changed 10 months ago by timo

  • Resolution set to wontfix
  • Status changed from new to closed

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

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.