Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Date fields increment on entity save when the timezone is UTC+12 eg Pacific/Auckland (at this time of year), Asia/Anadyr, Pacific/Chatham
To recreate on new standard install:
- Set site default timezone to Pacific/Auckland
- Add a new date field to a content type using 'Date only'
- Create a new node of the the content type and enter a date
- View the created node and the date will have incremented by 1 day
- Every edit & save will increment the date by 1 again
Proposed resolution
Remove any timezone conversion and time handling from date-only fields.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#40 | interdiff-34-38.txt | 1.34 KB | mpdonadio |
#38 | 2739290-38.patch | 21.29 KB | jhedstrom |
#38 | 2739290-38-TEST-ONLY.patch | 16.77 KB | jhedstrom |
Comments
Comment #2
mpdonadioBy any chance, do you know if this worked properly in Drupal 7 with the Date module, and if so which version of Date (one of the later release changed how the dates are actually stored).
Pretty sure the problem is the datetime_date_default_time() that are scattered throughout the module to warp the time on date-only to noon UTC, so they can be treated as DrupalDateTime objects (which derive from \DateTime). Going to have to review some old issues around this. I suspect we need to handle the edge cases in there.
Comment #3
mpdonadioConfirmed with Asia/Anadyr as the TZ (Pacific/New_Zealand wasn't an option for me).
The value in the database is OK. The problem is that
- the value is read from the database, the current time is set on the object.
- DateTimeWidgetBase::formBase() calls datetime_date_default_time(), which sets the time to noon UTC.
- the TZ is applied, so 12 hours are added and we roll into the next day.
I need to dig for the issue where we set this behavior. It used to be set TZ, then set default date, but we had weird problems with lots of date rollovers, and swapped the order. Obviously, we didn't consider all of the edge cases...
Blerch...
Comment #4
AndyD328Many thanks mpdonadio, that TZ should have read Pacific/Auckland not Pacific/New Zealand, my apologies.
Comment #5
AndyD328Would it still help if I investigate the D7 version of the module?
Comment #6
mpdonadio#5, if you have a D7 install handy with Date and configured for NZ that you can report on, it would help. My D7 instance is usually in a state of destruction from work on other things.
Comment #7
AndyD328D7 with Date 2.9 and 2.x-dev work fine with a site set to Pacific/Auckland and the field as date only.
Comment #8
jhedstromI'll dig into this a bit.
Comment #9
jhedstromFrom the comment on
datetime_date_default_time()
:wouldn't that only apply to timezones that are the same as UTC? It seems more reasonable to just set it to 0, which would resolve the problem entirely I think.
I'll work on a test.
Comment #10
jhedstromI wasn't able to reproduce this in local testing (I'm at UTC-8 server time, and even setting the timezone to Pacific/Auckland didn't create the issue). I expected this test change to fail, but it doesn't. Uploading here because it might fail on the test bots as expected since they are set to UTC server time.
Oddly, changing this to 0 instead of 12 made the date start decreasing by one day on each save:
Comment #11
mpdonadioThe problem with midnight UTC is that the negative offsets will become the previous day once you apply the timezone offset based on the system settings (eg, I'm UTC-4 at the moment, so 2016/06/03 00:00:00 UTC is 2016/06/02 20:00:00 for me => previous day). When you use noon UTC, most cases will warp to the same day. Rollover happens at the extremes, which is what is happening with this bug.
I keep confusing myself everytime I write up more thoughts. I am going to try to diagram this. It seems like we are in a catch-22 situation. Again.
I'm wondering if we need to back up and hash out in words what a date-only object is and what time it is and where. Then we can back out to whether we are implementing that properly. And taking into account that storage only has the date and not time or timezone, so that will always be implicit.
Still trying to figure out what issue we adjusted this in.
Comment #13
jhedstromI think the intent of the date-only field type is to not have any timezone handling. The current implementation seems to add timezone handling due to perhaps a limitation in the datetime widget form.
In some of the fixes over in #2627512: Datetime Views plugins don't support timezones, timezone offsets are explicitly removed for any date-only fields.
Comment #14
jhedstromUpdated IS to reflect the removal of timezone offset handling from date-only fields. Going to see how involved this fix is.
Comment #15
jhedstromHere's a start at removing timezone offset calculations from date-only fields.
I still was unable to get the test to mimic the exact behavior described in the IS (note the 3 subsequent form submissions do not increment the date). However, the test changes do illustrate an end user in a UTC-12 timezone experiencing the date shifting (the submitted 2012-12-31 date gets converted to 12-30, and then later is displayed as 2013-01-01).
Comment #16
mpdonadioRun TestBot, run!
Comment #19
jhedstromThese fixes are against 8.2.x.
Comment #20
jhedstromComment #21
jhedstromand again...
Comment #25
mpdonadioOn tablet, so I can't do a proper review. Patch looks mostly good, just some small things.
Still not 100% convinced this is proper approach. I'm going to try to dig for comparisons with other systems. But this, combined with the all day date range in the end date issue would really cover all scenarios.
Anyone else have thoughts?
Comment #26
mpdonadioJust some quick thoughts...
Prob need a CR about this in case anything in contrib is using this? We need to @deprecated that function at some point, too..
Nit. Can we do a === here?
Looks like an accidental change that would get this pushed back?
I'm wondering if we need to defer the TZ configuration to the test itself, and then loop through a set representing extremes at both ends?
Comment #27
jhedstromHa, I thought I was rolling back my own refactoring when I removed that--didn't realize that was already in core that way. Since we're refactoring that method anyway, is there harm in removing code mistakes?
Comment #28
jhedstromGood call on looping through some timezones. The extremes caught a few more issues.
Comment #29
jhedstromNote that the interdiff was done with
--ignore-all-space
to make the indentation change easier to review.Comment #30
jhedstromOops, this should be +12. I wanted to do +14 but couldn't find one that PHP supported.
And this comment is no longer relevant.
Comment #32
mpdonadioI think Pacific/Tongatapu will be the most extreme positive offset at UTC+13 and Pacific/Kwajalein should be UTC-12 which I think it the most negative extreme in PHP.
Comment #33
mpdonadioI went through the time zone list, and picked out some that I think will be good: few at each extreme, few in between, and UTC, all without DST so the test results are predictable:
I think this warrants a CR about the change, especially to datetime_date_default_time(). Kinda wondering if we should @deprecate that as part of this?
Otherwise, I am OK with this approach, especially if we can get all-day into the end-date patch.
Comment #34
jhedstromI was going to mark it as
@deprecated
, but realized there wasn't a good replacement to direct folks to. We could hard code the00:00:00
time everywhere it's used and just deprecate it without a replacement?This patch updates the timezone list to test.
Comment #35
mpdonadioReviewed this again. I think this is good to go, and paired with the all-day feature from #2161337: Add a Date Range field type with support for end date, will allow better flexibility for user needs. Have Change Record ready.
Comment #36
alexpottIs this change actually necessary? And doesn't this mean we need an upgrade path? What happens to existing fields - do they change when you resave them?
Comment #37
jhedstromIn the current patch, the only thing the change to the default time impacts in the tests is the display part (line 207 of
DateTimeFieldTest
, with the short/medium/long formatters). Rather than displaying date-only fields with a time of00:00:00
, they display with12:00:00
. In the real world, I'm not sure folks would be using date formats with a time component for date-only fields.Comment #38
jhedstromI spoke with @mpdonadio in IRC, and we agree it makes sense to have the test look for the default time if displaying a date-only field with a formatter that includes a time component.
Comment #40
mpdonadioUpdated CR to reflect new patch. Attached the interdiff between #34 and #38.
I am OK with not changing the default. The changes to DateTimeFieldTest are easier to read in place because of the loop that was introduced, or as `git diff -b origin/8.2.x`.
Comment #41
AndyD328Would be great to get this committed to 8.1, it's working well here.
Will it definitely be in 8.2?
Comment #42
mpdonadio#41, we are just waiting for a committer to take another look at this. There is a decent backlog of RTBC issues for them to go through.
Comment #43
mpdonadioAdding blocker tag b/c this is hard blocking and soft blocking some Datetime work that we would really like to get into 8.2.x.
Comment #44
alexpottCommitted 5694189 and pushed to 8.2.x. Thanks!
The patch does not apply to 8.1.x - I think it might be eligible because whilst it is a behaviour change - the current behaviour is wrong. @mpdonadio if you think it is worth back-porting then feel free to re-open.