Opened 14 years ago

Closed 14 years ago

Last modified 12 years ago

#13621 closed (fixed)

Regression in 1.2.1. date/time widgets are printing their values with an invalid format

Reported by: David Burke Owned by: jacmkno
Component: Forms Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

In 1.2 I used this in settings.py

TIME_INPUT_FORMATS = ('%I:%M %p',)

to accept times like 1:30 PM and it worked fine. After upgrading to 1.2.1 Django accepts times like that but it displays them in the 24 hour format, thus it prints times in a format it cannot accept. For example in the user settings when I edit and save (doing nothing) I get errors because the time is displayed in the 24 hour format yet it only accepts 12 hour format.

Attachments (5)

ticket13621.diff (12.1 KB ) - added by Horst Gutmann 14 years ago.
ticket13621-alternative2.diff (2.3 KB ) - added by jacmkno 14 years ago.
An alternative patch. Changes the validations to verify the localize field option before validating the format.
ticket13621-alternative3-tested.diff (1.5 KB ) - added by jacmkno 14 years ago.
An alternative patch, makes the localize attribute default to True in date/time fields
t13621-alternative4.diff (43.3 KB ) - added by Russell Keith-Magee 14 years ago.
Another alternative, with much more extensive tests
t13621-alternative5.diff (42.0 KB ) - added by Russell Keith-Magee 14 years ago.
Another alternative, this time preserving the isolation of input_formats on Field.

Download all attachments as: .zip

Change History (28)

comment:1 by Horst Gutmann, 14 years ago

Component: UncategorizedForms
Needs tests: set
Triage Stage: UnreviewedAccepted

comment:2 by Horst Gutmann, 14 years ago

Owner: changed from nobody to Horst Gutmann

comment:3 by ys@…, 14 years ago

I have the same problem with dates in the django admin, DATE_INPUT_FORMATS = ('%d.%m.%Y', ) doesn't works anymore it displays a Y-m-d date. It worked in django 1.2 but not in django 1.2.1

comment:4 by Horst Gutmann, 14 years ago

Has patch: set
Needs tests: unset

in reply to:  3 ; comment:5 by Yves Serrano, 14 years ago

The changeset [13296] seems to create this problem. With django trunk@13295 I don't have the date input problem.

comment:6 by Horst Gutmann, 14 years ago

Updated the patch to include even more unittests ;-)

in reply to:  5 comment:7 by Horst Gutmann, 14 years ago

Replying to yserrano:

The changeset [13296] seems to create this problem. With django trunk@13295 I don't have the date input problem.

Yes, the problem here was, that the logic within _format_value of each date/time related field was probably accidentally changed without handing the permitted input/output format down to the widget. My patch should address that for DateInput, DateTimeInput, TimeInput and the SplitDateTimeInput widget.

comment:8 by Vasil Vangelovski, 14 years ago

I have a formats.py file defined for Macedonian locale in one of my projects, looks like this:

DATE_FORMAT = 'd.m.Y'
DATETIME_FORMAT = 'd.m.Y H:i'

It all worked fine with 1.2. After upgrading to 1.2.1 the AdminDateWidget displayed dates in the '%Y-%m-%d %H:%M:%S' format, so resaving a model failed to validate.

comment:9 by Horst Gutmann, 14 years ago

I've attached an updated version of the patch. The original one had a problem when you were using multiple languages on your site or switching between them.

comment:10 by Horst Gutmann, 14 years ago

Another update to the patch. The previous version was a bit too aggressive and more or less disabled localize=False in order to get the format to work in the admin. I'v now moved that behaviour into the admin's form field generator to make fields by default localize=True if USE_L10N=True and no previous setting was specified for that flag.

by Horst Gutmann, 14 years ago

Attachment: ticket13621.diff added

comment:11 by jacmkno, 14 years ago

Owner: changed from Horst Gutmann to jacmkno
Status: newassigned
Summary: regression in 1.2.1 from 1.2 AM PM time input no longer works correctlyRegression in 1.2.1. date/time widgets are printing their values with an invalid format

This problem causes the django admin to become unusable in forms with date or datetime fields. This needs to fixed soon in the trunk. I changed the title to make it sound a bit more precise.

comment:12 by jacmkno, 14 years ago

Owner: changed from jacmkno to Horst Gutmann
Status: assignednew

by jacmkno, 14 years ago

An alternative patch. Changes the validations to verify the localize field option before validating the format.

comment:13 by jacmkno, 14 years ago

Triage Stage: AcceptedReady for checkin

ticket13621-alternative2.diff - Details:
Django is currently using localization formatting only for the validations and the client-side widget behavior but not when filling the initial data of the field. So at first I thought the problem was about filling the localized data in the field, but it turns out that both the widget and the form field use the localized format if the localize attribute is set to True. Finally I found that for validations, Django was not checking the localize attribute but only the USE_L10N setting. Fixing this took changing only 5 lines of code, hopefully it solves the AM/PM problem too, but i'm not sure (haven't tested that yet).

by jacmkno, 14 years ago

An alternative patch, makes the localize attribute default to True in date/time fields

comment:14 by jacmkno, 14 years ago

Owner: changed from Horst Gutmann to jacmkno
Status: newassigned

ticket13621-alternative2.diff had a problem. Django's client side I18N architecture is build in the expectation that all I/O from date time fields is localized, so the client side part of the admin was still placing localized data in the fields.

The new patch ticket13621-alternative3.diff, simply makes the date or time fields default their localize attribute to True which seems to work fine in EN, ES and ZH. Haven't tested the AM/PM problem yet.

in reply to:  14 comment:15 by Fede Heinz, 14 years ago

Replying to jacmkno:

The new patch ticket13621-alternative3.diff, simply makes the date or time fields default their localize attribute to True which seems to work fine in EN, ES and ZH. Haven't tested the AM/PM problem yet.

I just tried this patch, and I can confirm that it also solves the AM/PM problem, at least with es-es.

comment:16 by Ramiro Morales, 14 years ago

Needs tests: set
Triage Stage: Ready for checkinAccepted

comment:17 by Russell Keith-Magee, 14 years ago

Patch needs improvement: set

As far as I can make out, there are actually three problems here:

  1. settings.TIME_INPUT_FORMATS isn't honored by the input widget
  2. A form field that specifies input_formats=... doesn't pass that configuration down to the widget
  3. There's no easy way to enable localized widgets in admin (it's possible, but not trivial or automatic as you might expect)

Problem (1) is the regression from 1.2.

Problem (2) exists in 1.1.

Problem (3) is a request for an admin enhancement. It's a legitimate request, but it's separate to this bug.

@jacmkno -- the solution isn't just to force all datetime widgets to be localized by default; this hides the issue, rather than fixing it. All you have to do is set localize=False to reveal the bug again.

@zerok's patch is closer to the mark, but it convolves the three issues.

by Russell Keith-Magee, 14 years ago

Attachment: t13621-alternative4.diff added

Another alternative, with much more extensive tests

comment:18 by Russell Keith-Magee, 14 years ago

I've just uploaded a candidate patch for this issue. This fixes point (1) and (2) from my previous post.

However, on reflection, issue (2) requires some more thought.

When we talk about date/time formats, there are actually two different formats under discussion:

  1. The format that will be used to display dates to the user.
  2. The list of formats that will be accepted as user input.

The issue at hand is whether specifying an input format specified on the field (which is then used as an accepted format for input) should imply anything about the way the widget displays data.

In order for form resubmissions to work, (a) must be part of the list provided by (b), but this hasn't been enforced historically. This has worked because the default YYYY-MM-DD and HH:MM:SS formats have always been accepted input formats, so it didn't matter if you specified additional input_formats; the defaults would always be readable.

However, now that we have L10N support, there's no simple 'default' date format, and if the user manually specifies an input_formats list on a field, there's no guarantee that the locale will output dates in a manner compatible with this format.

The fix for this is to ensure that if input_formats is specified, the first format in that list is used as the widget's display format. Historically, the onus would have been on the developer to also provide a widget that explicitly named that format (i.e., DateField(input_formats=[...], widget=forms.DateWidget(format='...')) ). I'm not entirely happy with this fix, as it exposes even more of the internals of widgets into the implementation of Field.

It's also worth noting that this is something that most users won't have to deal with -- if you set up INPUT_DATE_FORMAT et al as a system wide setting, the default format of the widget will be the first INPUT_DATE_FORMAT value. The failure to do this was the regression that is the subject of this ticket.

So - the question becomes: do we actually want to fix (2) at all? Or should we treat this as an edge case (i.e., if you specify input_formats, then the onus is on you to make sure you specify a widget with a matching format)? I'm inclined to take the second approach, but I'd like to hear from others before I commit. If you want to see what the patch would look like without (2) being fixed, just revert the changes to django/db/fields.py. Some of the test cases will fail as a result, but for obvious reasons.

by Russell Keith-Magee, 14 years ago

Attachment: t13621-alternative5.diff added

Another alternative, this time preserving the isolation of input_formats on Field.

comment:19 by Russell Keith-Magee, 14 years ago

milestone: 1.3

For good measure, t13621-alternative5.diff is the patch without the fix for problem (2).

comment:20 by Karen Tracey, 14 years ago

I don't think we should fix (2) as part of this ticket. It's always been possible to get into trouble by having a field with a widget output format that is not one of the specified input formats (I remember at least one person on django-users hitting that). It might be something we could look at "fixing" in some way later, but since it is not new behavior I don't think changing it belongs in the fix for this ticket. So long as fixing (1) ensures that by default the widgets display a format that will be accepted as input, I think that is sufficient for this ticket.

comment:21 by Russell Keith-Magee, 14 years ago

Resolution: fixed
Status: assignedclosed

(In [13484]) Fixed #13621 -- Corrected the handling of input formats on date/time form fields. Thanks to bufke for the report, zerok and jacmkno for their work on the patch, and Karen, Jannis and Alex for feedback.

comment:22 by Russell Keith-Magee, 14 years ago

(In [13486]) [1.2.X] Fixed #13621 -- Corrected the handling of input formats on date/time form fields. Thanks to bufke for the report, zerok and jacmkno for their work on the patch, and Karen, Jannis and Alex for feedback.

Backport of r13484 from trunk.

comment:23 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

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