Hi Luke,
The static validation also breaks some third party apps like
django-hvad[0] and makes it very ugly to work around[1]. I agree with
removing it altogether, or at least make it simple to disable it when
using apps that change the form dynamically. Regards,
Iván
[0] https://github.com/KristianOellegaard/django-hvad
[1] https://github.com/KristianOellegaard/django-hvad/issues/10
On Wed, May 1, 2013 at 7:31 PM, Luke Plant <[email protected]> wrote:
> 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 [email protected].
> To post to this group, send email to [email protected].
> Visit this group at http://groups.google.com/group/django-developers?hl=en.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
--
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 [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/django-developers?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.
