Opened 15 years ago

Closed 10 years ago

Last modified 8 years ago

#8898 closed Bug (fixed)

`required` validation bypassed when using `DateTimeField` with `SplitDateTimeWidget`.

Reported by: Tai Lee Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords: DateTimeField SplitDateTimeWidget
Cc: real.human@…, jarek.zgoda@…, doza@…, gertvangool@…, jgelens@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

DateTimeField.clean calls Field.clean for the required part of its validation, but Field.clean doesn't understand multiple values, and so DateTimeField.clean continues with the assumption that a value has been provided, which raises an invalid error incorrectly when the DateTimeField is *not* required and no values have been provided. This also means that an invalid error is raised instead of a required error when the DateTimeField *is* required and no values have been provided.

Attachments (2)

8898-DateTimeField-r8966.diff (2.0 KB) - added by Tai Lee 15 years ago.
8898-DateTimeField-r8983.diff (2.1 KB) - added by Tai Lee 15 years ago.
Update tests to match None return value.

Download all attachments as: .zip

Change History (35)

Changed 15 years ago by Tai Lee

comment:1 Changed 15 years ago by Tai Lee

The attachment above includes tests for both cases (required and not required) which fail before the patch is applied, and pass after it is applied.

comment:2 Changed 15 years ago by Tai Lee

Summary: `required` validation bypassed when using `SplitDateTimeWidget`.`required` validation bypassed when using `DateTimeField` with `SplitDateTimeWidget`.

comment:3 Changed 15 years ago by Tai Lee

Triage Stage: UnreviewedDesign decision needed

After some further digging and discussion in #django-dev, I believe that the validation in DateTimeField.clean which is specific for SplitDateTimeWidget should be removed, and users should be guided to use SplitDateTimeField if they want to use SplitDateTimeWidget.

It seems that the intention is for MultiValueField and MultiValueWidget to be used together, and catching this edge case in DateTimeField to allow it to work with SplitDateTimeWidget actually makes it more difficult for users who might want to subclass DateTimeField and override clean to validate against their own MultiValueWidget, because the super class contains validation specifically for a two item tuple value from MultiValueWidget.

This special case validation doesn't actually work properly now anyway, as it bypasses the required validation.

I'm not sure about the history of DateTimeField or how and why this validation was added, so I'm bumping this to DDN.

comment:4 Changed 15 years ago by Jarek Zgoda

Cc: jarek.zgoda@… added

comment:5 Changed 15 years ago by Tai Lee

Triage Stage: Design decision neededAccepted

As per Karen's request, I'm moving this back to accepted, as the DDN was not intended to raise any red flags. The bug should be fixed, has a working patch with tests, and the previous suggestion to remove SplitDateTimeWidget validation from DateTimeField would be a backwards incompatible change that should have been raised in a separate ticket.

comment:6 Changed 15 years ago by Karen Tracey

I don't believe the patch is correct as coded. DateTimeField is documented to return None for an empty value (http://docs.djangoproject.com/en/dev/ref/forms/fields/#datetimefield) and the patch has it returning a list in the case of empty input from a SplitDateTimeWidget. But it's not necessary to post another patch because that I could fix myself.

Before fixing this I think it would be good to understand why DateTimeField is even worried about getting input from a SplitDateTimeWidget. As noted above, it seems that is what SplitDateTimeField is for? The code was added in [6577] without reference to a ticket. Malcolm, do you recall why you added this? And do you have any recommendation as to whether it would be better to remove the half-working support entirely, make a fix along the lines of the patch, or ...? Strict backwards-compatibility would seem to require we can't remove it at this point....

comment:7 Changed 15 years ago by Karen Tracey

Patch needs improvement: set

comment:8 Changed 15 years ago by Tai Lee

If we can't remove it, and it seems we can't due to backwards compatibility (regardless of the reasons why it was added), can we get this fix committed? It's currently impossible to use an optional DateTimeField in the Django admin, with an exception being generated if no value is specified.

Changed 15 years ago by Tai Lee

Update tests to match None return value.

comment:9 Changed 15 years ago by Tai Lee

Patch needs improvement: unset

Just to make sure that the attached patch shouldn't be considered a red flag or incomplete, I've updated it to return None as per the documentation and updated the tests to match.

comment:10 in reply to:  8 Changed 15 years ago by Jacob

Triage Stage: AcceptedDesign decision needed

Replying to mrmachine:

It's currently impossible to use an optional DateTimeField in the Django admin, with an exception being generated if no value is specified.

That doesn't seem to be true; this works fine for me:

class Bug8898(models.Model):
    timestamp = models.DateTimeField(blank=True, null=True)

I'm thinking there's not a bug here -- as Karen said, SplitDateTimeField is the right choice here -- but if there is can you please clarify how (if at all) this breaks forms and/or the admin?

comment:11 Changed 15 years ago by doza

Cc: doza@… added

comment:12 Changed 15 years ago by Tai Lee

I was using a custom DateTimeField in my models which had a formfield() method that always used django.contrib.localflavor.generic.forms.DateTimeField as the form class so I wouldn't have to set input formats everywhere.

This was overriding the form class specified in BaseModelAdmin.formfield_for_dbfield for DateTimeField db fields and exposing the validation bug in forms.DateTimeField, which was to bypass the "required" validation when used with SplitDateTimeWidget.

In [9760] ModelAdmin.formfield_for_dbfield was re-factored and no longer treated my custom DateTimeField as a models.DateTimeField, because the new code checks if db_field.__class__ is in self.formfield_overrides instead of using isinstance().

To summarise:

  • The bug in forms.DateTimeField when used with SplitDateTimeWidget still exists, though this combination is not normally found in Django core. I suggest that we should still remove this vestigial code to avoid confusion and make subclassing forms.DateTimeField easier. After all, MultiWidget should only be used with MultiValueField.
  • A new bug (I'd call it a bug - it is a change in behaviour) exists where subclassed model fields are not recognised as their parent class in BaseModelAdmin.formfield_for_dbfield(). I suppose the fix for this would be to loop over self.formfield_overrides and check with isinstance() in each iteration. When there's a match, update kwargs and return.

comment:13 Changed 15 years ago by Tai Lee

The new bug I stumbled across already has a ticket with patch, #10059, but this vestigial code should still be removed :)

comment:14 Changed 15 years ago by Karen Tracey

Also related: #9721. That one wants to mix together the SplitDateTimeWidget with a DateTimeField in order to allow for an optional time component. Perhaps that is why DateTimeField was given some support for getting input from a SplitDateTimeWidget in the first place? If so I think a better approach could be to add support for that kind of thing to the SplitDateTimeField/SplitDateTimeWidget combo rather than trying to mix a split widget with a non-split field.

comment:15 Changed 14 years ago by Gert Van Gool

Cc: gertvangool@… added

comment:16 Changed 14 years ago by anonymous

This bug has wider implications - if you have a form field with a MultiWidget, and you set it to required = False, then if you don't input anything in the widgets - it will tell you that you must input a value. The code which is flawed is this, inside clean() of a Field:

if value in EMPTY_VALUES:

return None

if you have 2 widgets then value == [, ] for an empty input, and the above doesn't work.

I will circumvent this in my code, but it'd be nice if it got corrected :)

comment:17 Changed 14 years ago by Karen Tracey

#12303 reported this again, and provided an alternative fix.

comment:18 Changed 14 years ago by samueladam

Another idea could be to attach EMPTY_VALUES to the Field and have the Widget edit it.

I bypassed this bug using forms.SplitDateTimeField.

comment:19 Changed 13 years ago by Łukasz Rekucki

Duplicated again by #14044.

comment:20 Changed 13 years ago by Carl Meyer

Triage Stage: Design decision neededAccepted

I agree with Karen that DateTimeField should not be used with SplitDateTimeWidget, and that adding ad-hoc multi-widget support to formfield clean() methods is a bad idea. That code in DateTimeField.clean() can't be removed immediately due to backwards-compat, but I would consider a patch with a PendingDeprecationWarning that refers people to SplitDateTimeField and allows us to remove that code in a couple releases. This should come with appropriate documentation and a note in the deprecation docs as well.

comment:21 Changed 13 years ago by Jeffrey Gelens

Cc: jgelens@… added

comment:22 Changed 13 years ago by Luke Plant

Patch needs improvement: set

Marking as patch needs improvement on the basis of Carl's comments.

comment:23 Changed 13 years ago by Julien Phalip

Severity: Normal
Type: Bug

See #13205 for a related issue.

comment:24 Changed 12 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:25 Changed 12 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:26 Changed 10 years ago by Claude Paroz

Patch needs improvement: unset

comment:27 Changed 10 years ago by Claude Paroz

Reading ticket #9721, it appears that DateTimeField with SplitDateTimeWidget may be used when only the time part of the field is optional, which would not be possible with SplitDateTimeField. To be investigated...

comment:28 Changed 10 years ago by Claude Paroz

It may be that #15511 filled the above use case (setting require_all_fields to False, then SplitDateTimeField.fields[1].required to False in a SplitDateTimeField subclass).
@mrmachine, can you confirm?

comment:29 Changed 10 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

Needs a line in the release notes, otherwise LGTM.

comment:30 Changed 10 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In 0179852d7faf461d55cf3ae69393abb3f3cd2910:

Fixed #8898 -- Obsoleted SplitDateTimeWidget usage with DateTimeField

Thanks Tim Graham for the review.

comment:31 Changed 9 years ago by Tim Graham <timograham@…>

In 714277cb4cedd8290101f9c6b3e6382f192ae177:

Removed support for SplitDateTimeWidget with DateTimeField per deprecation timeline.

refs #8898

comment:32 Changed 8 years ago by Tim Graham <timograham@…>

In 6b592697:

Refs #8898 -- Documented requirement to use SplitDateTimeField with SplitDateTimeWidget.

comment:33 Changed 8 years ago by Tim Graham <timograham@…>

In 86b34643:

[1.9.x] Refs #8898 -- Documented requirement to use SplitDateTimeField with SplitDateTimeWidget.

Backport of 6b5926978bfbaebc6e3b96bb2c8e5bc8302ac6b0 from master

Note: See TracTickets for help on using tickets.
Back to Top