Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

drishu
Copy link

@drishu drishu commented May 10, 2021

The patches on issue #200 where either conflicting or just not working, this resolves it for latest master.

@drishu drishu changed the title EWPP-1104: Fix date only. Fix date only field not working. May 10, 2021
$date->setTimezone($storageTimezone);
$values[$key] = $date->format('Y-m-d\TH:i:s');
if ($this->fieldInfo->getSetting('datetime_type') === 'date') {
$date = new \DateTime($value);
Copy link
Contributor

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.

Copy link
Author

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

Copy link
Author

@drishu drishu May 10, 2021

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.

Copy link
Contributor

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?

Copy link
Contributor

@ademarco ademarco May 17, 2021

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;
  }

@BramDriesen
Copy link

Tested and reviewed <3

@timdiels1
Copy link

Had exact same issue and works as expected! Would love to see this committed.

@cboyden-ucb
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants