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.

Sign up for GitHub

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

Daterange handler #2 #167

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

Daterange handler #2 #167

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/Drupal/Driver/Fields/Drupal8/DaterangeHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

namespace Drupal\Driver\Fields\Drupal8;

/**
* Daterange field handler for Drupal 8.
*/
class DaterangeHandler extends AbstractHandler {

/**
* {@inheritdoc}
*/
public function expand($values) {
$return = [];
foreach ($values as $value) {
// Allow date ranges properties to be specified either explicitly,
// or implicitly by array position.
if (!isset($value['value']) && isset($value[0])) {
$value['value'] = $value[0];
}
if (!isset($value['end_value'])) {
if (isset($value[1])) {
$value['end_value'] = $value[1];
}
else {
// Allow end value to be optional (D#2794481).
$value['end_value'] = NULL;
}
}
$start = str_replace(' ', 'T', $value['value']);
$end = str_replace(' ', 'T', $value['end_value']);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that this will work as expected in all use cases. For example this wouldn't work with time formats like "December 1, 1987" or "now +1 hour".

I'm not sure if it is even the scope of DrupalDriver to "fix" bad input. My gut feeling says that if the time format is not in the expected format we should throw an exception here. It seems more like the responsibility of the API consumer (e.g. DrupalExtension) to make sure the data is in the correct format.

But then again, DrupalDriver can realistically be expected to be able to deal with all kinds of date formats that are used by Drupal versions 6-8 (and future versions). So maybe it is a good idea to support multiple time formats here.

How about something like:

foreach (['value', 'end_value'] as $key) {
  if (!empty($value[$key])) {
    $time = strtotime($value[$key]);
    if ($time === FALSE) {
      throw new \LogicException();
    }
    $value[$key] = $time;
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this wouldn't work with time formats like "December 1, 1987"

that's a problem with the D8 datetime handler too;

or "now +1 hour".

relative datetime support was recently added to the D8 handler; I didn't add it to the daterange handler as I was being quick and dirty.

I'm not sure if it is even the scope of DrupalDriver to "fix" bad input.

Yes, there are fundamental questions about how liberal the driver should be with its inputs. It comes up with the entity reference handler where we allow reference by labels, and it comes up with the question of whether we should allow specifying labels as well as machine names in some other contexts.

The best context for discussing that is #155, the new plugin system. We need to decide if the Drupal driver provides basic plugins that are extended and overridden by more helpful human-friendly plugins from the Drupal extension, or if its just simpler to have only one set of human-friendly plugins that live in in the driver.

My suggestion is

  1. to accept this PR as is, as it's not worse than the datetime handler and better than the nothing we currently have for people who are massaging their inputs in custom code.
  2. answer the fundamental question of liberal inputs in Entity & Field plugins #155
  3. start a new issue to discuss the ideal datetime logic we want to have that applies (conceptually and DRY) to both datetime and daterange fields.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about extending DateTimeHandler so that the relative date handling is shared?

E.g something like:

    foreach ($values as $value) {
      // Allow date ranges properties to be specified either explicitly,
      // or implicitly by array position.
      if (!isset($value['value']) && isset($value[0])) {
        $value['value'] = $value[0];
      }
      if (!isset($value['end_value'])) {
        if (isset($value[1])) {
          $value['end_value'] = $value[1];
        }
      }

      // Filter out NULL values to allow end value to be optional.
      $return[] = parent::expand(array_filter($value));
    }

Copy link

@ericgsmith ericgsmith May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've given that approach a quick test ericgsmith@f59eecb and works for me setting relative dates.

Appreciate this is a old MR - but given the suggested scope didn't get merge, would be keen to get feedback on that approach.

$return[] = [
'value' => $start,
'end_value' => $end,
];
}
return $return;
}

}