Allow end date to be optional

TBH, I am not terribly keen on this, because of the ripple effects is has. Any consideration with this will need to wait until some of the refactoring we need to do gets wrapped up, the views filters get finished, and the timezone work is finalized. I really want to avoid having to do an option check in every single method (in addition to the date / date+time one we already do, which is tech debt we have to deal with).

it certainly is expectable that an Event will have a start time and may not have a fixed end time.. this is a normal use case.

for both UX and data validation the good old 'has end date' checkbox may get good to bring back

I guess the thing is that we now have two fields, datetime and daterange. If we make end dates optional in daterange, then we are essentially making the datetime field not needed because daterange will have all of it's features. I'm not saying we can't do this, just that this really does need to wait for other planned work to shake out first.

Marking postponed, in that case. But I do agree that this seems like pretty essential functionality.

#4.
I certainly get we have both fields. I think the split to datetime_range during the last phase of dev caused this. indeed there is a lot of tech dept here. re-integration of these types would be more idea in the long run.

the sort term answer for site builders is to use a conditional field state for single or end-date ready. which always makes getting views, node renders and other related queries of dates horrible cause you have to check two fields. but at least now, there is this sort term approach.

As a workaround, I ended up defining a new FieldType class in a custom module that extends the DateRangeItem class, and setting Required to false there. I then used hook_field_info_alter to assign that new class to the daterange field. That allowed the end date to be optional. But since we don't actually want the end date to be left empty, I used hook_entity_presave to look for daterange fields and use the start date as the end date if the end date is left empty. In our theme, any daterange field with the same start date and end date is rendered as a single date.

The new class:

<?php

namespace Drupal\my_module\Plugin\Field\FieldType;

use Drupal\Core\Field\FieldStorageDefinitionInterface;
use Drupal\Core\TypedData\DataDefinition;
use Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem;

/**
 * Plugin implementation of the 'daterange' field type.
 *
 * @FieldType(
 *   id = "daterange",
 *   label = @Translation("Date range"),
 *   description = @Translation("Create and store date ranges."),
 *   default_widget = "daterange_default",
 *   default_formatter = "daterange_default",
 *   list_class = "\Drupal\datetime_range\Plugin\Field\FieldType\DateRangeFieldItemList"
 * )
 */
class MyModuleDateRangeItem extends DateRangeItem {

  /**
   * {@inheritdoc}
   *
   * The entire reason this class exists is to overwrite the Required property
   * of end_value. It is only safe to do this because in this module we fill the
   * end_value with the start_value when left empty. Keep an eye on:
   * https://www.drupal.org/node/2794481.
   *
   * @see my_module_agency_entity_presave();
   */
  public static function propertyDefinitions(FieldStorageDefinitionInterface $field_definition) {
    $properties = parent::propertyDefinitions($field_definition);

    $properties['end_value'] = DataDefinition::create('datetime_iso8601')
      ->setLabel(t('End date value'))
      ->setRequired(FALSE);

    return $properties;
  }

}

The field info alter and presave hooks, in my_module.module:

/**
 * Implements hook_field_info_alter().
 *
 * Take over the DateRangeItem class from datetime_range so that we can remove
 * the requirement on end_value.
 */
function my_module_field_info_alter(&$info) {
  if (isset($info['daterange']) && class_exists('Drupal\my_module\Plugin\Field\FieldType\MyModuleDateRangeItem')) {
    $info['daterange']['class'] = 'Drupal\my_module\Plugin\Field\FieldType\MyModuleDateRangeItem';
  }
}

/**
 * Implements hook_entity_presave().
 *
 * Populate end date on daterange fields with start date if empty.
 */
function my_module_entity_presave(EntityInterface $entity) {
  // Limiting this to nodes for the time being. But If a daterange field is used
  // on a different entity type and no end date is supplied, I think the site
  // might blow up, so figure that out.
  if ($entity->getEntityTypeId() !== 'node') {
    return;
  }

  $daterange_fieldnames = array();

  // Fill $daterange_fieldnames array with fieldnames of each daterange field.
  $fields = $entity->getFieldDefinitions();
  foreach ($fields as $fieldname => $field) {
    if (substr($fieldname, 0, 6) !== 'field_') {
      continue;
    }
    $fieldtype = $field->getType();
    if ($fieldtype !== 'daterange') {
      continue;
    }
    $daterange_fieldnames[] = $fieldname;
  }

  // Handle each daterange field.
  foreach ($daterange_fieldnames as $daterange_fieldname) {
    $daterange_field = $entity->get($daterange_fieldname);
    if ($daterange_field->isEmpty()) {
      continue;
    }

    $start_value = $daterange_field->get(0)->get('value')->getValue('value');
    $end_value = $daterange_field->get(0)->get('end_value')->getValue();

    // If end_value is left empty, set it same as start_value.
    if ($end_value === NULL || $end_value === '') {
      $daterange_field->get(0)->get('end_value')->setValue($start_value);
    }
  }
}

This isn't perfect and I'm sure it could be improved (please, do tell). But it seems to be working for us for the time being, until a more permanent solution is put in place.

Echoing Webchick in #5 with a use case I was working on when I hit this issue: I have a content type for alerts which has two levels for alerts, low and high. I know about low level alerts ahead of time and can schedule them to publish with a date range ("Elm Dr. closed for pothole repairs from 10:00 a.m. to 2:00 p.m.). In this case the date range lets me set and forget. The high level alerts come unplanned and continue for some time (I get a call that a building is on fire). In this case I have no end date because the alert is ongoing.

It would be great if the end date is optional for cases like this.

Thanks @dkosbob for the workaround. Just as a note, this only works for optional daterange fields, since it doesn't adapt the field widgets and the start and end date required flag are based on the field required property. Combined with browser based validation for required elements, you'd still have to fill it out.

No end date isn't that the end date is the same that the start date? Don't know if they will be side effect on this approach. It's what dkosbob is doing.

But yes perhaps we really want to have the information that their is no information.

+1 on this FR, in my current project I've exactly the use case as described in #3. As a workaround I had to move to two separate date fields, but I'm missing the endtime validation and the slick UI

Now that Date Range is done, can we bring this back to life by considering #4?

Possible steps:

1. Make the end date optional in Date Range.
2. Deprecate Date in D8.
3. Rename Date Range to Date in D9.

I'd entertain work on this now, but I consider it soft blocked on the views issues and maybe the time zone one. I also think we need to refactor the functional tests to be more manageable to avoid reroll madness. So, let's play with play with the non-test changes and get feedback on that before test coverage.

Right now, I think I am ok with two fields with some overlapping functionality.

+1

As webchick already stated: this is essential functionality. I'll try to give an example: a calendar of mixed events, some of them lasting several hours, others several days. Of again others is not known, except the door opening time, how long they will really last. So, if the end-value is a mandatory field, this can lead to incorrect information on the site, because the editor is forced to name an end time that he does not even know.

A little workaround that works for me :

Create a php file in your module :

MODULE_NAME/src/Plugin/Field/FieldWidget/DateRangeCustomWidget.php

<?php

namespace Drupal\datetime_custom\Plugin\Field\FieldWidget;

use Drupal\Component\Utility\NestedArray;
use Drupal\Component\Utility\SortArray;
use Drupal\Core\Entity\EntityStorageInterface;
use Drupal\Core\Field\FieldDefinitionInterface;
use Drupal\Core\Field\FieldItemListInterface;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
use Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem;
use Drupal\datetime_range\Plugin\Field\FieldWidget\DateRangeWidgetBase;
use Symfony\Component\DependencyInjection\ContainerInterface;

/**
 * Plugin implementation of the 'daterange_custom' widget.
 *
 * @FieldWidget(
 *   id = "daterange_custom",
 *   label = @Translation("Date and time range custom"),
 *   field_types = {
 *     "daterange"
 *   }
 * )
 */
class DateRangeCustomWidget extends DateRangeWidgetBase implements ContainerFactoryPluginInterface {

  /**
   * The date format storage.
   *
   * @var \Drupal\Core\Entity\EntityStorageInterface
   */
  protected $dateStorage;

  /**
   * {@inheritdoc}
   */
  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, array $third_party_settings, EntityStorageInterface $date_storage) {
    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $third_party_settings);

    $this->dateStorage = $date_storage;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static(
      $plugin_id,
      $plugin_definition,
      $configuration['field_definition'],
      $configuration['settings'],
      $configuration['third_party_settings'],
      $container->get('entity_type.manager')->getStorage('date_format')
    );
  }

  /**
   * {@inheritdoc}
   */
  public function formElement(FieldItemListInterface $items, $delta, array $element, array &$form, FormStateInterface $form_state) {
    $element = parent::formElement($items, $delta, $element, $form, $form_state);

    // Identify the type of date and time elements to use.
    switch ($this->getFieldSetting('datetime_type')) {
      case DateRangeItem::DATETIME_TYPE_DATE:
      case DateRangeItem::DATETIME_TYPE_ALLDAY:
        $date_type = 'date';
        $time_type = 'none';
        $date_format = $this->dateStorage->load('html_date')->getPattern();
        $time_format = '';
        break;

      default:
        $date_type = 'date';
        $time_type = 'time';
        $date_format = $this->dateStorage->load('html_date')->getPattern();
        $time_format = $this->dateStorage->load('html_time')->getPattern();
        break;
    }

    $element['value'] += [
      '#date_date_format' => $date_format,
      '#date_date_element' => $date_type,
      '#date_date_callbacks' => [],
      '#date_time_format' => $time_format,
      '#date_time_element' => $time_type,
      '#date_time_callbacks' => [],
    ];


    $element['end_value'] += [
      '#date_date_format' => $date_format,
      '#date_date_element' => $date_type,
      '#date_date_callbacks' => [],
      '#date_time_format' => $time_format,
      '#date_time_element' => $time_type,
      '#date_time_callbacks' => [],

    ];

    $element['end_value']['#required'] = false;

    return $element;
  }

  /**
   * {@inheritdoc}
   */
  public function extractFormValues(FieldItemListInterface $items, array $form, FormStateInterface $form_state) {
    $field_name = $this->fieldDefinition->getName();

    // Extract the values from $form_state->getValues().
    $path = array_merge($form['#parents'], [$field_name]);
    $key_exists = NULL;
    $values = NestedArray::getValue($form_state->getValues(), $path, $key_exists);


    if ($key_exists) {
      // Account for drag-and-drop reordering if needed.
      if (!$this->handlesMultipleValues()) {
        // Remove the 'value' of the 'add more' button.
        unset($values['add_more']);

        // The original delta, before drag-and-drop reordering, is needed to
        // route errors to the correct form element.
        foreach ($values as $delta => &$value) {
          if( $value['end_value'] == null )
            $value['end_value'] = $value['value'];
          $value['_original_delta'] = $delta;
        }

        usort($values, function ($a, $b) {
          return SortArray::sortByKeyInt($a, $b, '_weight');
        });
      }

      // Let the widget massage the submitted values.
      $values = $this->massageFormValues($values, $form, $form_state);

      // Assign the values and remove the empty ones.
      $items->setValue($values);
      $items->filterEmptyItems();

      // Put delta mapping in $form_state, so that flagErrors() can use it.
      $field_state = static::getWidgetState($form['#parents'], $field_name, $form_state);
      foreach ($items as $delta => $item) {
        $field_state['original_deltas'][$delta] = isset($item->_original_delta) ? $item->_original_delta : $delta;
        unset($item->_original_delta, $item->_weight);
      }
      static::setWidgetState($form['#parents'], $field_name, $form_state, $field_state);
    }
  }





}

In formElement function I set end_value to required false
In extractFormValues i set end_value the same value as start_value if null.

I've made a workaround module, we're using while this issue is being resolved: https://www.drupal.org/project/optional_end_date

So far it's only implemented in a project still under development, so it's not yet tested on a live site.
It supports null values for the end date, so the we can work with date ranges without a specific end date (the default output is the same as with start and end being identical).

I think the best solution is to make the user CHOOSE either a temporary "Present" end date (as a checkbox) OR a fixed end date.
With JavaScript, when the user clicks on the checkbox, the end date input fields are hidden and the word "Present" is shown instead.

New York - 2016.04 - Present
London - 2011.02 - 2016.04

So either way, we still have a date RANGE (not a simple date in the case the end is optional OR a date range as it is now).
With this "Present" checkbox, we can always calculate the duration of the event. We can always enter a final date instead of "Present" by unchecking the checkbox.

The word "Present" should of course be changeable and translatable.

The #22 suggestion doesn't address what I see as one of the most common scenarios here:
describing a future event whose start date is known and whose end date is not (yet) know. 'Present' is completely inadequate in this case.

The most critical problem is: how to set things up so that content editors don't have to enter false information when they don't know the information.

#21 posting your work as a patch against 8.6, too, would help this feature more forward.

See my comment below.

I have posted a reply which it is held for approval, so here is a better version of it:

Having the OPTION to show in the Frontend "a Date Range + a checkbox" instead of just "a Date Range" should solve the problem.

And when #2942828: Make the Date and Date Range configurable to allow MIN and MAX dates, only past or only future dates, or both is implemented, the date range will be very flexible.

My suggestion from #22 covered only one part: events which started in the past and are still happening (Present).

To also cover the other part, events with an unknown end (past unknown or future unknown), we can improve that suggestion.
In the Backend, in the date range type settings we can add an option to show Date Range + "Unknown end date" OR "Present".
If the end date was selected as Unknown, there needs to be an OPTION to show ONLY the start date in the Frontend when displaying the event's start and end dates (to hide the word Unknown).

In the Frontend, the user sees either:

Choose Start Date - Choose End Date + a checkbox "Unknown end date"
or
Choose Start Date - Choose End Date + a checkbox "Present"

When the checkbox is checked the "Choose End Date" field is hidden and replaced by the word "Present"(or "Unknown") using JavaScript.
So the "Choose End Date" field depends on the checkbox.

From what I can see, the modules for D8 which have implemented conditional fields are select_or_other, conditional_fields and business_rules.
I have tested only the last 2 and in both the checkbox selection doesn't work properly yet.

The attached patch implements what I've done in the optional_end_date module (mentioned in #21).

The idea is to optionally (checkbox added to the storage settings of the daterange field) allow NULL values for the end_value.
Default behavior for the formatters, when the end date is not filled, is to act as if start and end is equal (only show the start date).

It's purely functional, I've not looked at or altered any tests, it's a take on a possible solution.

Go testbot, go!

@Birk
It's great to see there is progress on this issue.
Would you consider also adding an option to show the checkbox in the front-end like I suggested in #26?
It would certainly make things more simple to understand for the users who fill the data.
So, in the front-end, there will be a choice between entering an end date or clicking a checkbox (which will hide the end date input fields).

+++ b/core/modules/datetime_range/src/Plugin/Field/FieldType/DateRangeItem.php
@@ -83,6 +92,12 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
+      '#title' => t('Optional end date'),
+      '#default_value' => $this->getSetting('optional_end_date'),

Not sure if I like this label, but that is a nit.

NW for needing test coverage. It is possible, though, that we want to refactor DateRangeFieldTest first, since that is a beast of a test class. Adding coverage for datetime, dateonly, and allday for the widgets + formatters is going to nearly double the length of that class.

I've noticed that when I set the field as required, even with optional end date, the end date is required in the front-end.
I think what @dalra proposes is maybe a bridge too far, but when the end date is optional, it shouldn't be required in the front-end.

My original proposal could be simplified (we do not keep the present day anymore) and reduced to this:

In the back-end, in the daterange options, next to the "Optional end date" checkbox, add another checkbox called "Show a checkbox in the front-end to hide the end date and show a label"
Of course, when this is checked in the back-end, the front-end user will see:

Input start date + input end date + a checkbox to hide the end date (eg: with the label "Unknown end date" or "Ongoing")

The label should be configurable in the back-end.

So in the fronted, when the user checks the "Unknown end date" checkbox, the end date input field is hidden with JavaScript and the label "Unknown end date" is shown.
If they unchecked it, the end date is shown again.
This eliminates any confusion the front-end user might have.
These 2 checkboxes (one in the back-end to show another one in the front-end) solve the problem.

If this checkbox is not implemented, it's just a bad user experience:
1) the user doesn't know if the end date is required
2) with only start date + end date, for an ongoing event, (like "Art gallery in New York": 2016.04 - still open) the user doesn't know what to do, if they should enter today's date (the wrong choice) or not to enter anything (the user is uncertain if the submission will work).
In this case the chosen label is "still open" instead of "Unknown end date".

+++ b/core/modules/datetime_range/src/Plugin/Field/FieldType/DateRangeItem.php
@@ -46,7 +55,7 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
-      ->setRequired(TRUE);
+      ->setRequired(!$field_definition->getSetting('optional_end_date'));

I believe this means we will need an update path. Not adding that tag until we have a solid approach, though, so we know what we will need. But, I think that is what caused the required problem described in #31.

The patch in #27gave me a fatal on a clean install against 8.6.x when I tried adding the node:

The website encountered an unexpected error. Please try again later.Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'field_daterange_end_value' cannot be null: INSERT INTO {node__field_daterange} (entity_id, revision_id, bundle, delta, langcode, field_daterange_value, field_daterange_end_value) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6); Array
(
[:db_insert_placeholder_0] => 1
[:db_insert_placeholder_1] => 1
[:db_insert_placeholder_2] => page
[:db_insert_placeholder_3] => 0
[:db_insert_placeholder_4] => en
[:db_insert_placeholder_5] => 2018-02-01
[:db_insert_placeholder_6] =>
)
in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 829 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).

Drupal\Core\Database\Statement->execute(Array, Array) (Line: 625)
Drupal\Core\Database\Connection->query('INSERT INTO {node__field_daterange} (entity_id, revision_id, bundle, delta, langcode, field_daterange_value, field_daterange_end_value) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6)', Array, Array) (Line: 87)
Drupal\Core\Database\Driver\mysql\Connection->query('INSERT INTO {node__field_daterange} (entity_id, revision_id, bundle, delta, langcode, field_daterange_value, field_daterange_end_value) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6)', Array, Array) (Line: 32)
Drupal\Core\Database\Driver\mysql\Insert->execute() (Line: 1341)
...

Re the approach described in #32, not sure if I like the NULL-prints-label thing or not.

Any approach we take will also need a proper Usability Review and an Accessibility Review.

Re the approach described in #32, not sure if I like the NULL-prints-label thing or not.

Well, #32 can be improved.

We show another checkbox in the back-end to make the JavaScript label optional.
Some users might want to just hide the end date input field when the box is checked, without showing a label. This will show only the start date and hide the end date.

Those who need the end date label, can enable it.

We have 2 labels here:
1) next to the front-end checkbox. This could be short (1-2 words) because it's next to the checkbox.
2) one shown instead of the end date field. Here we can put a few more words to make sure the user understands

We need to make both labels configurable in the back-end.
Some users might put the same text in both, but some might want to put a synonym, to make sure the front end user gets it

+++ b/core/modules/datetime_range/src/Plugin/Field/FieldType/DateRangeItem.php
@@ -46,7 +55,7 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
-      ->setRequired(TRUE);
+      ->setRequired(!$field_definition->getSetting('optional_end_date'));

Reading through some of the earlier patches of #2161337: Add a Date Range field type with support for end date may shed some light on this problem, especially if some are seeing this work and other aren't (as it may appear). In that, we had problems using a dynamic schema in `DateRangeItem::propertyDefinitions()` based on the `$field_definition` settings. IIRC, this was because of when this method was invoked at different places (I would have to set some breakpoints again to be sure). I think we will have to `->setRequired(FALSE)` in the field definition, and then set required on the element in the widgets, based on the settings. Still need a update hook at the end, though.

Re #34, I am probably being dense, but I don't quite understand what is being described. A few things to keep in mind with this are

- JS can enhance the UX, but the UX and A11Y need to work properly w/o it. This is a firm core gate.
- With the datetime related widgets/formatters, we try to avoid the do-it-all scenario, and focus instead on building blocks that provide functionality, and then can be enhanced in contrib. We also don't need to do everything all at once. Optional end time has been identified as good additional functionality. I am leaning towards #32/#34 being deferred to a followup or contrib.

I have created some mockups from the patch in #27 and from my suggestions in #32 and #34.

When checkbox 3 is checked:

@dalra: I think most of what you suggest is overkill for a lot of use cases, and should be handled in follow-up issues. The basic premise here should be: let's make it possible to have the end date optional.
What you suggest is a huge impact on what needs to be stored, would need a lot of reviews, e.g. for accessibility. While optional end date should be do-able quicker and easier.

We talked about the proposal at #36 on the weekly usability meeting. There was consensus that the options provided in these mockups sound like a potentially neat contrib module like "date_time_advanced_config", but that for core, which aims for hitting the "80%" case, these sorts of options would feel like overkill.

Rationale:

- For all other Drupal fields that offer a "required" vs. "optional" (e.g. text, number, and so on), the data entry experience is to just leave the optional field blank and then it won't get printed upon display. Other fields do have the ability to define a default value, but not a "what to print here if someone leaves it blank" value, and that seems odd to add to this one field and not others.

- This also seems to match what in a quick check Google Calendar, Meetup, Facebook Events, iCal, etc. do. Those services don't actually let you pick an optional end date... they default to the current one, and if the from + to date match, they simply print the from date (this seems like very sensible behaviour for Drupal as well). There are no options to choose customized text to show instead; that would complicate the data entry quite significantly.

So, given the other Drupal fields and these other popular event management services will demonstrate generally expected behaviour, and the closer we can match them, the better off we'll be, I'm inclined to agree with @daften on this proposal being out of scope. But once again, feel free to create a contributed module that offers these options, if you feel people would benefit.

@daften
If the end date is optional, the only thing that is stored instead of it is NULL.
So there's no impact here.

In the mockups, I only suggested a few very simple things:

1) Show a simple checkbox in the front-end to improve usability and user experience.
2) Give a name (a label) to that checkbox. Don't show a box without a name.
3) Have an option to show a label when the end date is hidden. It's very useful also to the people who use screen readers.
4) When you want to show in the frontend a date range which already has a NULL end date, have an option to show the label from 3) instead of NULL.

I really don't think that an optional label and (1 checkbox plus its name) would require much effort in terms of coding.

While optional end date should be do-able quicker and easier.

If you have any suggestions please share them.
However, as it is now, with only the patch from #27, won't pass any usability or use experience tests.
I gave an example in #32.

@webchick

I saw your reply only after my previous post.

Please consider the events that have started in the past which do not yet have an end date (are still happening).
In this case, the users might enter the current day as the end date. That would be totally wrong, because it would provide a end date even though there is none yet.

Eg: How long have you been living in that city?

From 2016.10 to:
a) (Enter today's date - wrong choice - OR don't enter anything). This is confusing, leaves the user guessing what to do.
b) a checkbox: I'm still living here (of course the date is saved as NULL and we just show a label for NULL)

@dalra, if there isn't much effort in terms of coding, I'd advice to make a patch :)

The coding part is often not the most difficult part, it's the usability and accessibility that are the difficult points sometimes. This also applies to the editor interface. Having the basic functionality of an optional end date (as it was there in D7) will be difficult enough. I'm not saying, and neither is @webchick, that your suggestions are invalid. I'd just advice you to put them in a different (follow-up) issue. This would help keep the basic discussion here clean and ONLY about optional end dates.

Those services don't actually let you pick an optional end date... they default to the current one, and if the from + to date match, they simply print the from date (this seems like very sensible behaviour for Drupal as well).

What Google calendar also does update the end date every time you update the start date, to keep the offset from the start date constant. This makes editing dates much less annoying.

Perhaps we don't need to allow the end date to be null, maybe it's OK to
1) simply default the initial end date to equal the start date (which we probably do already)
2) change the end date when the start date is changed to preserve the relationship
3) make the formatters show just the start date when the end date equals the start date

This makes it easy for a sitebuilder to setup a fairly good editorial experience.

Setting the end date to equal the start date seems an acceptable way of communicating "the idea of an end date doesn't apply here". Allowing NULL would be a way of allowing sitebuilders to say "this could have an end date but I don't know it"; but that doesn't seem like an 80% use case.

There's no doubt that if there was a situation where end dates were frequently not needed for a certain date field, it would be a better UX to have a checkbox like @dalra describes. But it's very easy to implement that in a contrib widget. The simplest possible version of @dalra's suggestion, which might be worth considering in core is to have a widget setting "Don't show end date input initially"; if checked it would hide the end date input using only css unless an "Add end date" link was toggled. But anyway, definitely this kind of UX refinement can definitely be postponed to a separate issue.

#43, (1) and (2) are in #2863444: Discourage/make impossible to select a "to" date that is before the "from" date, which mostly works, but needs tests. (3) should be the default behavior in core already (see DateTimeRangeTrait::viewElements() and I know we have coverage in DateRangeFieldTest for this).

How about something like this? We still need something that works w/o JS, so #2863444: Discourage/make impossible to select a "to" date that is before the "from" date can only be an enhancement, not a solution.

It is also worth mentioning that the only correct way to test this patch is to apply it it 8.6.x, then do a site install. Without the update path, the database schema / field definition will be wonky.

Added some extra logic to the #45 patch, that will require the end date, if optional_end_date is not selected.

Should the optional_end_date setting be moved to the field config instead of the field storage config, since it doesn't alter the storage anymore?

Should the optional_end_date setting be moved to the field config instead of the field storage config, since it doesn't alter the storage anymore?

I'm included to keep it with storage so that the `hasData` check prevents it from being changed. Worried that if we get NULL values in storage and someone changes optional => FALSE, it will cause weirdness.

#48 Fair point.

Added an update hook, that changes the end_value column of existing fields.

After the update the "Status report" page had an error with "Mismatched entity and/or field definitions" on the fields updated.
Instead of changing the schema directly (like #49), this patch simply saves the field definition again, and because the definition is updated the database schema is updated as well.

I'm not sure this is the correct way of handling this, I've been looking at EntityDefinitionUpdateManager::applyUpdates(), but it updates everything that is mismatched.

I haven't been able to find a best practice for updating existing fields in a hook_update_N, The one I've used in this patch seems to be the least intrusive, that wont result in a mismatch error, but maybe someone can chip in if there's something obvious I've missed.

This is looking nice.

There is an update hook in #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken, but that one is really complicated and I haven't tried to simplify it yet. There aren't a lot of examples in core.

I think the next step would be an update path test to check the assumptions in the update hook. That will require a fixture. I would start with the fixture in #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken, and then add three daterange fields (date+time, date-only, allday) and a few nodes, and add some assertions. If we coordinate the fixture between the two issues, it will minimize having to add them to core and whichever patch lands first will be able to use the other.

Then that should settle the mechanics of the change, and we can focus on the UX.

It turned out simply saving the StorageConfig didn't update the database (I must have been testing it wrong when I made it).
So I've updated the update hook to use the changeField() function again. And then manually update the storage_schema.

I've used the fixture from #2716891 as a base, and added 3 daterange fields to the node (allday, date and datetime). I've also added a daterange field a taxonomy_term to test not revisionable fields as well.

It's my first ever test, so it took me a while figuring out some of the magic, and I might very well have done something the "wrong way". Please point me in the right direction, if the test is a lackluster or otherwise in bad form.

Fixed an issue, where no start or end date was filled (and the field wasn't required), but a form error was called anyway.

I've also fixed the code style, so it follows the Drupal standard.

I wonder if this option could be configured for each content type, instead of in the field setting?
Then you can make the end date optional for some content types, and mandatory for others.

As far as I know, field settings are per-field, by convention. So, I'm going to assume this needs to be tested.

1. Reproducing: enable Datetime Range. Create a new date range field on the article content type. Creating an article with a start date, but no end date: successfully reproduced reported issue. I receive the error This value should not be null..
2. Applying patch from #53: curl -l https://www.drupal.org/files/issues/2018-05-03/2794481-53.patch | git apply -v. Received some errors I don't fully understand:
   

   warning: squelched 7 whitespace errors 
   warning: 12 lines add whitespace errors.

   Given that this reads like a non-functional issue, testing the patch anyway

3. Clearing cache through UI, [tangent] since I'm using the new core quick-start command, and apparently drush cr doesn't work with SQLite. Actually.. wee.. looks like clearing it through the UI also causes problems (forever spinning). Maybe quick-start isn't a good way to do core issue testing. Restarted via Ctrl+C and re-running quick-start to turn the PHP server back on. [/tangent]
4. Going back to node add article page, and trying to add start date but no end date: still cannot, same error as step 1.

Not marking this as RTBC, since the patch doesn't seem to work, and also looks like there might be whitespace issues. Re-queing latest patch for testing, and marking as "Needs reroll" because I think that's what needs to happen here? Sorry in advance if I'm wrong.

btw this issue has a ton of tags on it.. are they all still applicable?

I've tested the patch on a normal (not quick-start) installation, and it seems to work. It seems you're missing a few steps in the reproduction, I've done the following:

1. Clean installation of Drupal, with the datetime_range module enabled.
2. Create new Date range field on the article content type.
3. Try to create new article with a start date, but no end date. Get the null error, as expected.
4. Apply the patch.
5. Update the database /update.php or drush up.
6. Change the Date range field storage settings so it allows optional end date.
7. Try to create new article with a start date, but no end date. Results in the node is created.

The patch warnings are all caused by the core/modules/datetime/tests/fixtures/update/dump.php.gz file, but the code is applied correcly. I don't know how to fix these warnings, should I create the patch in a different way?

Sorry I have been AWOL. Life got in the way. Few quick things.

This still needs tests. I am only seeing an update test in the patch. At a minimum we need updates in DateRangeItemTest and DateRangeFieldTest, but considering the later is 1300+ lines of mess, we may want to figure out some refactoring in a side issue. I need to review the status of the constraints and some of the other REST issues to see where we land.

This needs manual testing, both on a clean install (to test the patch itself) and on an existing install w/ data to test the update path.

We can ignore coding issues in the fixture.

Given that the usability of the widgets was a bug problem with getting this in core, it will need a proper Usability Review by that team.

@Birk I missed the update.php part! Thanks for pointing that out.

I've moved the end date validation out of the DateRangeWidgetBase::validateStartEnd() and into DateRangeItem::getConstraints() instead (which seems like a better place for this anyway). So the entityValidateAndSave function can be used in the test.

These are the first tests I've written, and it seems scarce, but I lack the experience of figuring out how much and exactly what to test, I've included what I felt meaningful.

Should we wait with the DateRangeFieldTest till refactoring is decided?

trigger test

I want to emphasize that most of the comments here relate to END DATE, but the solution should be symmetrical, meaning the user might want to fill ONLY the end date and leave the start date empty.

for example

Describe the period when people didn't have vaccination for smallpox ("always" until 1796)

I just want to give everyone the heads up that the 'datetime_iso8601' datatype has never been set to optional in core. I discovered this because someone created #3017945: [upstream] Field types with a required property *and* an optional @DataType=datetime_iso8601 property trigger fatal error in DateTimeNormalizer when optional property is empty.

We might see some minor regressions when we change the date range field's start and end date to optional. I don't this is a big deal.

The JSON API might have to allow for an optional 'datetime_iso8601' datatype.

#63
This is in interesting use case Although it has as much todo with how that field label is phrased and how that date is then interpreted when displayed. I imagine your content type of 'medicines' would have a 'date invented'. Then referencing it to say 'before then there was none' in a regular sentence is a normal human expression of that timeline. In so, your 'date invented' field would never have an end date -- unless mankind lost the formula (:

#63
+1

I feel the necessity that blank spaces are possible on the start date as well.
There are times when you do not want to display the start date intentionally and may not know for a long time like history.
The end date is also similar in thinking abstractly.

Regarding the concept of start and end, we have similar concerns about time.
I feel that it is necessary to satisfy the four conditions.

single
Start only
End only
period

These repetitions(Multivalue)

It is inconvenient to select the date, date and time in the field setting.
I want to use multivalued with no time and time.


I appreciate your discussion and development!

First, thank you for the great work.

Concerning path #60 I noticed two errors.

1 - it creates a core folder in the Drupal core folder, containing a modules folder. If I am not wrong the changes should be made exclusively in the modules folder in the original Core folder.

2 - I had to create a second patch in order to see the end date empty in the node edit form when optional and empty. With patch #60 the end date is set on start date.

I had to change from

if ($items[$delta]->end_date)

to

if ($items[$delta]->end_date && $items[$delta]->start_date != $items[$delta]->end_date)

on line 44 in DateRangeWidgetBase.php

@Penef:

I am not sure about your second point, but I think I can explain the first one.

This patch does not add the code/core/ subdirectory. You can confirm that by checking out a fresh copy of Drupal's git repository and applying the patch with either git apply -v path/to/2794481-60.patch or patch -p1 < path/to/2794481-60.patch.

I am guessing that you are using composer to manage your site, and you are running into a bug with older versions of the composer patches plugin (cweagans/composer-patches). Try upgrading that to the latest version (currently 1.6.5) and update your composer.json as in the current version of drupal-composer/drupal-project.

References:

• https://github.com/cweagans/composer-patches/issues/43
• https://github.com/drupal-composer/drupal-project/issues/412

@benjifisher

It is the first time I have this kind of problem (even on recent patches), but you are right I am using composer to apply the patch.

cweagans/composer-patches is already at 1.6.5 on my installation, but the problem persists.

It is eventually an error in the way I applied the patch, but as I use the same way on various projects without encountering this error, I was guessing it might be the patch.

Thank you for the answer.

Second point is still valid (on my side).

Best regards

@Penef:

Please see the second link I quoted. The latest version of cweagans/composer-patches (1.6.5) adds a new option, but if your configuration does not use that option, then you will still apply patches with the wrong patch level.

As I said in #68,

... and update your composer.json as in the current version of drupal-composer/drupal-project.

Specifically,

    "extra": {
        "patchLevel": {
            "drupal/core": "-p2"
        },
...
    }

The patch already have automated tests, update path, and update path tests.

Hi!

With the comment in #70, patch is applied properly.

Related with the patch itself, in #52/#53 it was added a new file (core/modules/datetime/tests/fixtures/update/dump.php.gz) and is there.
Is that file neccessary? It seems to work without the file being there.

@gorkagr this file is used for preparing the testing environment (the database) and having a controlled scenario for executing the tests.

Tested #60 and worked on 8.7. Thanks!

1. --- /dev/null
   +++ b/core/modules/datetime/tests/fixtures/update/dump.php.gz
   
   +++ b/core/modules/datetime/tests/fixtures/update/dump.php.gz
   @@ -0,0 +1,530 @@
   

   Neds to be in datetime_range.

2. +++ b/core/modules/datetime_range/datetime_range.install
   @@ -0,0 +1,30 @@
   +function datetime_range_update_8600() {
   +  $database_schema = \Drupal::database()->schema();
   

   Need to update to an 880X hook.

3. +++ b/core/modules/datetime_range/src/Plugin/Field/FieldType/DateRangeItem.php
   @@ -44,9 +53,10 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
   +    // @todo Write a comment on why we can't use the $field_definition here.
        $properties['end_value'] = DataDefinition::create('datetime_iso8601')
   

   We either needs to resolve this @todo or remove the comment.

4. +++ b/core/modules/datetime_range/src/Plugin/Field/FieldType/DateRangeItem.php
   @@ -94,6 +110,7 @@ public static function generateSampleValue(FieldDefinitionInterface $field_defin
   +    // @todo Do we need to update this?
        $start = REQUEST_TIME - mt_rand(0, 86400 * 365) - 86400;
   

   Ditto, or w/ a followup (there may be an issue already to remove these.)

Also, there are item test and update path tests, but we also need formatters tests, and probably widget tests.

Also going to ping @WimLeers to see if there is REST impact here.

The impact on the rest and jsonapi modules is simple: they will automatically respect the optionality and validation constraint that this patch is introducing. It'd be nice to have explicit test coverage for them, but that's definitely not required.

Furthermore, this is a widening of what is permissible, not a narrowing, so no existing API clients could possibly be negatively impacted by this.

The only way they could be negatively impacted is if they were performing a request whose body did not contain an end date, which would have triggered a validation error, and the client is checking for that particular error string: this patch does not guarantee through test coverage that the error message is exactly the same before and after. But that's such a remote edge case that I would not expect this patch to explicitly test that.

Thanks @WimLeers. I added a followup to enhance the REST test coverage to account for this new feature.

This still needs to be set back to NW because some tests are missing, but let's trigger the testbot first to see if the changes break anything.

From #75:
1. Moved.
2. Updated.
3. Removed, but please post another patch if anyone knows the reason. I couldn't find a reason for this anywhere here.
4. Comment removed, and I replaced REQUEST_TIME with \Drupal::time()->getRequestTime() as per Time Service Added and REQUEST_TIME deprecated. We're in a static method here so I couldn't use dependency injection. I left mt_rand() as-is because we're not running in a security context.

This should fix the test failures.

Looks like we're running into Added configurable match limit to the entity autocomplete matcher. Anyone know where that stuff gets set?

I added the above things to the list of remaining tasks in the IS.

Some things I discovered while playing around with this on the front-end, which I also added to the list:

3.

If the start date is missing, stop deleting the end date on saving the form.

This is a bit wacky as we're losing entered data. If we don't want to accept a missing start date, don't allow form submission. Instead, show an error. However, we could also allow this and actually save the data. I recommend doing this instead. This was mentioned earlier. Just as we may not know the end date, we may also not yet know the start date.

4.

If missing, indicate that the end date is unknown/unspecified on display.

This can best be illustrated with an example. If we have a "Drupal Week" field, but don't know the end date yet, the start date should be displayed with some indication that no end date was set. Displaying the start date on its own doesn't make much sense. Basically, I'm disagreeing with #27:

Default behavior for the formatters, when the end date is not filled, is to act as if start and end is equal (only show the start date).

This is too unclear. If that's what we want to do, we should show start date - start date, but it doesn't work in all contexts. There needs to be a way to specify that data is missing, and should be added at some point. We could show the separator after the start date and making this hyphen display configurable: "Display separator with missing end date?". Or we could have the opposite question: "Assume a missing end date is the same as the start date?"

Probably the simplest thing would be to forget about config, and just add the separator before or after (depending on which subfield is missing, start or end date). This will work in both cases. If users want a missing date to really be the same date as the other subfield, they should specify this explicitly, and fill this in.

Blocks Recurring Dates Field: #2915298: [PP-1] Allow end date to be optional (date_recur)

Retested #60 to see if we have a patch passing for 8.8.

#87: Why #60? Why not the latest one?

@colan I am using #60 internally for our project. I was not sure about #87 since it is displayed in red.

@colan I think the best way for point 4 at #84 is to make an unknown end date indicator optional. (see #36)

We use the Date range field to display the dates for trade shows. Mostly they are longer than one day, but sometimes they are only one day. Our content authors don't want to add an end date in this case (all independent of each other). So I think the #27 hint is a common use case which is done with the current patch from #81.

Re-rolling #81 with the date field's timezone support from:
https://www.drupal.org/project/drupal/issues/2632040 #273

This is probably specific to my use case but thought I'd share if others are looking to combine the two.

Regarding some of the thoughts from comment #84:
3. I agree, missing start dates are a very valid use case and since we're already touching all the relevant code places, we should add that.

For field configuration, we could add a second checkbox ("Start date is optional").
The field formatters' viewElements could then simply check for if ($item->isEmpty()) {continue;}, with DateRangeItem::isEmpty returning TRUE based on the "optional parts" settings.

4. I also agree that there is a major difference between "missing data" and "implicitly the same".

If missing, indicate that the end date is unknown/unspecified on display.

-- @colan, #84

when the end date is not filled, is to act as if start and end is equal (only show the start date)

-- @birk, #27

Core date widgets currently have the assumption that "empty value" means "now". If we made this "Empty value behavior" configurable, we could include the options "Current date/time" (default), "Same as other value" (as suggested in #27) or "Leave empty" (as suggested in #84). That would allow for appropriate data processing (i.e. filling the start/end date or leaving it as NULL).
I'd suggest this configuration either on the field storage settings, or on the widget settings. In my experience, editors always have some kind of training for how to use their site - different site owners expect different logics that also affect other parts of the sitebuilding process (e.g. Views). Thus, I think this setting does not need to be handed to the editor (suggested in #36) but to the sitebuilder, which hopefully helps reduce the complexity.

I think the best way for point 4 at #84 is to make an unknown end date indicator optional. (see #36)

For the indicator when outputting the date on the front end, we could leave this up to the theme, by outputting only the appropriate element on missing date parts: Only fill the start_date key if the end date is missing, only the end_date if the start date is missing, or start_date, end_date and separator if both are present. That way, a template can easily check which data is present, and do things like add prefixes (e.g. "until {{ end_date }}", "since {{ start_date }}") in certain cases.
This also keeps the meaning (and thus description) of the separator formatter setting intact: "The string to separate the start and end dates".

Additional thoughts:

5. How do Views filters deal with NULL values in start/end date field values?

Updating the patch from #81 to fix the test failure.
The trigger_error() there is called from system_entity_form_display_presave() when the database updates are executed from DateRangeUpdateTest::testEndValueNull

I believe the only way to fix this is simply to run db updates on the dump.
According to the schema that dump was produced before 09/02/2018.
So I ran all database updates from the current 8.8.x HEAD (commit 154038f1401583a30e0ea7d9c19db02f37b10943)

Is this dump truly necessary? Any other way we can test this without such dump?
I'm attaching the updated dump as well.

Here I have just fixed the failed test of patch #97 for Drupal 9.1.x.

Why not allow start date and end date optional ?

It's a new patch for this.

I have successfully applied the patch but in my view, the end date is not shown

#Updated: I have removed the date field from the view and added another with another id and it works now

I have checked the above issue and applied the latest patch for the same but still issue is not resolved. Please look into the issue .

Patch in #98 tested and working although if no end date is set then the date is not rendered in views.

Need to be able to render the each part of the date in views so that the following is possible.

01/01/2001 - current

It looks like if either date is null then views treats the whole field as null.

Re-rolling #99 for whitespace fixes and wondering if that composer related test fail will repeat.

The patch from #99 is suggesting that both the start date and the end date could be optional (although this is only partially implemented in the patch).

So to be clear on the logic:

• if the WHOLE field is optional, then no data need be entered (regardless of other options)
• if the start date is optional, then either end date OR both.
• if the end date is optional, then either start date OR both.

My opinion is this is the most flexible and generic (80%) option that covers the most cases, and makes the validation rules more consistent.

The patch at #98 reflects the dominant discussion on the issue: that the end date can be marked optional. It appears to work OK in my manual testing so far.

Re #99, I thought that being able to make end OR start optional could have cleaner logic (and generally make the field more flexible) but as I tried coding this I found the logic (and UX) was still awkward to cover the combinations of field/start/end dates being required. I'm going to review #98 next.

Re-rolled #98 to fix whitespace changes.

Everything seems to work as designed. Having an optional end date highlights that there is no easy way to clear an existing date value in the editing interface.

I think a lot of limitations of the field display (not being able to do custom things like - current when end dates are missing, for example) could be solved by adding a special theme twig so that start/separator/end can be controlled. I think that would be really practical here.

(Note that you can control things in views by using conditional twig to check if the end value exists and conditionally printing, so the limitation is mostly in the node display, in my opinion.)

I think a lot of limitations of the field display (not being able to do custom things like - current when end dates are missing, for example) could be solved by adding a special theme twig so that start/separator/end can be controlled.

Does this potentially address this:

I thought that being able to make end OR start optional could have cleaner logic ... as I tried coding this I found the logic (and UX) was still awkward to cover the combinations of field/start/end dates being required.

Adding a theme twig would give a lot more flexibility in the output, but no it doesn't solve the limitation whereby some people might want an "optional start date" in the editing UI. I agree with @webchick at #39 that there's a happy medium.

The patch in #109 contains a db dump of test fixtures so I don't recommend using it. The patch in #99 doesn't do what it says it is supposed to do (add optional start date, I didn't see a checkbox for it on the field settings). I am testing #98 and so far things are working as expected after selecting the "optional end date" checkbox on the field settings.

As a general note, for anyone migrating a datetime range field from D7 to D8/9 you may think they need this patch but it after configuring the end date to import values properly you may find that you don't need it. The key for me was that the value2 in D7 maps to end_value in D8/9. Here is my config:

  field_event_datetime:
    -
      plugin: sub_process
      source: field_event_datetime
      process:
        value:
          plugin: format_date
          from_format: 'Y-m-d H:i:s'
          to_format: 'Y-m-d\TH:i:s'
          source: value
        end_value:
          plugin: format_date
          from_format: 'Y-m-d H:i:s'
          to_format: 'Y-m-d\TH:i:s'
          source: value2

I can say though that when I didn't have end_value set properly the patch in #98 worked just fine.

In the formatter, when checking if we should display the end date, I would like check the formatted value, and not just the time. For example, if the formatter just has the date and hour, and the start and end date are in the same hour but have a different minutes/second, IMO we shouldn't show the end date. ... but maybe this change should be another issue (or just a contrib module formatter), as this is how core currently does it.

Edit: It would be nice to work a similar change from #2823847: DateRange formatters should compare rendered dates instead of raw timestamps into this patch. The two patches conflicts.

Attached is a rerolled version of the patch from #109 without the db dump file and the test (src/Functional/Update/DateRangeUpdateTest.php) which relies on it. I'm not sure of the best way to approach amending/replacing that test however.

#118 tested working for Drupal 9.1.6.

In the method: "getConstraints" we can use early return approach with

if (!empty($this->getSetting('optional_end_date'))) {
  return parent::getConstraints();
}

The function will be:

  /**
   * {@inheritdoc}
   */
  public function getConstraints() {
    $constraint_manager = \Drupal::typedDataManager()
      ->getValidationConstraintManager();

    if (!empty($this->getSetting('optional_end_date'))) {
      return parent::getConstraints();
    }

    $label = $this->getFieldDefinition()->getLabel();
    $constraints[] = $constraint_manager
      ->create('ComplexData', [
        'end_value' => [
          'NotNull' => [
            'message' => t('The @title end date is required', ['@title' => $label]),
          ],
        ],
      ]);

    return $constraints;
  }

It's better because reduce one huge "if" conditional, and reduces the Hadouken effect in the code

What is a “Hadouken effect”? 🙂

I'm guessing it refers to when the indentation level gets really far over. It's better to return early, rather than have a bunch of nested conditionals.

For the curious, it's called a guard (which "remov[es] one level of nesting and resulting in flatter code"), and Uncle Bob is a fan as per Clean Code. (Everybody here has read that book, right? ;)

It makes the code much "cleaner" as there's less nesting, which should be avoided. So 👍 from me.

@xmacinfo #122 and #123 are right about that.

Hadouken effect is only an informal/funny way to say it

Please, disconsider it

Following the new patch with this improvement

The patch works for me!

The patch in #126 is working fine for me so far. I had to remove the field and add it again in order to make it work.

so according to #127 and #128 let's update the status

Per #75 we need formatter and update tests, and probably widget tests. There was an update test, but it was removed in #116.

I'm not seeing any of the other remaining tasks crossed out either.

@Petr Illek - Did you run run "drush updatedb" after applying the patch? There is a update task that should make sure you do not have to nuke the field to make core date range work.

Just a small remark: in Drupal 7, when the End Date was optional and tried to fill the End Date field, but left the Start Date empty, it was showing a validation error: "A 'Start date' date is required if an 'end date' is supplied for field %field #%delta.".

Now the behavior implemented in the patch is to simply ignore the End Date value if the Start Date was not supplied. I guess it would be nice to have an error message for the new implementation as well.

Just testing the latest patch and when rendering a date field with no end date in views it doesn't render at all because it thinks it's empty.

Just tried the patch together with commerce and got the following error message when tried to add something to cart with an optional end date.
Not sure if this is the right place, but thought it's maybe helpful.

Auf der Website ist ein unvorhergesehener Fehler aufgetreten. Bitte versuchen Sie es später nochmal.

Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'field_date_range_end_value' cannot be null: INSERT INTO {commerce_order_item__field_date_range} ("entity_id", "revision_id", "bundle", "delta", "langcode", "field_date_range_value", "field_date_range_end_value") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6); Array ( [:db_insert_placeholder_0] => 58 [:db_insert_placeholder_1] => 58 [:db_insert_placeholder_2] => default [:db_insert_placeholder_3] => 0 [:db_insert_placeholder_4] => und [:db_insert_placeholder_5] => 2021-11-25 [:db_insert_placeholder_6] => ) in Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (line 810 of core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php).
Drupal\Core\Database\StatementWrapper->execute() (Line: 823)
Drupal\Core\Database\Connection->query() (Line: 98)
Drupal\Core\Database\Driver\mysql\Connection->query() (Line: 32)
Drupal\Core\Database\Driver\mysql\Insert->execute() (Line: 1375)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->saveToDedicatedTables() (Line: 960)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->doSaveFieldItems() (Line: 622)
Drupal\Core\Entity\ContentEntityStorageBase->doSave() (Line: 452)
Drupal\Core\Entity\EntityStorageBase->save() (Line: 801)
Drupal\Core\Entity\Sql\SqlContentEntityStorage->save() (Line: 339)
Drupal\Core\Entity\EntityBase->save() (Line: 118)
Drupal\commerce_cart\CartManager->addOrderItem() (Line: 213)
Drupal\commerce_cart\Form\AddToCartForm->submitForm()
call_user_func_array() (Line: 113)
Drupal\Core\Form\FormSubmitter->executeSubmitHandlers() (Line: 51)
Drupal\Core\Form\FormSubmitter->doSubmitForm() (Line: 593)
Drupal\Core\Form\FormBuilder->processForm() (Line: 321)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 97)
Drupal\commerce_product\ProductLazyBuilders->addToCartForm()
call_user_func_array() (Line: 101)
...

Minor grammar fix.

Hi everyone !

I updated the patch to allow having an end_date without a start_date ("value" is now nullable).

I also added a test to test that the case I'm introducing (having start_date=NULL and end_date=XXXXX) passes the tests.

The patch has been rolled from 9.3.x.

Best regards,
Barig

Hi again,

I rerolled the patch to squash the commits.

Thanks !

All new feature development should be targeted at the latest core branch, that being 10.0.x.

Addressed Drupal CS issues of patch #140.

#126 worked for a site running Drupal 9.4.8

Current implementation generates:

So, it provides "(optional)" suffix to end date label. But... we have asterix (*) as a way to point if something is optional or not. I'd like to propose update for #142 to generate this UI:

If end date is optional, no asterix on it and wrapper (as not all fields inside wrapper are required).

If end date is required as well, then this UI:

and if all is optional:

I like the usage of asterisks (*) in #144.

+1 for proposal in #144

The current implementation is confusing.
The only visual difference between the required date and the optional date is the red asterisk.
This asterisk is just missing if the date is optional and a normal user has no visual indication that the date is optional.
As I said in #26 and #36 a better solution is to also put a checkbox after the optional end date to visually hide it and make it clear to everyone that the end date is optional.
This is how they did it on other websites and it makes sense.

At first sight: Why would I want the extra effort of a checking box?!
The red asterisk is perfectly sufficient.
I like #144 because of the simplicity. It should be the default.

I can understand that there are use cases for #36. They are just too complex for the default.

"(optional)" is not a pattern we use elsewhere, we use the asterisk to denote when a field is required, and that is a pattern which is consistent across applications.

We should not attempt to introduce a new pattern for that in this issue.

+1 for #144 also from me. Let's keep this simple. A checkbox needs to be clicked each and every time when someone is filling out the form - quite annoying. It doesn't remove clutter though as the input field is hidden but there would be a checkbox instead. We also don't have such a checkbox for other optional fields (e.g. Drupal commerce's "address line 2" on billing addresses etc.) which would be inconsistent.

If people are anxious about users not noticing that the second field is optional, they can always customize and add a description, specific placeholder, colors etc. to make it clearer.

The checkbox is useful in case you want your site's average users to correctly enter a date range for an event that is still happening and will continue to happen after today.

In the back-end there can be an option which shows or hides the front-end checkbox.

Example:
- With the checkbox the uses select the correct end date (none):
I am a student from [2021/09/15] - [x] Present
When the front-end user clicks the [x] Present checkbox, the the end date [Y/m/d] is hidden

- Without the checkbox most people will select the current date as the end date, which is wrong.
I am a student from [2021/09/15] - [2022/12/21]

They will be confused:
"What end date should I select here? I am still a student.
Do I select TODAY as the current end? That doesn't seem right... I will be a student TOMORROW as well.
Do I leave it empty? Maybe... BUT what if it shows an error?"

Every other website solves this confusion by using a checkbox for No end date (ex: the Present).
Again, this front-end checkbox can be made OPTIONAL (hidden) from the back-end.

Let's keep this simple. When the end date is made optional the scope of this issue is done.
And it is according to all Drupal UI standards.
And we can finally finalize this important functionality.

#151 etc could be moved to a separate issue.

Fixed CCF for #144.
Please review.

Let's keep this simple. When the end date is made optional the scope of this issue is done.
And it is according to all Drupal UI standards.
And we can finally finalize this important functionality

This is right. Getting a complex issue like this fixed requires a tight scope. #151 is a useful consideration, but better to address in a follow-up issue I suggest.

The above patch #153 replaces the instance of REQUEST_TIME but strangely the CCF is still failing.

-    $start = REQUEST_TIME - mt_rand(0, 86400 * 365) - 86400;
+    $start = \Drupal::time()->getRequestTime() - mt_rand(0, 86400 * 365) - 86400;

Updating to target the 10.1.x branch.

Looks good! Left a minor feedback. Also, this most probably requires a change record.

Addressed review and created CR.

Hiding files as fix is in MR.

Left some small comments.

Posted a review in the MR

Reviewing the remaining tasks and was only able to say that 1 is done.

Since this isn't using a formatter anymore first bullet may not be needed but could someone familar with the issue confirm?
Same for 2nd bullet

If not needed "Needs test" tag probably can be removed.

Yes, I think the 1st and 2nd bullets in remaining tasks are not needed and about the last bullet in remaining tasks I don't think we have to do it in this issue because the current behaviour in the MR is to show only start date when end date is missing which I think is fine for this issue.

Applied the MR on a Standard profile install of 11.x
Added a range field to the Basic page content type.
Checking "Make End date optional"

Created a Basic page and didn't set an end date. Node saved without issue
Went back to the settings unchecked "Make End date optional"
Went back to my Basic page and now I'm required.

Ran drush updb and post_update and install hook ran without issue.

How come the setting is under the field storage though and not a form formatter setting though? Won't this mean editors can't reuse this field in 2 places where 1 should have optional end date and the other not?

Also think issue summary could use some work.

add validation constraint

may be having a case of the Mondays but was a validation added?

Also there are some UI changes that are missing from the issue summary.

I am going to work on shifting the setting to field settings so that this field can be reused in 2 places where 1 should have optional end date and the other not. I am also going to work on adding the validation constraint.

Only if you think that is best approach. Wasn’t saying that it was needed more general observation/discussion

Added history of discussion to IS to enable easier reviewing of the discussion so far.

The key questions

5 key questions have recurred again and again over the life of this issue:

1. Is the start date optional too?
2. What value is stored in the end date property if it is left empty in the widget?
3. Does the optional_end_date setting live in the field config or field storage config?
4. How are optional and non-optional properties distinguished in the widget?
5. How are missing end dates displayed in the formatter?

I have added these to the IS. The answer in the current MR is:

1. Is the start date optional too? NO
2. What value is stored in the end date property if it is left empty in the widget? NOT SURE
3. Does the optional_end_date setting live in the field config or field storage config? STORAGE
4. How are optional and non-optional properties distinguished in the widget? NOT SURE
5. How are missing end dates displayed in the formatter? ONLY START DATE IS SHOWN

I think we need to be careful to keep track of our answers to these important questions in the IS, even as we work on the details of patches.

Kunal

@kunal.sachdev some feedback/requests for you:
A. Please could you tell us the answer for the questions I've answered "NOT SURE" above
B. In #48 a subsystem maintainer suggested to keep the setting in the field storage config
C. Please post a screenshot of the widget so we can see that the "required" indicators follow th suggestion made in #144

Tests

Lastly, I believe that even though we are not making large changes to the formatter and widget, we still need to explicitly test that they work with missing end dates. Therefore we need both formatter and widget tests.

Scope

If this issue is to ever get committed, realistically we need to keep the scope tight and defer to follow-ups anything we can. Therefore I suggest:
- Optional start date. This is a valid use case, but let's open a follow-up issue for it.
- Customising display of missing end date. This is a valid use case, but let's open a follow-up issue for it. Per product manager's usability review in #39, core should not offer complex options here and it is fine to just display start date if the end date is missing.

Therefore I think the current MR is correctly scoped.

Answering some of the questions asked in #180 :

1. What value is stored in the end date property if it is left empty in the widget? NULL
2. How are optional and non-optional properties distinguished in the widget? I think asterix (*) is used to distinguish between optional and non-optional properties.

In #48 we have suggestion to keep the setting in the field storage config but I do agree with #174 that if this setting is kept in the field storage config editors can't reuse this field in 2 places where 1 should have optional end date and the other not that's why I am going for shifting the setting to field settings.

Screenshot of the widget if the end date is optional:

Screenshot of the widget if the end date is required:

2. What value is stored in the end date property if it is left empty in the widget?

The answer can only be NULL, or to use a separate Boolean like end_date_is_all_day.

This issue happened before, in the Date module for 7.x. See #3202962: Expand field specification with new element for "all day". Using a "magic value", such as storing the date with a certain time to indicate that it's an all-day value, was a very bad approach that led to multiple issues. It eventually was rewritten in 7.x-3.x, but bugs linger in the 7.x-2.x branch and it's hard for sites to update or clean their data that was messed up.

#35 discusses some specific problems with putting the setting on the field config not the field storage config.

The screenshots in #181 match the consensus suggestion in #144, so that's good.

For now I am keeping the setting in field storage config and I think we can decide on shifting the setting to field config in another follow-up.

Created follow-ups:

1. #3395096: Allow start date to be optional
2. #3395100: Customise display of missing end date.
3. #3395103: Shift the optional_end_date setting from field storage config

@kunal.sachdev Thanks for creating the follow-ups. Can you please set the status of the follow-ups to Postponed and post a comment that they are postponed on this issue when you change the status? You can also add this issue as a parent issue or as a related issue. I don't think it makes sense for someone to work on these issues until this issue is fixed.

Also, I think it might be a good idea to list the follow-up issues somewhere in the issue summary, because this issue is pretty long and takes some time to read through, so a good issue summary that has all the relevant information is even more important for this issue to make it easy for core committers and other contributors new to the issue.

The issue summary also lists a remaining task (2.) that is not marked done yet. Can you please update the IS with the current approach in your implementation and/or with a reference to the follow-up you created, so that it is clear that the task is either done or out of scope?

W/r/t the last follow-up, a subsystem maintainer mentioned in #48 that the setting should be kept in field storage. Unless there is compelling evidence that the reasons given by them are maybe outdated in the meantime (they commented 5 years ago, so it might no longer be true due to some other changes in core), I think we should follow their advice and keep the setting in storage. If we think the setting should be in the field settings, I don't think we should have a follow-up, but I think we should do it in this issue. It doesn't make much sense to introduce a new setting now and then shift that from storage to field settings later. We could then use a field setting right from the start.

Closed #3395103: Shift the optional_end_date setting from field storage config and added new remaining task to decide if we want to shift the optional_end_date from field storage config or not.

Thanks kunal. Added remaining tests needed to IS

may be having a case of the Mondays but was a validation added?

Yes the validation constraint is added in \Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem::getConstraints and test is also added for that \Drupal\Tests\datetime_range\Functional\EntityResource\EntityTest\EntityTestDateRangeTest. I think I am going to remove that constraint and add a separate constraint class and its validator because I think it's the more correct way to do it.

Discussed about some points with @lauriii and @tim.plunkett in scrum :

• About the validation constraint which is currently added in \Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem::getConstraints and decided to keep it that way.
• About the optional_end_date setting which currently lives in the field storage config and decided to keep it as it is for this issue and if we want to shift it to some other settings then we can do it in the follow up I opened #3395103: Shift the optional_end_date setting from field storage config.

Just reading through the MR now in detail; it looks like the MR is not actually configuring whether the property is required or not in the schema level. It looks like there were some challenges with this approach in #33. I did find \Drupal\Core\Field\Plugin\Field\FieldType\StringItemBase::propertyDefinitions which configures the property based on the field definition, so I wonder if that's worth trying.

However, if we are not configuring this on the property level, but instead in the validation constraints, then this could be a field config setting, because it could be applied according to the field config. We have pre-existing examples of this like \Drupal\Core\Field\Plugin\Field\FieldType\NumericItemBase::getConstraints.

I do think @tim.plunkett raised a compelling argument that in some cases you may want this field to have different behavior in different bundles, which would support the idea of configuring this in the field config. So the question then becomes, is it acceptable for the end_value property to be always optional, for the requirement to be enforced in validation constraints depending on the field config setting?

Yes - I agree with @lauriii and @tim.plunkett, making this vary per field config rather than storage if there's no DB schema change feels like a good approach.

optional_end_date setting is moved to the field config and hence closed #3395103: Shift the optional_end_date setting from field storage config.

So we have consensus on the solution, and the MR is complete.

I am not convinced a further usability review is needed, so I think this can be RTBC after code review of recent changes.

Looks good to me except some suggestions and one question.

@kunal.sachdev believe as the starter of the MR you can close the threads just FYI.

Nice to see it moved out of the storage setting.

Tested as before
Created a range field with "Optional" setting unchecked
Created a node and got an error about there being no end date.
Checked the "Optional" setting
Created a node without an end date and could save just fine.

Using a 2nd content type
Reused the field and set the setting to the opposite of the first content type
Settings were honored on both content types.

LGTM!

Nice work on this issue and moving out of storage settings.

1. This sentence is really awkward and fails readability IMO: "Allow end date to be optional opposed to the default behaviour where end date is required when "Required field" is checked."
2. I also think that field should be disabled unless the "Required" is checked. This setting makes no sense if the whole field is not required.

For the first issue, would this work better? Allow the end date to be optional, even if the date range is required.

For the second issue, that would be solved with #states api.

I haven't looked in the MR yet but the checkbox shouldn't have anything to do with the "Required" checkbox. The "Required" checkbox controls if the datetime range field must contain a value. The new checkbox added here, defines what is a valid value for the datetime range field; whether we expect to always have a pair of start and end, or can we accept just the start date. The new setting should impact the validation even when the field is not required. If the field is not marked as required, the end date should be still required when the user enters a start date.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Usability review

We discussed this issue at #3398823: Drupal Usability Meeting 2023-11-10. That issue will have a link to a recording of the meeting. For the record, the attendees at the usability meeting were @AaronMcHale, @Emma Horrell, @benjifisher, @rkoller, @sime, and @simohell. If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

In the review we compared the setup of this option to the LinkItem options. The recommendation was that the checkbox "Optional end date" be converted to radios. This has two benefits

1. Radio options are easier to describe (breaks up the logic)
2. Makes a future change to add "optional start date" easier, as simply a third radio (see screenshot).

I have coded this change and created a MR against the existing MR, rather than just adding to the current MR. The functionality currently works as expected, so aside from changes to interface language, this is a positive review of the existing MR.
https://git.drupalcode.org/issue/drupal-2794481/-/merge_requests/2

I'm not confident that I have the language right, but I opted to be a little verbose. This isn't easy text due to the confusion that is caused in combination with the "required" option.