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 <[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.
