Hi Luke, I'm also skeptical about validation when: - it tries to analyze statically dynamic behaviors; - it fails on code that actually works.
To me it looks like the admin outgrew its own validation, and as a consequence the validation needs to be re-engineered. Christopher Medrela applied for a GSoC on validation. While his proposal doesn't address this specific problem, I think it would make it easier to resolve. Overall, when it comes to correctness vs. convenience, I sit on the side of correctness :) so count me as +1 if you're changing validation to stop assuming static behavior that isn't guaranteed. All this is fairly hand-vawy, because my only exposure to the validation code until now is as a user, not as a developer. However, I'm willing to dive in and provide more concrete feedback on a patch. -- Aymeric. On 2 mai 2013, at 00:31, Luke Plant <l.plant...@cantab.net> 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 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. > > -- 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.
