#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)
Change History (28)
comment:1 by , 14 years ago
Component: | Uncategorized → Forms |
---|---|
Needs tests: | set |
Triage Stage: | Unreviewed → Accepted |
comment:2 by , 14 years ago
Owner: | changed from | to
---|
follow-up: 5 comment:3 by , 14 years ago
comment:4 by , 14 years ago
Has patch: | set |
---|---|
Needs tests: | unset |
follow-up: 7 comment:5 by , 14 years ago
The changeset [13296] seems to create this problem. With django trunk@13295 I don't have the date input problem.
comment:7 by , 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 , 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 , 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 , 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 , 14 years ago
Attachment: | ticket13621.diff added |
---|
comment:11 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Summary: | regression in 1.2.1 from 1.2 AM PM time input no longer works correctly → 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 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | assigned → new |
by , 14 years ago
Attachment: | ticket13621-alternative2.diff added |
---|
An alternative patch. Changes the validations to verify the localize field option before validating the format.
comment:13 by , 14 years ago
Triage Stage: | Accepted → 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).
by , 14 years ago
Attachment: | ticket13621-alternative3-tested.diff added |
---|
An alternative patch, makes the localize attribute default to True in date/time fields
follow-up: 15 comment:14 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | new → 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 by , 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 , 14 years ago
Needs tests: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
comment:17 by , 14 years ago
Patch needs improvement: | set |
---|
As far as I can make out, there are actually three problems here:
- settings.TIME_INPUT_FORMATS isn't honored by the input widget
- A form field that specifies input_formats=... doesn't pass that configuration down to the widget
- 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 , 14 years ago
Attachment: | t13621-alternative4.diff added |
---|
Another alternative, with much more extensive tests
comment:18 by , 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:
- The format that will be used to display dates to the user.
- 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 , 14 years ago
Attachment: | t13621-alternative5.diff added |
---|
Another alternative, this time preserving the isolation of input_formats on Field.
comment:19 by , 14 years ago
milestone: | → 1.3 |
---|
For good measure, t13621-alternative5.diff is the patch without the fix for problem (2).
comment:20 by , 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 , 14 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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