Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#13621 closed (fixed)

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

Reported by: bufke 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: UI/UX:

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 zerok 5 years ago.
ticket13621-alternative2.diff (2.3 KB) - added by jacmkno 5 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 5 years ago.
An alternative patch, makes the localize attribute default to True in date/time fields
t13621-alternative4.diff (43.3 KB) - added by russellm 5 years ago.
Another alternative, with much more extensive tests
t13621-alternative5.diff (42.0 KB) - added by russellm 5 years ago.
Another alternative, this time preserving the isolation of input_formats on Field.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 5 years ago by zerok

  • Component changed from Uncategorized to Forms
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 5 years ago by zerok

  • Owner changed from nobody to zerok

comment:3 follow-up: Changed 5 years ago by ys@…

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 Changed 5 years ago by zerok

  • Has patch set
  • Needs tests unset

comment:5 in reply to: ↑ 3 ; follow-up: Changed 5 years ago by yserrano

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

comment:6 Changed 5 years ago by zerok

Updated the patch to include even more unittests ;-)

comment:7 in reply to: ↑ 5 Changed 5 years ago by zerok

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 Changed 5 years ago by vasiliyeah

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 Changed 5 years ago by zerok

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 Changed 5 years ago by zerok

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.

Changed 5 years ago by zerok

comment:11 Changed 5 years ago by jacmkno

  • Owner changed from zerok to jacmkno
  • Status changed from new to assigned
  • Summary changed from regression in 1.2.1 from 1.2 AM PM time input no longer works correctly to Regression 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 Changed 5 years ago by jacmkno

  • Owner changed from jacmkno to zerok
  • Status changed from assigned to new

Changed 5 years ago by jacmkno

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

comment:13 Changed 5 years ago by jacmkno

  • Triage Stage changed from Accepted to Ready 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).

Changed 5 years ago by jacmkno

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

comment:14 follow-up: Changed 5 years ago by jacmkno

  • Owner changed from zerok to jacmkno
  • Status changed from new to assigned

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.

comment:15 in reply to: ↑ 14 Changed 5 years ago by fheinz

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 Changed 5 years ago by ramiro

  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

comment:17 Changed 5 years ago by russellm

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

Changed 5 years ago by russellm

Another alternative, with much more extensive tests

comment:18 Changed 5 years ago by russellm

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.

Changed 5 years ago by russellm

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

comment:19 Changed 5 years ago by russellm

  • milestone set to 1.3

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

comment:20 Changed 5 years ago by kmtracey

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 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from assigned to closed

(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 Changed 5 years ago by russellm

(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 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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