Use ModelForms in ModelAdmin. Refactor FormSets to work like ModelForms.

[Brian Rosner]

This ticket really is two things, but they go hand in hand.

1. We need to get ModelAdmin working with the ModelForm class. This will require some helper function to dynamically create a ModelForm class object at run-time.
2. Refactor FormSet, BaseFormSet and InlineFormset to work like its ModelForm buddy.

With the correct refactoring of the formset code it will fix several other bugs found in newforms-admin.

[Brian Rosner]

use ModelForm in ModelAdmin. refactors formset code.

[Brian Rosner]

The provide patch makes the FormSet class and its children work in a declarative way. I have introduced a new function named modelform_for_model which dynamically creates a ModelForm class. I believe that should be renamed to the current form_for_model and continue to depercate the form_for_instance helper function. But that can all be worked out later.

This patch also fixes inline file fields. I am also marking patch needs improvement and tests since I haven't 100% finish tests and there are some few corner cases that need to be looked at again.

[Malcolm Tredinnick]

From a quick read, I don't think modelform_for_model() should be exposed publicly (bury it in django.contrib.admin if it's needed). The idea is to move away from form_for_*() because everybody was wanting to add their own lever and button to customise them. Adding yet another method of this variety isn't going to help.

Normal "public" use-case is going to be creating a form for a specific model, so people should be using ModelForm directly. Admin is somewhat special in requiring dynamic form creation for unknown models, so it might need its own helper function, but don't leak that into the public API.

[Brian Rosner]

Oh ok, that is a good point. I wasn't 100% sure whether or not these dynamic creation function should be publicly accessible. If anything not documented. Would be okay to just remove them from the __all__ tuple to prevent them at the django.newforms namespace? I don't think hiding them in the admin application would be best since the newforms version of the create/update generic views would then have that dependancy. See #3639.

[Øyvind Saltvik <oyvind@…>]

attempt to update patch after merge with trunk

[Brian Rosner]

updated to r6955 (this does not include any new changes since my last patch)

[Øyvind Saltvik <oyvind@…>]

Slight change to BaseModelFormSet to allow queryset as a kwarg

[Brian Rosner]

includes fixes recommended by malcolm, joseph and karen. also some various cleanups.

[Brian Rosner]

removed the index parameter to get_form_class replaced with change=False

[Malcolm Tredinnick]

I've just read through this and a lot of flags go up. I'm not really loving the design: it smells too complicated. I'm just writing down my first thoughts, mostly for Joseph's benefit (feel free to go whichever way you like, though, Joseph; it's your baby). Trying to predict the future, this doesn't feel very maintainable (or, rather, it feels like only a few people are ever going to motivated enough to understand it to maintain it).

There's no need to use warnings in any of this code. It isn't in trunk yet. Anybody writing production code against the branch has to realise all bets are off as far as any kind of stability goes.

Do we really need Yet Another Metaclass(tm) here? Why not put the __new__ method on the formset class directly if it's really needed? Django seems to be going overboard with metaclasses at the moment, but apparently only because we can. There isn't really a huge use-case for it being replaced that can't be done just by class subclassing. Metaclasses make sense when they apply to multiple classes; they aren't intended as a holder for __new__ methods.

I'm not sure putting the num_extra attribute into _meta is a good plan. That hides it from the template. Anything in a leading underscore variable isn't intended to be accessed by public code, but the number of empty instances of an "edit inline" equivalent is useful information to have access to publically.

Replacing the ModelFormsOptions class feels wrong somehow, too. I suspect it should be possible to use it more directly. I think it's probably because trying to use a ModelForm as something like a FormSet -- which looks like what the code is trying to do -- doesn't sit well. A FormSet can be a collection of ModelForms (the main model's form, plus one for each of the related instances), but I don't think a FormSet "is a" ModelForm (or a variation on that). Trying to keep ModelForms corresponding to a single model is going to stop our brains leaking out our ears at a later date.

[Malcolm Tredinnick]

Brian, Can you please note in a comment here what the "other bugs" are that you are trying to fix here. So that we can check whatever final design we do end up with either fixes them or leaves them on the table.

[Brian Rosner]

Replying to mtredinnick:

I've just read through this and a lot of flags go up. I'm not really loving the design: it smells too complicated. I'm just writing down my first thoughts, mostly for Joseph's benefit (feel free to go whichever way you like, though, Joseph; it's your baby). Trying to predict the future, this doesn't feel very maintainable (or, rather, it feels like only a few people are ever going to motivated enough to understand it to maintain it).

There's no need to use warnings in any of this code. It isn't in trunk yet. Anybody writing production code against the branch has to realise all bets are off as far as any kind of stability goes.

Makes sense. Have noted to remove it.

Do we really need Yet Another Metaclass(tm) here? Why not put the __new__ method on the formset class directly if it's really needed? Django seems to be going overboard with metaclasses at the moment, but apparently only because we can. There isn't really a huge use-case for it being replaced that can't be done just by class subclassing. Metaclasses make sense when they apply to multiple classes; they aren't intended as a holder for __new__ methods.

I do tend to feel similar, but I did it this to make formsets work like the Form and ModelForm class with the declarative syntax for defining fields. This was something that Joseph and I talked about and seemed to be the consensus of our discussions. InlineFormset has a decent use-case for a metaclass which would allow for generic relations to define its own metaclass to perform generic foreign key resolution.

I'm not sure putting the num_extra attribute into _meta is a good plan. That hides it from the template. Anything in a leading underscore variable isn't intended to be accessed by public code, but the number of empty instances of an "edit inline" equivalent is useful information to have access to publically.

This was done to follow the convention of how all the other metaclass type classes worked with options relating to itself. This would have to be made publicly usable similar to how orderable and deletable are accessible. It was never intended that the user would being using _meta directly in the template due to its underscore convention.

Replacing the ModelFormsOptions class feels wrong somehow, too. I suspect it should be possible to use it more directly. I think it's probably because trying to use a ModelForm as something like a FormSet -- which looks like what the code is trying to do -- doesn't sit well. A FormSet can be a collection of ModelForms (the main model's form, plus one for each of the related instances), but I don't think a FormSet "is a" ModelForm (or a variation on that). Trying to keep ModelForms corresponding to a single model is going to stop our brains leaking out our ears at a later date.

Not sure what you mean by replacing it? It was moved to options.py and subclassed by the other option classes since they shared common options. Can you explain a bit more about what you mean by using a ModelForm like a FormSet?

To answer the "other bugs" here is what I have in mind that my patch doesn't seem to address well enough.

1. The use of form in the FormSetOptions is slightly confusing, but has a reason. It is used to define the form that the FormSet class uses. I think this needs to be addressed somehow. Not sure if this is something that needs to be fixed up in FormSet or what.

2. Customizing the form used in either the ModelAdmin or InlineModelAdmin still seems very hairy. Right now with my patch there isn't much in the way really being able to do that nicely.

3. In writing this patch it become apparent the inheritance used for generating the ModelForm class is slightly flawed. See #6337 for more information.

[jkocherhans]

Updated the patch to work with r7125. Still a couple of changes I'd like to make, but this is as far as I could get tonight.

[jkocherhans]

Ugh. I can't even keep my head wrapped around this code most of the time (not just the patch, but all the model-formset integration stuff in general). It feels like django.contrib.admin.options felt a few months ago (only this mess is mostly of my own creation) :). I'm going to take a step back and try to think about this with fewer assumptions.

[jkocherhans]

Fixed in [7270].