Hi all,

Can I get some feedback on what to do about ticket 19445?

https://code.djangoproject.com/ticket/19445

I have re-opened this ticket because I don't think it was addressed
satisfactorily. I apologise in advance that this is a bit involved!

The fundamental problem is that the validation routines attempt to do
static validation of a ModelAdmin class, but ModelAdmin has gained
methods that mean that many of the things that we want to validate are
only generated at run time (by which I mean the point when a request is
received).

So ModelAdmin.get_form() is the method used to create a form class, but
the validation routines want to validate ModelAdmin.form . Even without
subclassing ModelAdmin.get_form(), it's possible to create a form class
that will not pass validation, but will work fine in practice (see
ticket for more details).

The main reason I'm bothered about this is ticket 19733, which is about
requiring the ModelForm.Meta.fields attribute to be present. Please see
below for why 19733 is involved.

Even without #19733, it seems that validation of ModelAdmin.form is
broken in the general case. The attempts to fix it so far were quite hacky:

https://github.com/django/django/commit/0694d2196f0fadde37ff2d002a9a4a8edb3ca504

And I'm fairly sure there are other bugs with validation related to the
dynamic methods such as get_fieldsets() and get_readonly_fields(), which
both take a request object.

Since we can't do static validation properly, I'm tempted to remove it
altogether, but I imagine the static validation will catch lots of
problems, which is pretty helpful for newbies. A large part of the
ModelAdmin validation would need to be removed - everything that depends
on knowing what fields are going to be present.

One possibility is to remove the static validation, but where specific
errors are common (e.g. misnamed fields), we try to make the error
handling further down the line much more friendly.

If you read the current validation routines, there are quite a few
places where potential errors have to be ignored (e.g. a field that
doesn't exist on the model is referred to), because there are other
things which could mean that it is actually correct. To me this implies
that validation is happening at the wrong point.

~~~~~~~~~~~~

Why it matters for #19733:

https://code.djangoproject.com/ticket/19733

#19733 will (eventually) require ModelForms to have Meta.fields
explicitly set. (Or, alternatively, Meta.exclude, but this is discouraged).

To cover the case where we really do want all fields, you can do 'fields
= "__all__"', as discussed previously on django-devs.

However, if you are creating a ModelForm to be used solely in the admin,
this is not good. We've agreed that that admin should use all fields by
default, and it has its own ways of specifying which fields to use.
Having to specify anything about fields on the custom ModelForm is
confusing.

There is one good solution I've found: specify a ModelForm without a
Meta inner class, or at least without a Meta.model attribute.

This already works fine with the admin. The ModelAdmin already knows the
model to use, and happily constructs the Meta inner class for your form
if it is missing.

This is a really good solution, because:

1) It's DRY - you specify everything once:

###########
class MyModel(models.Model):
    field1 = models.BooleanField()

class MyForm(forms.ModelForm):
    def clean(self, *args):
        # ...

class MyAdmin(admin.ModelAdmin):
    form = MyForm
    fieldsets = (
        ('Group', {
              'fields': ['field1']
          }),
    )

admin.site.register(MyAdmin, MyModel)
###########


2) It is secure.

If we do 'MyForm.Meta.fields = "__all__"' just to satisfy ModelForm,
then not only is it confusing, we've also created a functioning
ModelForm that could end up being used outside the admin, which has all
fields included and is therefore potentially insecure. We want to avoid
that.

3) It provides a simple upgrade path.for people who hit the new
requirement on ModelForm, where they were only using them for ModelAdmin.

Instead of asking people to add a fields attribute, we tell them to
remove the whole Meta inner class, or at least the model attribute. This
can be in the upgrade notes.

4) It works well in terms of implementation.

The admin uses all the ModelForm machinery in terms of location Model
fields. However, we need ModelForms to have the opposite behaviour from
the admin in this respect - ModelForms should not use all fields unless
explicitly told to.

This means that the obvious place to patch up the mismatch is in the
admin implementation - it should create an appropriate ModelForm.Meta
class for you, as it does currently.


There is, in fact, already a ModelAdmin in the test suite that fails
validation if it is updated according to the method above, which is what
brought me to ticket 19445.


~~~~~~~~~~~~~~


Thoughts?

Luke


-- 
CARLSON'S CONSOLATION
    Nothing is ever a complete failure; it can always serve as a
bad example.

Luke Plant || http://lukeplant.me.uk/

-- 
You received this message because you are subscribed to the Google Groups 
"Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to django-developers+unsubscr...@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.


Reply via email to