I couldn’t find any similar issues on Drupal.org so I opened this one, if anyone knows of a similar issue please feel free to link and close this as duplicate.
Problem/Motivation
This was found while diagnosing an issue on the Scheduler module. However it also affects the “Authored On” field.
https://www.drupal.org/project/scheduler/issues/3085947
The node edit form's "Authored On" field uses the timezone of whatever user was on that form during the last cache clear. This can end up with a Chicago User saving a node that has an “Authored On” value in UTC.
This only seems to affect new nodes, editing existing nodes are not affected.
Steps to Reproduce
Requirements: Have two users, one with a UTC timezone and one with a Chicago Timezone
Step One: Create content as an UTC User
- Log in as the UTC user
- Clear the Drupal cache
- Create a new Basic Page
- Note the Authoring information (e.g 4:00:00 PM)
- Save and publish the node.
Step Two: Create content as a Chicago User
- Log in as the Chicago user
- Do not clear the cache
- Create a new Basic Page
- Note the Authoring information
- Observe that the Authoring information shows approximately the same time at UTC (e.g 4:02 PM)
- This should show a time relative to Chicago (e.g 11:00 AM)
For authoring information this is relatively benign however it does impact Scheduling.
Proposed resolution
It looks like this is being caused by code in the Datetime::getInfo() method.
core/lib/Drupal/Core/Datetime/Element/Datetime.php
public function getInfo() {
// .. node: some code removed for brevity …
return [
'#input' => TRUE,
'#element_validate' => [
[$class, 'validateDatetime'],
],
'#process' => [
[$class, 'processDatetime'],
[$class, 'processAjaxForm'],
[$class, 'processGroup'],
],
'#pre_render' => [
[$class, 'preRenderGroup'],
],
'#theme' => 'datetime_form',
'#theme_wrappers' => ['datetime_wrapper'],
'#date_date_format' => $date_format,
'#date_date_element' => 'date',
'#date_date_callbacks' => [],
'#date_time_format' => $time_format,
'#date_time_element' => 'time',
'#date_time_callbacks' => [],
'#date_year_range' => '1900:2050',
'#date_increment' => 1,
// Timezone is set here and gets cached.
'#date_timezone' => drupal_get_user_timezone(),
];
}
The getInfo() method only gets called on cache clear, therefore the element that gets loaded gets the timezone of whichever user loads the page after the cache is cleared.
This bug has seemingly been in place for some time, but it was made much more visible with the deprecation of drupal_get_user_timezone()
in #2799987: Datetime and Datelist element's timezone handling is fragile, buggy and wrongly documented. Prior to that the site's timezone would be used rather than the server's timezone when caches are built without a user (eg, via drush, or some other backend process.)
Comment | File | Size | Author |
---|---|---|---|
#33 | datetime_getinfo_kang-3087606-33.patch | 2.83 KB | Oscaner |
#22 | datetime_getinfo_cac-3087606-22.patch | 3.31 KB | jhedstrom |
Comments
Comment #2
partdigital CreditAttribution: partdigital commentedComment #3
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedYes this is big problem on multi-location sites that are using Scheduler module.
First reported in #3085947: Entered date is stored using cached timezone from previous user then we realised it was a Core issue.
Comment #4
kim.pepperThis function was deprecated in #3009377: Replace drupal_get_user_timezone with a date_default_timezone_get() and an event listener in Drupal 8.8.0-dev. Are you able to test with the latest 8.8.x branch?
Comment #5
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedHi @kim.pepper,
Thanks for linking that issue. I have just tested with 8.8-dev updated as of today, and yes the problem is still there. You are right that in core/lib/Drupal/Core/Datetime/Element/Datetime.php, datetime::getInfo() method now has
'#date_timezone' => date_default_timezone_get()
not'#date_timezone' =>drupal_get_user_timezone()
. However, that does not seem to solve the problem. I think as @partdigital pointed out in the issue summary, the getInfo() method only gets called on cache clear, and that is the key problem.Comment #6
yunke CreditAttribution: yunke commentedI also found this problem and tried to solve it as follows:
First, in getInfo() ,delete :
then:
Comment #7
larowlan@yunke yes that seems like a good approach - did it work?
Comment #8
jhedstromAfter further research, I don't think #3009377: Replace drupal_get_user_timezone with a date_default_timezone_get() and an event listener caused this issue, but it made a presumably already-existing bug much more noticable. Prior to that change, when caches are cleared on the backend (where the default timezone is set to UTC), the cached value at least fell to whatever the site's default timezone was.
Comment #9
jhedstromIn manual testing, this resolves the issue. We'll need tests to ensure against future regression though.
Comment #10
jhedstromI think this has more to do with dates than the form system, so moving component.
Comment #11
jhedstromThis adds a test that demonstrates the current bug, and the fix. The test-only patch is also the interdiff.
Comment #12
jhedstromNote, these were straight-up bugs. The values here are strings, not arrays. No error was thrown because DateTimePlus catches all exceptions and logs them.
Comment #13
jhedstromThis is an easier to understand test.
Comment #14
acbramley CreditAttribution: acbramley at PreviousNext commentedTest looks really nice to me, straight forward and to the point, nice one!
Comment #15
acbramley CreditAttribution: acbramley at PreviousNext commentedÜber-nit: This could be a 1 liner
Comment #16
kim.pepperLooking forward to using
in PHP 7.4 :-)
Comment #17
larowlanLooks great
The one-liner could also be
Comment #18
kim.pepperGood catch! I agree this is ready on green.
Comment #21
Berdir> Prior to that change, when caches are cleared on the backend (where the default timezone is set to UTC), the cached value at least fell to whatever the site's default timezone was.
Confused by this. What exactly is backend here? If you talk about /admin/something or the clear cache admin_toolbar link then that's just a regular request, the other issue doesn't care if it's frontend or backend.
But maybe backend here is actually drush or console.. and maybe they're not triggering the request subscriber that sets the default timezone. So that *would* be a regression of that issue, but I'd say that maybe drush should be responsible for setting that then?
Comment #22
jhedstromThat's what I meant by backend. No user is set when doing a
drush cr
, so whateverdate.timezone
value is set inphp.ini
is used, in my case,UTC
.So that *would* be a regression of that issue
It's a regression, but the bug of caching whatever user or non-user had set in the
Datetime::getInfo()
was already a bug, this just makes it much more obvious. Either way, this should fix both I think :)Comment #23
jhedstromUpdating the IS to reflect the notes in #22. It would be great to get this fixed.
Comment #24
BerdirThe fix and test looks fine to me.
Despite 3 different nitpicks on the same line, I think the current line to set the default is also fine :)
And I posted in https://github.com/drush-ops/drush/issues/1166#issuecomment-575320943 about the more general problem that might also affect other places that rely on the timezone being set correctly in drush.
Comment #26
jonathan1055 CreditAttribution: jonathan1055 as a volunteer commentedPatch #22 was passing before (many times), but failed this morning, so I re-queued it, and it passed. Setting back to RTBC as per #24
Comment #27
catchCommitted 3d9ac25 and pushed to 9.0.x, 8.9.x, and 8.8.x. Thanks!
Comment #31
catchComment #33
Oscaner CreditAttribution: Oscaner at CI&T commentedPatch for Drupal core 8.7.x