ModelForm inheritance logic is broken

[Ilya Semenov]

A simple example:

class TicketForm(forms.ModelForm):
  class Meta:
    model = Ticket
    fields = ('priority','summary')

class EditTicketForm(TicketForm):
  pass

Supposedly, the program behaviour generally should not change if TicketForm usage is replaced with EditTicketForm. However, it changes in a very unexpected way (instance saving still works fine, but the "fields" filter is gone!). Actually it brings some more "gotchas", but they're harder to explain with a simple example.

When I looked closer, I discovered that django.newforms.models.ModelFormMetaclass is not very well thought when it comes to the inheritance.

First, regarding the simple problem mentioned above, it says:

class ModelFormMetaclass(type):
  ...
  opts = ModelFormOptions(attrs.get('Meta', None))
  attrs['_meta'] = opts

It can be easily seen that for EditTicketForm, as there's no Meta in it, opts becomes "empty" ModelFormOptions with model=None, fields=None, and exclude=None. Therefore, the further save() call doesn't use proper "fields" filter, since it is lost in the parent class'es Meta.

Another related issue is that ModelFormMetaclass (according to the code and comments) is supposed to allow only one base class with the defined Meta attribute. I can see the rationale behind this limitation - it wouldn't be easy to deal with several Metas with contradictory fields= or exclude=. However, the current implementation is broken in that case, as it actually allows to have two Metas (one in the base classes, and one in the child class), with unpredictable results. We need either to disallow that completely, or come up with some logic which handles these cases accurately.

The proposed patch fixes the arisen problems. With inherited Metas, the following algorithm is imposed:

• If a child class defines a Meta class, it is taken as an argument to ModelFormOptions.
  
• If a child class doesn't define a Meta, it is taken from its parents. The parents are obliged to define no more than one Meta.
• If neither a child class nor its parents define any Meta class, an "empty" ModelFormOptions is created.

Also, I extracted a function from DeclarativeFieldsMetaclass to reuse it (and get rid of copy-pasted code) in ModelFormMetaclass.

The patch includes documentation describing the proposed behaviour (previously, the model form inheritance was not covered in the documentation at all).

[Brian Rosner]

I agree with these points. In my patch on #6241 I was running into a similar
problem and just worked around it. The ModelFormMetaclass` needs to be
made more flexible with the Meta class. I also have a use-case where the meta
options are needed to be passed into the __new__ method so I would
take that into consideration with how you are building the options.

I would also suggest as opposed to using get_declared_fields that
ModelFormMetaclass just inherits from DeclarativeFieldsMetaclass.

This patch will also need tests to ensure this functionality.

Being the maintainer of the #6241 patch work that this should be looked at
and resolved. Thanks for submitting the patch semenov!

[Ilya Semenov]

The ModelFormMetaclass needs to be made more flexible with the Meta class.

I must be missing something. Do you suggest having a Meta class for a Meta class? Sounds overcomplicated. What problem does that suppose to solve?

You see, I don't think we should cover several ModelForm's problem with a single ticket. That gives smaller chance for it to get to the trunk :)

I described a specific problem with inheritence and solved it with a proper patch which doesn't bring much complexity to the code (actually I think the code became simplier).

I would also suggest as opposed to using get_declared_fields that ModelFormMetaclass just inherits from DeclarativeFieldsMetaclass.

I don't think it's possible to just inherit DeclarativeFieldsMetaclass, since __new__ method there is static (and there's no self to access the parent).

This patch will also need tests to ensure this functionality.

Sure, will add the tests.

[Chris Beaven]

I'll trust brosner that this is a real issue.

[Malcolm Tredinnick]

#6241 really is a separate issue to this patch and maybe it will add further changes or maybe not. In any case, this is a problem now, regardless of formsets and the patch here is a reasonable holistic approach to fixing the problem. I'm going to apply a variation on this patch for now and we can thrash out formsets in the other ticket as an independent issue. It's not worth blocking this for the other ticket at the moment.

[Malcolm Tredinnick]

(In [7112]) Fixed #6337. Refs #3632 -- Fixed ModelForms subclassing, to the extent that it can be made to work.

This ended up being an almost complete rewrite of ModelForms.new, but
should be backwards compatible (although the text of one error message has
changed, which is only user visible and only if you pass in invalid code).

Documentation updated, also.

This started out as a patch from semenov (many thanks!), but by the time all
the problems were hammered out, little of the original was left. Still, it was
a good starting point.

[Malcolm Tredinnick]

This was further modified in [7115], but I forgot to include the ticket reference in the commit message. Inheritance now behaves "naturally" when Meta is subclassed.