-
-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix date only field not working. #235
base: master
Are you sure you want to change the base?
Conversation
$date->setTimezone($storageTimezone); | ||
$values[$key] = $date->format('Y-m-d\TH:i:s'); | ||
if ($this->fieldInfo->getSetting('datetime_type') === 'date') { | ||
$date = new \DateTime($value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we keep applying the storage timezone, even on the date type? The only thing we need to change is the storage format, which depends on the field type, but we should keep the initial conversion into UTC, that's necessary, as explained in the comment above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my tests the answer is no, but I need to still debug to see why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the date has no time it should not have any timezone conversion, also mentioned here https://git.drupalcode.org/project/drupal/-/blob/9.2.x/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php#L167 (saving or printing makes no difference, no time, no timezone).
If we take for example:
$storageTimezone = new \DateTimeZone('UTC');
$date = new \DateTime('2021-05-09');
var_dump($date);
$date->setTimezone($storageTimezone);
var_export($date->format('Y-m-d'));
then the output is:
object(DateTime)[2]
public 'date' => string '2021-05-09 00:00:00.000000' (length=26)
public 'timezone_type' => int 3
public 'timezone' => string 'Europe/Brussels' (length=15)
'2021-05-08'
So we can a) not set a timezone for faulty conversion, or b) assume a default time to circumvent the problem
$date->setTime(12, 0, 0);
(very important here to set this before $date->setTimezone
, so before any conversion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we need a datetime object, if we have no time? If we are anyway expecting the step author to provide the date in the DateTimeItemInterface::DATE_STORAGE_FORMAT, can't we just apply the timezone conversion for datetime fields, while simply pass along fields having datetime_type: date
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, wouldn't this work?
/**
* {@inheritdoc}
*/
public function expand($values) {
$siteTimezone = new \DateTimeZone(\Drupal::config('system.date')->get('timezone.default'));
$storageTimezone = new \DateTimeZone(DateTimeItemInterface::STORAGE_TIMEZONE);
foreach ($values as $key => $value) {
if (strpos($value, "relative:") !== FALSE) {
$relative = trim(str_replace('relative:', '', $value));
// Get time, convert to ISO 8601 date in GMT/UTC, remove TZ offset.
$values[$key] = substr(gmdate('c', strtotime($relative)), 0, 19);
}
elseif ($this->fieldInfo->getSetting('datetime_type') === 'datetime') {
// A Drupal install has a default site timezone, but nonetheless
// uses UTC for internal storage. If no timezone is specified in a date
// field value by the step author, assume the default timezone of
// the Drupal install, and therefore transform it into UTC for storage.
$date = new \DateTime($value, $siteTimezone);
$date->setTimezone($storageTimezone);
$format = DateTimeItemInterface::DATETIME_STORAGE_FORMAT;
$values[$key] = $date->format($format);
}
}
return $values;
}
Tested and reviewed <3 |
Had exact same issue and works as expected! Would love to see this committed. |
Likewise - this PR fixes the issue #200 where the Given Content... step fails to fill in the datetime field if that field is date-only. |
The patches on issue #200 where either conflicting or just not working, this resolves it for latest master.