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

[Tai Lee]

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.

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

[Tai Lee]

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.

[Tai Lee]

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.

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

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

[Tai Lee]

Update tests to match None return value.

[Tai Lee]

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.

[Jacob]

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?

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

[Tai Lee]

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

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

[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 :)

[Karen Tracey]

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

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

[Łukasz Rekucki]

Duplicated again by #14044.

[Carl Meyer]

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.

[Luke Plant]

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

[Julien Phalip]

See #13205 for a related issue.

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Claude Paroz]

​https://github.com/django/django/pull/2028

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

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

[Tim Graham]

Needs a line in the release notes, otherwise LGTM.

[Claude Paroz <claude@…>]

In 0179852d7faf461d55cf3ae69393abb3f3cd2910:

Fixed #8898 -- Obsoleted SplitDateTimeWidget usage with DateTimeField

Thanks Tim Graham for the review.

[Tim Graham <timograham@…>]

In 714277cb4cedd8290101f9c6b3e6382f192ae177:

Removed support for SplitDateTimeWidget with DateTimeField per deprecation timeline.

refs #8898

[Tim Graham <timograham@…>]

In 6b592697:

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

[Tim Graham <timograham@…>]

In 86b34643:

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

Backport of 6b5926978bfbaebc6e3b96bb2c8e5bc8302ac6b0 from master