Problem/Motivation

The datetime field does not support end dates, which makes it impossible to create a date range. This is a very important feature for a calendar.

Proposed resolution

Add new field field type to represent a data range, which will allow collection of end date.

Remaining tasks

User interface changes

New field widgets.

API changes

New field type and associated goodness.

Data model changes

New schema for date ranges.

CommentFileSizeAuthor
#372 xss_escaped.png48.73 KBxjm
#367 interdiff-348-367.txt13.07 KBmpdonadio
#367 2161337-367.patch114.39 KBmpdonadio
#13 2161337-13.patch9.78 KBswentel
#13 interdiff.txt1.06 KBswentel
#9 2161337-7.patch9.71 KBswentel
#7 2161337-7.patch9.71 KBswentel
#16 2161337-15.patch2.16 KBSKAUGHT
#25 2161337-25.patch9.85 KBdarrick
#30 2161337-30.patch10.57 KBdarrick
#31 interdiff.txt894 bytesdarrick
#35 2161337-35.patch12.65 KBmpdonadio
#35 interdiff-30-35.txt5.25 KBmpdonadio
#41 2161337-41.patch17.89 KBmpdonadio
#41 interdiff-35-41.txt8.93 KBmpdonadio
#42 2161337-42.patch19.11 KBmpdonadio
#42 interdiff-41-42.txt1.04 KBmpdonadio
#50 2161337-50.patch25.63 KBdarrick
#50 interdiff-42-50.txt8.78 KBdarrick
#54 2161337-54.patch24.99 KBdarrick
#54 interdiff-50-54.txt2.81 KBdarrick
#64 interdiff-54-64.txt6.72 KBdarrick
#64 2161337-64.patch24.93 KBdarrick
#66 2161337-65.patch28.47 KBdarrick
#68 interdiff-65-68.txt1.86 KBdarrick
#68 2161337-68.patch28.72 KBdarrick
#77 2161337-77.patch29.48 KBBerdir
#77 2161337-77-interdiff.txt3.51 KBBerdir
#78 interdiff-77-78.txt3.86 KBdarrick
#85 2161337-85.patch29.51 KBmpdonadio
#90 2161337-90.patch26.6 KBdarrick
#91 interdiff-85-90.txt13.36 KBdarrick
#114 2161337-114.patch14.77 KBmpdonadio
#116 2161337-116.patch42.8 KBmpdonadio
#116 interdiff-114-116.txt28.61 KBmpdonadio
#120 2161337-120.patch45.34 KBmpdonadio
#120 interdiff-116-120.txt11.19 KBmpdonadio
#123 2161337-123.patch46.6 KBmpdonadio
#123 interdiff-120-123.txt20.37 KBmpdonadio
#125 2161337-125.patch47.44 KBmpdonadio
#125 interdiff-123-125.txt1.61 KBmpdonadio
#128 date_range_edit_form.png24.61 KBDuneBL
#132 2161337-131.patch51.1 KBmpdonadio
#132 interdiff-125-131.txt3.66 KBmpdonadio
#133 2161337-133.patch51.1 KBmpdonadio
#133 interdiff-131-133.txt1.18 KBmpdonadio
#140 Screen Shot 2016-06-02 at 8.51.01 PM.png18.89 KBFluxSauce
#152 2161337-151.patch50.92 KBmpdonadio
#152 interdiff-133-151.txt11.57 KBmpdonadio
#154 2161337-154.patch53.09 KBmpdonadio
#154 interdiff-151-154.txt9.3 KBmpdonadio
#158 2161337-158.patch74.17 KBmpdonadio
#158 interdiff-154-158.txt30.52 KBmpdonadio
#162 2161337-162.patch106.05 KBmpdonadio
#162 interdiff-158-162.txt43.78 KBmpdonadio
#166 2161337-166.patch107.43 KBmpdonadio
#166 interdiff-162-166.txt5.97 KBmpdonadio
#169 2161337-169.patch107.43 KBpguillard
#169 interdiff-2161337--166-169.txt2.29 KBpguillard
#181 2161337-181.patch110.25 KBmpdonadio
#181 interdiff-169-181.txt10.78 KBmpdonadio
#182 2161337-182.patch111.03 KBmpdonadio
#182 interdiff-181-182.txt6.33 KBmpdonadio
#184 2161337-184.patch112.19 KBmpdonadio
#184 interdiff-182-184.txt14.75 KBmpdonadio
#192 Date vs Date range.png113.43 KBjonathanshaw
#192 Date vs Date range 2.png64.9 KBjonathanshaw
#194 2161337-194.patch112.18 KBmpdonadio
#194 interdiff-184-194.txt1.76 KBmpdonadio
#200 2161337-200.patch112.2 KBmpdonadio
#200 interdiff-194-200.txt1.14 KBmpdonadio
#202 2161337-202.patch109.69 KBmpdonadio
#202 interdiff-200-202.txt7.23 KBmpdonadio
#206 2161337-206.patch109.56 KBmpdonadio
#207 interdiff-202-206.txt3.78 KBmpdonadio
#213 2161337-213.patch109.57 KBmpdonadio
#213 interdiff-206-213.txt829 bytesmpdonadio
#223 field_options.png63 bytesxjm
#223 default_value.png34.99 KBxjm
#223 defaults_in_action.png8.03 KBxjm
#223 start_date_too_early.png5.78 KBxjm
#223 default_formatter.png7.5 KBxjm
#223 formatter_options.png13.74 KBxjm
#224 field_options.png57.17 KBxjm
#230 2161337-230.patch112.12 KBmpdonadio
#230 interdiff-216-230.txt6.22 KBmpdonadio
#232 2161337-232.patch112.21 KBmpdonadio
#232 interdiff-230-232.txt1.56 KBmpdonadio
#236 2161337-date_range-235.patch110.19 KBtim.plunkett
#236 interdiff-2161337.txt37.94 KBtim.plunkett
#242 2161337-242.patch110.5 KBmpdonadio
#242 interdiff-236-242.txt37.55 KBmpdonadio
#243 2161337-243.patch109.26 KBmpdonadio
#243 interdiff-242-243.txt42.27 KBmpdonadio
#246 2161337-246.patch109.45 KBmpdonadio
#246 interdiff-243-246.txt9.06 KBmpdonadio
#247 2161337-247.patch109.98 KBmpdonadio
#247 interdiff-236-247.txt105 KBmpdonadio
#247 interdiff-246-247.txt31.03 KBmpdonadio
#253 2161337-253.patch109.51 KBmpdonadio
#253 interdiff-236-253.txt104.52 KBmpdonadio
#253 interdiff-247-253.txt7.43 KBmpdonadio
#267 add_a_date_range_field-2161337-267-8.x-1.x-do-not-test.patch111.96 KBsylus
#277 2161337-253.patch109.51 KBxjm
#281 2161337-281.patch108.82 KBeffulgentsia
#281 interdiff-277-281.txt4.3 KBeffulgentsia
#282 daterange_labels.png67.83 KBeffulgentsia
#283 2161337-283.patch108.95 KBeffulgentsia
#283 interdiff-281-283.txt2.07 KBeffulgentsia
#288 2161337-287.patch109.3 KBeffulgentsia
#288 interdiff-283-287.txt768 byteseffulgentsia
#289 interdiff-253-287.txt6.88 KBmpdonadio
#290 2161337-290.patch109.29 KBeffulgentsia
#290 interdiff-287-290.txt786 byteseffulgentsia
#291 2161337-291.patch109.12 KBeffulgentsia
#291 interdiff-290-291.txt2.8 KBeffulgentsia
#294 2161337-293.patch109.14 KBmpdonadio
#294 interdiff-253-293.txt10.77 KBmpdonadio
#294 interdiff-291-293.txt1.07 KBmpdonadio
#305 date-range.png38.45 KBYesCT
#319 2161337-316.patch109.29 KBmpdonadio
#319 interdiff-293-316.txt925 bytesmpdonadio
#319 Screen Shot 2016-08-19 at 9.58.48 PM.png44.19 KBmpdonadio
#332 2161337-332.patch103.59 KBswentel
#336 2161337-336.patch103.5 KBswentel
#338 2161337-338.patch103.5 KBswentel
#339 interdiff-2161337-339.txt15.66 KBswentel
#346 interdiff.2161337.338.344a.txt2.86 KBYesCT
#346 2161337-344a.patch103.53 KBYesCT
#346 interdiff.2161337.344a.344b.txt17.93 KBYesCT
#346 2161337-344b.patch103.83 KBYesCT
#348 interdiff.2161337.344b.348.txt2.74 KBYesCT
#348 2161337-348.patch104.01 KBYesCT
#356 2161337-missing-hunks.txt10.01 KBmpdonadio
#363 332-interdiff.txt11.17 KBswentel
#363 2161337-362.patch102.15 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

MarkusDBX’s picture

I'm also curious of this, any updates? Is end date functionality supposed to go into core in the first release of Drupal 8? Or are you supposed to create two fields in Drupal 8, for this kind of functionality?

kiwad’s picture

Was looking for more info about this and found a comment by @KarenS in a D8 core issue

https://www.drupal.org/node/501428#comment-6699586

The D8 version of the contrib Date module will provide a more complex field that is comparable to what is in Date now. My plan is to call the new contrib Date field the 'Date Range' field, to indicate it supports a start and end date and the other complexity.

Although, i'm not sure if this means that we need to look for a DateRange project or the current contrib...

Is it better to start with 2 core date fields to manage Start-End dates for now in D8 ? However that might be a pain to restructure data if a DateRange module eventually happens...

kclarkson’s picture

I am "very" surprised that this was not in the initial core discussions. I am assuming most people what an end date when using the date module.

A couple of questions:
-At this point are we forced to use two fields?
-What are the advantages to disadvantages to two separate fields?

jhedstrom’s picture

The core 'datetime' module also does not support the storage of timezones with the date, so that is another feature the contrib module will need to take on.

colan’s picture

Category: Support request » Feature request

Is there a core issue for getting an end date into core 8.1.x?

If not, perhaps we should open one? It would be better to work upstream if possible.

Anonymous’s picture

Re #3:
Kind of a late reply, but using only what exists at this point, two fields is the only way to go other than custom coding. However, it will be hard (and unsupported) to migrate that data once we get the functionality in.

Re #5:
Afaik there is no specific request, but the more general discussion can be found here: #2543958: [META] DateTime Module Improvements.

swentel’s picture

Status: Active » Needs review
FileSize
9.71 KB

First stab at this (currently not a different field type, see https://www.drupal.org/node/2632040#comment-10676660 for more background.

Tests should be green.
Widget also shows up - but it's not 'nice' with the inline floating labels :/

swentel’s picture

Project: Date » Drupal core
Version: 8.x-1.x-dev » 8.1.x-dev
Component: Date Field » datetime.module

holy crap - I just realized that this is still in the date queue, moving to core

swentel’s picture

reuploading to get a test run ....

swentel’s picture

Priority: Normal » Major
Issue tags: +Contributed project blocker

Also, this is major and a blocker for contribs - and even relatively simple sitebuilder tasks too ..

colan’s picture

swentel’s picture

Status: Needs work » Needs review
FileSize
9.78 KB
1.06 KB

Fix tests - interesting here - since that this /could/ potentially mean the upgrade path can be easy (or at least less annoying).

SKAUGHT’s picture

in terms of some UX.. just adding and end date makes the user fill out a second date popup as well. perhaps instead of using the 'collect an end date' checkbox we can use better options on the select list of the field settings:

'#options' => array(
static::DATETIME_TYPE_DATE => t('Date (all day)'),
static::DATETIME_TYPE_DATETIME => t('Date and time'),
static::DATETIME_TYPE_DATETIMERANGE => t('Date and time with range'),
),

this leaves the issue of 'spanning days' of course. then maybe use the checkbox for the option to add the date popup. Under the idea that most people will be collecting a time on the same day.

SKAUGHT’s picture

jhedstrom’s picture

  1. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -7,6 +7,9 @@ field.storage_settings.datetime:
    +    enddate_get:
    
    +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
    @@ -33,6 +33,7 @@ class DateTimeItem extends FieldItemBase {
    +      'enddate_get' => FALSE,
    

    Rather than enddate_get, can we go with something like collect_enddate? I didn't fully understand the meaning of enddate_get until I read further through the patch.

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
    @@ -107,15 +139,22 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
    +        $values['value2'] = gmdate(DATETIME_DATE_STORAGE_FORMAT, $timestamp);
    ...
    +        $values['value2'] = gmdate(DATETIME_DATETIME_STORAGE_FORMAT, $timestamp);
    

    We should add some small random time to the sample enddate so they aren't identical to the start date.

jhedstrom’s picture

Cross-post. The review in #18 is for the patch in #13. The patch in #16 seems to be an interdiff, rather than a full patch?

swentel’s picture

I based my patch on the functionality that exists in the date project, variables/keys are also coming from there. All up for better names indeed :)

mpdonadio’s picture

Do we want to postpone #2632040: [PP-1] Add ability to select a timezone for datetime field on this since they touch the exact same portions of code, and likely also want TZ support for both? Seems logical to do this first.

This is looking good.

swentel’s picture

I'm fine with postponing - interesting that you mention, do we want different timezone per date, or is the timezone for all dates that are stored per delta ? Will have to look how the date project currently does this.

SKAUGHT’s picture

Myself, I do think this functionality is more important than a timezone subfield. Given that right now i can't use drupal8 to make a basic event content type stating 10am-4:30pm..

i'ld say Timezone is more clear with 'a Location of event' Field to an end user.. in most situations.

Surely, coordination with #2632040 will need to occur regardless of which ever issue may make it into core first..

darrick’s picture

Hopefully this helps. Reran the patches from #13 and #16 to fix the testing errors. Noting that one missing piece is setting the default enddate.

colan’s picture

Status: Needs work » Needs review

Let's see if the errors go away before changing the state back to "Needs work".

mpdonadio’s picture

#25, thanks for the patch. Can we get an interdiff? They really help review progress on issues.

darrick’s picture

Status: Needs work » Needs review
FileSize
10.57 KB

@mpdonadio I'm not familiar with the interdiff process but will look that up while I'm waiting for this one to process. I added a datetime.install file to update the schema for the enddate_get field. Bit of learning curve on that so bear with me :)

darrick’s picture

FileSize
894 bytes

Here is the interdiff. Assumed 15 was an interdiff. So this is between 13+15 and 30.

mpdonadio’s picture

Status: Needs review » Needs work
Issue tags: +SprintWeekend2016, +SprintWeekendDCNJ, +Needs upgrade path tests

Initial thoughts. Looking at the fatal that the code mentions.

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
    @@ -47,20 +48,38 @@ public static function defaultStorageSettings() {
    -      ->setLabel(t('Date value'))
    +      ->setLabel(t('Date value (start)'))
    

    String change would be a BC break, so we need to either keep the old ones, or set them conditionally based on whether the end date is there.

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
    @@ -47,20 +48,38 @@ public static function defaultStorageSettings() {
    -      ->setLabel(t('Computed date'))
    -      ->setDescription(t('The computed DateTime object.'))
    +      ->setLabel(t('Computed date (start)'))
    +      ->setDescription(t('The computed start DateTime object.'))
    

    Ditto on string change.

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
    @@ -94,11 +126,19 @@ public function storageSettingsForm(array &$form, FormStateInterface $form_state
    +        static::DATETIME_TYPE_DATE => t('Date (all day)'),
             static::DATETIME_TYPE_DATETIME => t('Date and time'),
    -        static::DATETIME_TYPE_DATE => t('Date only'),
    +        static::DATETIME_TYPE_DATETIMERANGE => t('Date and time with range'),
    

    String changes.

swentel’s picture

Strings can be changed in minors if I read https://www.drupal.org/core/d8-allowed-changes#minor right.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Still working on this...

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Issue summary: View changes
FileSize
12.65 KB
5.25 KB

Punting this one back.

The main problem is DateTimeItem::propertyDefinitions(). Because of the way the Field API works, I don't see how we can use this logic here to add properties based on the enddate_get setting, since the schema has already been created, but not updated when the settings form gets saved, which is leading to an exception because of a null value.

mpdonadio’s picture

Status: Needs work » Needs review
DanieleN’s picture

Nice Patch, thanks!

Actually, i've a datefield only with a date (All day), also without a time field.
Seems the end_date field don't support this.

Actually i've disabled the time field in hook_form_FORM_ID_alter().
My code support the AddMore Buttons.
The $count_addmore = $form['field_offer_blocked']['widget']['#max_delta']; is the actually count of field-date.

So the user see only the date field without the time field.

Complete Code:

/**
 * Implements hook_form_FORM_ID_alter().
 * FORM_ID: "node_offer_edit_form"
 *
 */
function vhs_offer_form_node_offer_edit_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  // hide time field for field_offer_blocked
  _vhs_offer_hide_blocked_time($form, $form_state, $form_id);
}

/**
 * Implements hook_form_FORM_ID_alter().
 * FORM_ID: "node_offer_form"
 *
 */
function vhs_offer_form_node_offer_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  // hide time field for field_offer_blocked
  _vhs_offer_hide_blocked_time($form, $form_state, $form_id);
}

/**
 * Hide time field_offer_blocked and show only timerange
 * @param $form
 * @param \Drupal\Core\Form\FormStateInterface $form_state
 * @param $form_id
 */
function _vhs_offer_hide_blocked_time(&$form, FormStateInterface $form_state, $form_id) {
// end_date field patch "datetime_2161337-30.patch" 
//don't allow control field form
// Like the start_date we want only show 
//date field (all day) without time field
// start_date and end_date are multiple fields, 
//so we iterate through all addMore fields
// $count_addmore is the current count of this field item

  $count_addmore = $form['field_offer_blocked']['widget']['#max_delta'];
  for ($i = 0; $i <= $count_addmore; $i++) {
    $form['field_offer_blocked']['widget'][$i]['value2']['#date_time_element'] = 'none';
  }
}

Better way: This would be in the patch (..or in the core)

DanieleN’s picture

Version: 8.1.x-dev » 8.0.2
swentel’s picture

Version: 8.0.2 » 8.1.x-dev

Don't change the version number please.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Working on this again.

mpdonadio’s picture

First pass at formatter updates. Need to figure what to do with timeago.

mpdonadio’s picture

First pass at end date with timeago. Temperature check in sprint room was that if the end date is present, base the timeago on that. We really want to avoid a setting here, to not get into the situation of Date does everything again. It is easy enough to write a new formatter if you need different behavior.

Onto checking how the default values work.

Also think we want to get this patch in w/o Views support, and handle those plugins as followup after we get the TZ at entry support in.

tim.plunkett’s picture

+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
@@ -112,8 +112,18 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
       $date = $item->date;
+      $date2 = $item->date2;
...
+      if (!empty($item->date2)) {
...
+          datetime_date_default_time($date2);
...
+      elseif (!empty($item->date)) {

Not sure that local variables are needed here, but if you keep them, might as well use it consistently

mpdonadio’s picture

Issue summary: View changes

#44, that is consistent with how the rest of that method is written. IOW, sometimes the $item property is used, other places the local is used. The patch does it's best to try to keep the code style consistent (for better or worse)...

Default values seem to work OK w/ manual testing.

Manual review of the UI and general approach would be appreciated, so we can begin updating the webtests to cover these new cases.

Starting on upgrade path in the meantime.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Issue tags: +Needs upgrade path

I've been reading the issues about the schema upgrades, and I don't quite understand how to add the columns/indexes to all of the field instances in this case. Help here would be appreciated.

Going to start on some test coverage, but unassigning myself if anyone else wants to jump in.

darrick’s picture

Time ago in D7 is based on the start datetime. Which makes sense to me. I.e. if you have an event from 6-7pm I would say that event happened x time ago based on when it started not when it ended.

@mpdonadio what issues about schema upgrades are you referring to?

mpdonadio’s picture

#48, I'm not wed to using the start date. I am not sure if the D7 behavior is intentional, or it just works that way. A bunch of us thought that basing off of the end date made more sense. Let's see what others say. It's an easy change.

Schema change. In the patch, DateTimeItem::schema() defined a new column. Since https://www.drupal.org/node/2554097, schema changes that add columns require update hooks. The storage setting is added in datetime_update_8001() in the patch, but adding the new column to add datetime field instances isn't. One, I'm not sure the best way to get the list off all of the datetime field instances, and two, I'm not sure if we need to do a copy/update/copy update, or whether we can just add the column and the index to each table. If you try a manual update, you see the failure, and you can also see the problem in the last batch of test results. Hoping @swentel can chime in here; I'm not familiar with this aspect of the Field API :)

darrick’s picture

This patch updates the widget for setting the default value for the end date. Also modified the default formatter to only show the time if the start and end dates are the same. Also formatter no longer shows end datetime if equal to start datetime.

swentel’s picture

I can look at the upgrade path on thurday, unless someone beats me to it before it.

mpdonadio’s picture

Status: Needs work » Needs review

#50, thanks for the updates; they look like good changes. We should do another pass to think about other edge cases.

It looks like some formatting changes crept in. We need to try to avoid them, as they are considered out-of-scope changes for the patch (a committer will likely reject the patch with these).

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeFieldItemList.php
    @@ -56,11 +61,44 @@ public function defaultValuesForm(array &$form, FormStateInterface $form_state)
    -            )
    -          )
    -        )
    +            ),
    +          ),
    +        ),
           );
    
  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeFieldItemList.php
    @@ -95,12 +149,13 @@ public function defaultValuesFormSubmit(array $element, array &$form, FormStateI
    -      $storage_format = $definition->getSetting('datetime_type') == DateTimeItem::DATETIME_TYPE_DATE ? DATETIME_DATE_STORAGE_FORMAT: DATETIME_DATETIME_STORAGE_FORMAT;
    +      $storage_format = $definition->getSetting('datetime_type') == DateTimeItem::DATETIME_TYPE_DATE ? DATETIME_DATE_STORAGE_FORMAT : DATETIME_DATETIME_STORAGE_FORMAT;
    
  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeFieldItemList.php
    @@ -109,12 +164,24 @@ public static function processDefaultValue($default_value, FieldableEntityInterf
    -        )
    +        ),
    

#51, thanks, b/c the way storage works, I wasn't able to figure out a way to make the schema dynamic based on the options (the tables get created before the settings form is presented).

darrick’s picture

Status: Needs work » Needs review
FileSize
24.99 KB
2.81 KB

Okay. I removed the formatting changes. Also fixed some errors in how the default end date was being set.

nkraft’s picture

Watching the this. Was just about to set up my own Event content type and discovered the lack of a decent widget and the lack of the end date. Thanks for taking this on. Will this potentially be in the next core update, or is it destined for a contrib module?

mpdonadio’s picture

#56, we decided end dates and timezone-upon-entry are going to be in core, and we are doing our best to try to get them into 8.1 in April. Manual testing would be appreciated so we can start on the integration tests, and hopefully we can figure out this upgrade path (and tests).

colan’s picture

I think the feature freeze is February 24th so that gives us 2 weeks, correct? I hope to have some time on Friday to take a closer look at this (e.g. testing, etc.).

nkraft’s picture

That's fantastic @mpdonadio -- so I should hold off on creating end dates for now;

You mentioned manual testing. What does that entail, exactly? I'm wondering if its something I can help with, or if its beyond my backend skills...

mpdonadio’s picture

#59, we need people to try out the patch on systems and make sure the UX is decent and that we didn't miss anything from field creation through rendering. Then we can add the integration test coverage where needed.

ongdesign’s picture

My two cents, testing the patch on a clean install:

The current patch does not support null values for end dates if the start date is populated. However, the start date can be null if a valid end date is specified. I think I understand the rationale given in this thread, but it seems odd to me that the end date is the one presented as optional (an explicit user action is required when adding a Date field to "collect end date"), yet it's the required field. There's also no real documentation of the fact that the end date will become the required field, if it's added.

The interval formatting also seems a little wonky to me (e.g., "2/17/16 to 25/16") -- I've never seen intervals denoted like that.

Testing the patch on an existing install, I can't get drush updb to complete without deleting all existing instances of date fields first.

mpdonadio’s picture

#61, thanks for the manual testing; those all sound like bugs we need to iron out.

The last part about the `drush updb` has to do with the upgrade path that we still need to write.

darrick’s picture

I've been looking into fixing the bugs mentioned by #61. But also into the schema update. I'm repurposing some of the code from the EntityDefinitionUpdateManager::getChangeList function to dig up the fields which need updating.

Any tips on how I should go about updating the schema? The updateFieldStorageDefinition doesn't work. Currently my only option seems to be to do it directly:

 $schema = Database::getConnection()->schema();
 $schema->addField($entity_type_id . '__' . $field_name, 'newcol', $spec);

Also, is someone else already working on this. If not I could probably get a patch done later today.

darrick’s picture

Status: Needs work » Needs review
FileSize
6.72 KB
24.93 KB

Hopefully this will get us closer.

darrick’s picture

Status: Needs work » Needs review
FileSize
28.47 KB

Excuse the noise. Was working off the wrong branch.

darrick’s picture

Status: Needs work » Needs review
FileSize
1.86 KB
28.72 KB

Fixed the last testing error.

Also poking around the D7 date_api found the date_limit_format function which would help clean up the formatting when the end date is the same day as the start date. But not sure where to put that function. Possibly might be useful in the DateHelper class.

Referring to make this code in the DateTimeDefaultFormatter class more robust for handling different date formats:

        // Check if start day == end day.
        if ($this->getFieldSetting('datetime_type') == 'datetime') {
          // TODO better logic to split the date and time based on the format.
          $length = strspn($output2, $output);
          if ($length) {

            $output2 = substr($output2, $length - 1);
          }
        }
mpdonadio’s picture

Issue tags: +Needs tests

Don't have time to review the patch, but the I think end date logic you describe should be part of DateTimeFormatterBase so derived formatters have a chance to override/change the functionality.

I am also really wondering if special handling of the formatters for same date/month/year are really needed. I'm leaning towards core providing outputting both dates if they aren't equal, and leave it at that. Contrib can handle fancy handling, and so could a custom module for a site.

This issue is holding back a lot, and we don't want to get bogged down in minutiae. The upgrade path is key here, and also making sure we have good test coverage over everything we are adding.

darrick’s picture

#69 Okay. I don't want to hold anything up. Maybe once the dust settles we can revisit it or roll it into a separate issue.

As for the tests and upgrade paths. I'd be willing to help. But have no clue about what tests are needed and for the upgrade path I'm assuming you mean D6 to D8 and D7 to D8 as I believe I've already provided for a 8.0.x to 8.1.x database update.

Berdir’s picture

+++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateTimeItem.php
@@ -68,18 +83,26 @@ public static function propertyDefinitions(FieldStorageDefinitionInterface $fiel
+    $schema = array(
       'columns' => array(
         'value' => array(
-          'description' => 'The date value.',
+          'description' => 'The start date value.',
+          'type' => 'varchar',
+          'length' => 20,
+        ),
+        'value2' => array(
+          'description' => 'The end date value.',
           'type' => 'varchar',
           'length' => 20,
         ),
       ),
       'indexes' => array(
         'value' => array('value'),
+        'value2' => array('value2'),

That seems pretty complicated and risky for a minor update.

I'm not sure if it was discussed but wouldn't it be a lot less problematic to just have two different field types? There was some discussion above about a Date Range (although I'm not sure that name makes sense to me.. )

We did similar changes with text fields with/without a format.

With everything being classes, it's actually pretty easy to extend one field type from another and add additional properties/schema, without impacting anything that already exists.

mpdonadio’s picture

#71, I think the main concern I had was that what will we do when we need recurrences (needed for proper calendar support) and timezone support (see the relate issue, but we haven't ironed how how this would be implemented)? Splitting this out will add two more field types, and adding those would potentially add more field types:

- date only
- date only with end date
- date+time
- date+time with end date+time
- date only w/ custom timezone
- date only w/ recurrence
- ...

That could potentially end up being a lot of field types for someone to choose. And making the wrong choice would mean a site owner would have to figure out a way to get data from one field type into another.

Berdir’s picture

Date only and date+time is already a single type, so date with end date could easily support with and without time too.

But yes, I'm not sure where to draw the line with recurrence, timezone etc. But do you really want to maintain a single field type that has 6 different properties/columns and supports all kind of imaginable combinations of those?

That said, schema can also be flexible, although we only support changing it when it's empty. We already have cases like this with entity reference fields for example, that have different schema depending on whether they reference a config or content entity. So we could also make it so that value2 (which find a horrible name, fwiw) is only defined when the feature is enabled and not allow to change it for fields with data. Then at least we avoid the upgrade path complexity and unnecessary columns when they're not needed.

swentel’s picture

@Berdir did some reasoning about splitting here: https://www.drupal.org/node/2632040#comment-10716892
I suggested it too in that issue to split (a bit above) - I'm constantly shifting on the idea. One the concerns would be the explosion of formatters and widgets, potentially leading to a lot of duplicate code, but not entirely sure. Only a patch could prove if it's worth or not.

Berdir’s picture

I see. What about making the schema dynamic then? I'm worried about that upgrade path, I think it's missing quite a few things?

Specifically, it doesn't yet update the stored definitions and schemas so that the storage knows that the database was updated. That's quite complicated, see https://www.drupal.org/node/2641828#comment-10711058 where I changed a base field from one type to the other. It also only seems to update base field tables, doesn't yet update the data and data revision tables and then there's the complexity with non-sql backends, which would have to implement all this too? Oh, and test coverage :)

mpdonadio’s picture

The patches before #35 attempted to do the dynamic schema. See my comments there. We worked on this at DCNJ, but couldn't get around

Yes, this is complicated... I am not sure which is the least bad situation here.

Berdir’s picture

Ah, I think I see the problem that you mentioned. Not sure about a proper fix, but as a workaround, this patch seems to work. Wondering if the link field has similar issues with required not required, but possibly not.

I also had to remove a duplicated use in one test and Drupal\datetime\Tests\DateTimeFieldTest is currently failing for me, with and without my changes.

Either way, we definitely need UI tests here for creating a date field, so we test coverage for that.

darrick’s picture

FileSize
3.86 KB

Patch in #77 seems to works as advertised. I created both a single date field and a start and end date field on a content type. The table schema was correctly created for both. I then edited the fields to make the first single date include a end date and the other to only collect a start date. The schema was correctly updated (i.e. date_value2 was added to the first schema and removed from the second.)

I don't have a time to update the patch but created an interdiff to remove the schema update code in datetime.install and also remove the formatting code mentioned in #69

Berdir’s picture

Note that updating will only work as long as there is no data. We will need to add code to prevent that those settings are changed if there is data, similar to how \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::storageSettingsForm() does it, just disable those form elements.

darrick’s picture

#79 During my tests I wasn't able to change the storage settings if the field has data.

jhedstrom’s picture

+++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php
@@ -52,6 +52,32 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+      $element['value2'] = array(
...
+      if ($items[$delta]->date2) {

Can we normalize and either use value2 or date2 consistently throughout? The db column name is value2 but elsewhere in code we're using date2.

I'd also prefer better naming of enddate_get as mentioned in #18, but if that doesn't bug anybody else, I won't insist :)

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ryantollefson’s picture

Based on #82, does this mean we will be waiting until October to get this in core?

Is it possible to get this as a separate module as a stop-gap for the next 7 months?

mpdonadio’s picture

Sorry I have been a bit AWOL on this. Been dealing with something that has been occupying most of my free time.

Do we think we have this at a place with the widget and formatter where we can start on these tests and bring this to completion.

#83, we can see if we can get this into 8.1 as an exception to the policy.

mpdonadio’s picture

Reroll (think b/c #2684095: Convert field kernel tests to KernelTestBaseTNG).

$ git checkout 72d53b30d110cd2dd2bf3fd8b0f32835b7364339
$ git apply --index 2161337-77.patch
$ git rebase 8.2.x
First, rewinding head to replay your work on top of it...
Applying: 2161337-77.patch
Using index info to reconstruct a base tree...
A	core/modules/datetime/src/Tests/DateTimeItemTest.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/datetime/tests/src/Kernel/DateTimeItemTest.php
mpdonadio’s picture

If this comes back with fails in Migrate, we can ignore: #2685281: Spurious fails in 8.2.x in MigrateBlockTest. We can still work on this; you just need to go into the queued test and click the "run anyway" button.

darrick’s picture

I'd really like to see this in 8.1. Would it be helpful if I re-rolled a patch with my interdiff in #78, audit for the inconsistencies mentioned in #81 and better naming of the variables as mentioned in #18.

That would leave tests. And as for variable names I would change enddate_get to collect_enddate and enddate_require to require_enddate.

I can do this over the weekend.

mpdonadio’s picture

On mobile, so being brief. Need time to look at 78. Rest look ok to do.

darrick’s picture

New patch as mentioned in #88. Will wait for tests before uploading interdiff and summary.

darrick’s picture

Issue summary: View changes
FileSize
13.36 KB

#81 I believe date2 is used instead of value2 because it's a computed value in a format which the datetime element prefers. So, didn't make any changes.

#18 I renamed enddate_get to collect_enddate and enddate_require ot reguire_enddate throughtout.

#69 I removed the code for special formatting of start and end dates if the start and end date was the same. #61 seemed to have some trouble with this. So that should be fixed now.

#77 Seemed to negate the need to provide a update path for the field storage tables so I removed that code.

#37 Current patch supports start and end with date only.

I've been following this issue from the beginning and AFAICT we need tests, 6.x - 8.x, 7.x - 8.x upgrade path and also some testing and feedback from the community.

BTW: Found two minor bugs during testing:

#2686407: Datetime type should be disabled on storage setting form if field has data
#2686409: Time Ago summary does not render on Manage Display for Timestamp and Datetime fields

mpdonadio’s picture

Thanks darrick! This patch doesn't have to have upgrade paths from 6.x and 7.x; those would be part of Migrate (and probably contrib since the source is contrib). So, we need to make sure the UI for the changes is good, and then we can bang out that test coverage.

dawehner’s picture

I really like this feature!

  1. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -7,6 +7,12 @@ field.storage_settings.datetime:
    +    require_enddate:
    

    Yeah this is more like a decision between a description that follows the technical hierarchy vs. one which follows english sentences. At least I don't care that much.

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -53,6 +53,21 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
             $output = $this->formatDate($date);
    ...
    +        $output = $this->t('@date to @date2', [
    +          '@date' => $this->formatDate($date),
    +          '@date2' => $this->formatDate($date2),
    

    It is weird that we calculate this twice here.

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeDefaultFormatter.php
    @@ -68,12 +86,31 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +        $elements[$delta][] = [
    +          '#markup' => $this->t(' to '),
    +        ];
    

    I'm wondering whether it would be better to use #prefix on the date2 element instead.

  4. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    @@ -38,12 +39,25 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    -        $this->setTimeZone($date);
    

    What is the reason what we remove this here?

  5. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeTimeAgoFormatter.php
    @@ -113,8 +113,17 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    -      if (!empty($item->date)) {
    +      // Base the timeago calculation off of the end date, if it exists.
    +      if (!empty($item->date2)) {
    +        if ($this->getFieldSetting('datetime_type') == 'date') {
    +          // A date without time will pick up the current time, use the default.
    +          datetime_date_default_time($date2);
    +        }
    +        $output = $this->formatDate($date2);
    +      }
    +      elseif (!empty($item->date)) {
    

    I'm wondering whether this should rather calculate the middlevalue between these 2 dates.

  6. +++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php
    @@ -62,9 +88,22 @@ public function massageFormValues(array $values, array $form, FormStateInterface
    +  public function massageDateProperty(&$item, $property) {
    +    $date = $item[$property];
    

    Any reason why this method is public?

13jupiters’s picture

Thanks all for the hard work on this. My testing effort was unsuccessful but before I litter the thread with problems, can someone verify just what version of Core the patch applies to now? I applied patch in #90 to 8.1.0-beta1 - pre-install, post-install - and to 8.0.5 (post-install). When manually updating the core Date module and running updb, hook_install_8001 appeared to fire successfully.

When attempting to add new Date fields to a node type, the form had the proper/expected options ("collect end date?" "require end date?"), but failed to save - no node__field_mydatefield tables, complaints about missing "value2", objects not existing, etc.

So the question is - have others manually patched 8.0.5 or 8.1.0-beta with #90 successfully? Should I be trying against 8.2-dev? Did I miss another required patch?

Edit: a single "date(all day)" - with no end date - still functions properly.

By the way - is there any progress in getting an exception made that would allow end date into 8.1?

mpdonadio’s picture

The patch is against 8.2, but there is little difference between 8.1 and 8.2 now, so it should apply cleanly to both, and I suspect it will apply cleanly to 8.1-beta1.

Hopefully I will have time to move this forward again.

I haven't been able to catch a framework manager to ask about getting this into 8.1.

13jupiters’s picture

Patch in #90 applied cleanly to 8.1.0-beta2 and end dates are working quite well. Very encouraging, great work everyone! A fully functioning calendar now within sight - something a lot of regular users are going to assume D8 has...

jhedstrom’s picture

I just spent some time manually testing #90. It is looking really close. A few issues I noticed:

  • I am allowed to enter an end date that is earlier than the start date (I checked in 7.x and this was not allowed)
  • The default message for a required end date that is missing is just the validation constraint: This value should not be null.
  • If I do not require an end date, but if I make the date field required, then client side validation marks the end date as required. (This has been noted above, just noting it is still an issue)

These are pretty minor and I would be fine following those up in separate issues since this is already such a large change.

jhedstrom’s picture

Thinking about this a bit more, I'm now thinking the suggestion above in #71 to have a separate field type (Date range) will make the most sense. If we pursue an all-in-one field as the D7 version did, the code will be needlessly complex (especially once we add on timezone support). Further, anybody with an existing date field that contains data will not be able to use the end date anyway (w/o custom programming or some sort of migration), so a separate field would not have a negative impact on folks with existing data.

mpdonadio’s picture

#98, how would you address the concerns in #72 if we do the split? Or, are you thinking that we do date range as its own field, and then add timezone support to all of the field types?

jhedstrom’s picture

re #99 yeah, I was thinking timezone would be added for all dates, but the logic and formatters for a range could be isolated to its own plugin class, making things a bit simpler and more testable.

mpdonadio’s picture

Status: Needs review » Needs work
Issue tags: -Needs upgrade path tests, -Needs upgrade path

OK, talked with some people, mulled this over, and I think we need to retreat a bit to move forward.

Despite all of our efforts, the approach to add this to the existing fields has hit a brick wall. We need to redo this patch as two new fields, date+end and datetime+end. We can figure out better ranges, but we need clone the two existing fields and make verions w/ end date support.

One, this will be a lot easier. Two, it eliminates the upgrade path. C, we're stuck.

It will also allow us work on the timezone patch in parallel. If that lands first, it will be easy to add that into the new fields.

If anyone objects, please let us know.

heddn’s picture

I think having a separate field makesa ton of sense. +1 from me.

darrick’s picture

#101 Doesn't the current date field support both date and datetime? Are those also going to be split?

mpdonadio’s picture

#103, the existing types will not be split.

darrick’s picture

So six months ago I created a date_combo module (http://cgit.drupalcode.org/merci/tree/modules/date_combo?h=8.x-2.x). It's missing some widgets and formatters. But I could roll it into a patch.

Is date_combo a good name for the field?

And should it handle both date and date w/time, like the single date field?

ryantollefson’s picture

I'm certainly not a coder, so I admit that I don't understand all of the nuances behind these decisions; however, from the website designer/end-user perspective, fewer fields seems more intuitive.

Think about how date selection works in something like Google Calendar... everything has start+date+time and end+date+time. You check the box for all-day and the time fields disappear. Everything has a default end-date that is the same as the start-date, you change it only if needed. This type of setup seems pretty intuitive for the end-user.

heddn’s picture

The thing is we aren't just dealing with calendar based dates. We also have things like created date, date of birth, etc. Or maybe we have a re-occurring calendar invite. With no end, date. To support all of these, you need more flexibility than just a single date field that stores single dates as deltas.

So adding a date field that has a start/end on it makes sense, in some cases.

natedillon’s picture

Is there going to be a way to migrate from the existing date field to the new one? I'm currently building a Drupal 8 site that needs to use the date field for events, but I want to do this in a way in which I will be able to easily transition to the new one when it's available. I'm guessing the only thing I can do for now is set up fields for start date/time, end date/time, all day, etc., but is there anything I should be thinking about doing now that will make this an easier transition for me later?

heddn’s picture

Re 108, migrate is in core. Moving data from/to fields is very simple.

For me, I'm using Delta 0 as my start date and Delta 1 as end. With my custom admin theme that is based on Seven, I can even easily make the ui read start/end for data entry using some very basic twig magic.

teek’s picture

Hi All,

I just installed 8.1.0. Was a bit disappointed by the date/time entry field. The date field gives a nice popup where I can select a date (perfect!) but the time field next to it requires a very precisely formatted time string (like 18:20:00) and does not give a popup. There is also no way to add an end date.

I gather from the large amount of text above that there is "some" discussion regarding how to tackle this... I just would like to know, what are the time scales? When can I expect a nice time picker and a way to add an end-date/time? in 8.2.0?

Are there any modules you recommend that have this functionality right now?

Thanx in advance.

heddn’s picture

The timepicker is another issue. If one doesn't already exist for it, it should be created. The end date/time will be done when someone writes it up.

In the mean time, you can use two fields (for start/end) or use a single field with cardinality of 2 and use twig override in the admin theme to adjust the label text of delta 0 & delta 1.

mpdonadio’s picture

#110, if your browser supports HTML5 date elements, then the time will use an appropriate time entry. Chrome has this, Firefox doesn't. There is an issue to add a polyfill for this in #1838234: Add jQuery Timepicker for the Time element of the datetime field.

At this point, both end date and the polyfill are targeted for 8.2.x.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Ok, just reviewed the outstanding issues. I am going to tackle this, so @jhedstrom can focus on #2632040: [PP-1] Add ability to select a timezone for datetime field.

For the sake of sanity, this will focus on the fields and we will handle the views integration as a separate issue (esp since we have one or two views quirks remaining).

mpdonadio’s picture

Work in progress to start to show approach. Not worth testing yet.

kaizerking’s picture

worked for me

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs work » Needs review
FileSize
42.8 KB
28.61 KB

This almost works. The widgets need some TLC, and the date-only version doesn't work (likely a stupid copy/pasta error). Too tired to debug now.

And then tests...

Unassigning myself in case someone wants to work on this tomorrow. Will pick back up around 7:00pm EDT (2016-05-13T19:00:00-04:00).

mpdonadio’s picture

Oh, yeah. The schema additions, too. That would help.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Actively working on this tonight.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
45.34 KB
11.19 KB

Can't figure out what I botched with the schema. Field formatter settings work, but the field settings don't (datetime vs date).

Still needs polishing, better validation, and tests.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned

Tagging out for a bit.

mpdonadio’s picture

Issue summary: View changes
Status: Needs work » Needs review
Related issues: +#1918994: Improve Datetime and Daterange Widget accessibility
FileSize
46.6 KB
20.37 KB

This essentially works. Updated IS with todo list.

Would appreciate prelim feedback, help with the Constraints, and help with the theming.

Then more polishing and tests.

mpdonadio’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
47.44 KB
1.61 KB

Element validation. Not sure if we still want/need a Constraint on this, too?

jhedstrom’s picture

Not sure if we still want/need a Constraint on this, too?

I'd say we do, since those would fire from the API, not just from the UI.

DuneBL’s picture

FileSize
24.61 KB

#125 : Very nice patch; I would love to have it in core.

Here is a small feedback:

1-After saving a node containing several date_range fields, I got the following warnings/notices:

Warning: DateTime::diff() expects parameter 1 to be DateTimeInterface, object given in Drupal\Component\Datetime\DateTimePlus->__call() (line 294 of core/lib/Drupal/Component/Datetime/DateTimePlus.php).

Notice: Trying to get property of non-object in Drupal\datetime\Plugin\Field\FieldWidget\DateRangeWidgetBase->validateStartEnd() (line 144 of core/modules/datetime/src/Plugin/Field/FieldWidget/DateRangeWidgetBase.php).

Notice: Undefined index: group in datetime_field_views_data() (line 25 of core/modules/datetime/datetime.views.inc).

2-Edit Form:

In the Edit form, the "Start" label is inline with the field label, it should be on the next line. See the attached screenshot.

3-Feature request:

I really love the "Separator" setting because it allows us to output something like "StartDate to EndDate" if we choose " to " as a separator. What do you think to add another setting "Prefix" to get something like "From StartDate to EndDate" if we choose "From" as Prefix?

FYI, I am using 8.1...

edysmp’s picture

Applied #125 and works. Missing view filter for date(I believe that now is for text)... so useless for me :(

mpdonadio’s picture

#128, I am going to replicate that tonight and post a new patch before moving onto tests.

#129, views filters for this field will be a followup. It is too much to do in one patch, especially with the views related bugs we are currently trying to squash.

FluxSauce’s picture

Drupal 8.1.2 with #125 applied, added a date range field, then viewed the form and got the following Notice:

Notice: Use of undefined constant DATERANGE_DATETIME_STORAGE_FORMAT - assumed 'DATERANGE_DATETIME_STORAGE_FORMAT' in Drupal\datetime\Plugin\Field\FieldType\DateRangeFieldItemList::processDefaultValue() (line 98 of core/modules/datetime/src/Plugin/Field/FieldType/DateRangeFieldItemList.php).
mpdonadio’s picture

Status: Needs work » Needs review
FileSize
51.1 KB
3.66 KB

This should fix the warning.

Will handle theme issue later on.

Prefer not to add too many options for the formatters. 1, we will bikeshed them forever and 2, they are easy to extend yourself for customization.

Going to start on the tests. We can do the constraint as a followup (I also think #2716891: DateTimeIso8601::setDateTime() vs. DateTimeItem::schema(): (at least) one of them is broken complicates matters here).

mpdonadio’s picture

This should address the warning in #131, but note this patch is technically against 8.2.x, even if it cleanly applies to 8.1.x.

FluxSauce’s picture

Wow, that was a fast one! Unfortunately, the bug still exists, and actually manifests a second bug (pretty much the same) when editing the field.

Notice: Use of undefined constant DATERANGE_DATETIME_STORAGE_FORMAT - assumed 'DATERANGE_DATETIME_STORAGE_FORMAT' in Drupal\datetime\Plugin\Field\FieldType\DateRangeFieldItemList::processDefaultValue() (line 98 of core/modules/datetime/src/Plugin/Field/FieldType/DateRangeFieldItemList.php).

Something I neglected to mention and may be causing it - Default value: Default dates: Current date. Removing the default eliminates the error.

FluxSauce’s picture

mpdonadio’s picture

#136, I was literally working on this when you posted that. Did you try again with the patch in #133? The string in the error message doesn't appear anywhere in 8.2.x anymore with that applied, and I don't get a notice and default dates work fine for me. This is the hunk in the patch now:

+++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateRangeFieldItemList.php
@@ -0,0 +1,115 @@
+      $date = new DrupalDateTime($default_value[0]['default_date'], DATETIME_STORAGE_TIMEZONE);
+      $storage_format = $definition->getSetting('daterange_type') == DateRangeItem::DATERANGE_TYPE_DATE ? DATETIME_DATE_STORAGE_FORMAT : DATETIME_DATETIME_STORAGE_FORMAT;
+      $value = $date->format($storage_format);
+      // We only provide a default value for the first item, as do all fields.
darrick’s picture

#136: Just tested the patch from #133 and not able to reproduce your issue.

Testing Date Range with Date AND Time:

  • Set default to "Current Date"
  • Used single value.
  • Error messages set to "All messages"
  • Added a new node. No errors.
  • Editing the node. No errors.
  • Changed widget to Select. No errors.
  • Cycled through the formatters. No errors.

Testing Date Range with Date ONLY (same as above except with following issues):

  • Default formatter shows the date and time. Maybe should set the default formatter with a date only format.
  • Custom formatter shows the date and time. Default format should be date only.
  • Select List widget shows hour and minute select boxes. Bug. No way to get around it in the gui.

Edited to add regarding#128: The labels on the widgets could use a little work. I.e. When using multiple values you have a header with the field name ("Date Range") then below that inputs for the multiple values. With the field name repeated again (value x) start. Would be better to strip out the both the field name and value x. For single value fields I agree with the start label should be below the field name with the field name having in separate div.

FluxSauce’s picture

133 fixes the PHP notice, I hesitated because of the 8.2.x. Sincere thank you!

For completeness (I'm migrating from a D7 site so I'm doing constant comparisons):

* Defaults - should have the ability to set distinct defaults for start and end. Here's the D7 interface, values are same, blank, now, strtotime:

Drupal 7

* User Interface: Checkbox for Show End Date - show_todate

FluxSauce’s picture

johannez’s picture

Hello guys,

I'm working currently on a custom field that includes start and end date plus has the option to repeat weekly/monthly. I'm happy to help out and convert my work into a patch, if interested.
One thing I noticed though was that the value for the core Datetime module is saved as an ISO standard string in the database. All the other dates in the system (created, changed, etc.) are saved as UNIX timestamps. Is there a strong argument to save it as that string instead of the common timestamp? If there was a previous discussion about that, please let me know. I couldn't find anything here.

mpdonadio’s picture

#142, thanks for the offer to help. Work on the current patch would be appreciated, especially test coverage; it's nearly ready other than that. I am not sure if we want to go down the road with a third restart. The current work is also keeping in mind views integration and how it will tie into some other parallel work (like the timezone mess).

You are strong encouraged, though, to publish your work as a contrib module, and then people have a choice. It may end up that recurrences stay in contrib anyway.

ISO dates where chosen a long time ago because they are easy to manipulate as strings, especially for database backends that lack date processing, for extracting the various parts.

johannez’s picture

Thank you, Matt, for the explanations.
I understand that ISO dates are easier to manipulate with db commands. However, not using timestamps is probably the reason we have that timezone mess ;)
However, I think it's manageable and I'll stay with saving datetime as ISO UTCs into the database.

I have to finish my client project first until the end of this month. Then I'll clean up the code and see, if I can provide a patch here or make it separate contrib module.
Once we got the end date down, it'll be easier to build the repeating date functionality as we can then re-use this module.

Regarding views integration, I noticed that the exposed filter for datetime only has a text field and not a pretty date selector widget. I'll look into that too.

13jupiters’s picture

@johannez in #144, I think you'll find that the datetime widget for exposed filters has been handled in #2648950: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter

jhedstrom’s picture

+++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateRangeItem.php
@@ -0,0 +1,162 @@
+class DateRangeItem extends FieldItemBase {

With these new plugins, is there any way they can share a common base that provides more value than just FieldItemBase? While not ideal, the value and value2 db columns used by D7 worked, and if we continued that here, then the range plugin could just extend the base fields of the single-date plugin. We also wouldn't need new type constants, and it might simplify views integration since at least one of the columns (value) would be shared between the field types.

Jaesin’s picture

If that were the case, the end data wouldn't need to be a separate field at all. Just a different widget or widget option like a "Collect an end date?" check box. Like what we see in #77.

mpdonadio’s picture

#90 probably represents the last real attempt to do this in the existing fields. Thoughts about the split start in #97 or so.

I don't have objection to using value, value2; I just chose new names b/c of the comments earlier in the issue about how bad these were :)

DuneBL’s picture

#132: Many thanks for the patch; here is my feedback

1_The warnings showed in #128 are gone in 8.2 (but still there in 8.1)

2_There is still a problem with the Start Label in Edit form (Single value):
=>the "Start" label is inline with the field label, it should be on the next line.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Hopping back on this. I have time to put another dent in this the next few days.

Going to continue forward with the idea of a new field for this, and start out with changing the schema back to 'value' and 'value2', and then adding an all-day option.

jonathanshaw’s picture

If I interpret #146 right, the suggestion is about architecture (2 columns of one field), not the naming of the fields/columns. So do continue to use better names than 'value' and 'value2' if you think that's right. I really appreciate your work here!

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
50.92 KB
11.57 KB

Renamed schema keys.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
53.09 KB
9.3 KB

First pass at all day.

jhedstrom’s picture

+++ b/core/modules/datetime/datetime.views.inc
@@ -11,42 +11,44 @@
+  if ($field_storage->getType() == 'datetime') {
+    // @todo This code only covers configurable fields, handle base table fields
+    //   in https://www.drupal.org/node/2489476.
+    $data = views_field_default_views_data($field_storage);
+    foreach ($data as $table_name => $table_data) {
+      // Set the 'datetime' filter type.
+      $data[$table_name][$field_storage->getName() . '_value']['filter']['id'] = 'datetime';

Since now 1 out of 2 columns are shared between the field types (value), this need no longer be conditional. A follow-up issue will need to add support for value2.

mpdonadio’s picture

#146, so I renamed the schema keys; that will help with the views integration. I think I want to get the tests in place first (and passing), then see if we can refactor to leverage the existing classes. Mainly, I think we are pretty much at the point where we need the tests to make sure we don't start going backwards (especially with some of other patches that are getting close, and will likely rebase/merge against).

Starting on tests...

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
74.17 KB
30.52 KB

Got about half the tests done; this is mostly copy-pasta from the old ones. Still need to go through all the deprecated functions in them...

Jaesin’s picture

Maybe this has already been brought up but If a customer decides they need to collect an end date on an existing datetime field, we will need to create a migration. That's a significant con for a new field type IMHO.

mpdonadio’s picture

#108 and #109 discuss migration with a new field, and is one of the reasons we are going down this road.

Hoping to post the first pass at the tests w/in the next 30 min. We are in the home stretch.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
106.05 KB
43.78 KB

The all-date option has a timezone bug that I need to look into. The value in storage ends up midnight-to-midnight UTC instead of in the user's local time. I'll look at that tomorrow. Rest of the features and tests should be good for review, other than layout issues. Want to get a front-end dev to help out with this to do it right.

KarenS’s picture

Nice work! Just a note that I have found that storing an all-day date as midnight can be confusing because doing a timezone conversion on it would shift the date in many cases. What I did was to store all-day dates as noon instead. That is less confusing, the date stored in the database always looks right and will in fact be right even if someone applies the wrong timezone to it.

Also, I totally concur with the idea of creating a new field for this purpose. It seems wrong to apply all this overhead to every place that simple dates are used, which is why I didn't add it to the base field. My intention was always to have a simple field for simple purposes and a more complex field only when all this overhead is actually required.

mpdonadio’s picture

Thanks Karen.

This is somewhat scattered across a few issues and some IRC, but once this lands we will have two day options. One is based on the day only from the existing field, and that still uses noon as it's time. However, we discovered that people in NZ are having problems with date rollover with this (see #2739290: UTC+12 is broken on date only fields). In that patch, we are removing timezone support for that option only, so the date is just a date.

This patch introduces all-day, which is really a date+time, where the time is always midnight to midnight using timezone handling (and we have another issue for zone selection upon input). So, there will be both uses cases available for people.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
107.43 KB
5.97 KB

Should be green. Manually test and review away.

Going to see a FE dev at DevDays wants to clean up the widget form; we know that needs TLC.

mpdonadio’s picture

Added some issues that will impact this, or will impact them depending on commit order.

Anonymous’s picture

Status: Needs review » Needs work
Issue tags: +DevDaysMilan

This is great, thank you for all the work!

I did some manual testing with date, datetime, and an all day field. Overall, it works very good.

For the allday field (and possibly the other ones, didn't check), I did find a warning in the logs:

Message Notice: Trying to get property of non-object in Drupal\datetime\Plugin\Field\FieldWidget\DateRangeWidgetBase->validateStartEnd() (line 164 of /var/www/html/core/modules/datetime/src/Plugin/Field/FieldWidget/DateRangeWidgetBase.php).

and

Warning: DateTime::diff() expects parameter 1 to be DateTimeInterface, object given in Drupal\Component\Datetime\DateTimePlus->__call() (line 298 of /var/www/html/core/lib/Drupal/Component/Datetime/DateTimePlus.php).

To reproduce, just create a new content type and add an allday field. Then create one item of your new type, filling in the allday field, and save.

Some nitpicks:

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -0,0 +1,115 @@
    +        /** @var \Drupal\Core\Datetime\DrupalDateTime $start_date */
    +        $end_date = $item->end_date;
    

    That should be $end_date in the comment.

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -0,0 +1,159 @@
    +        /** @var \Drupal\Core\Datetime\DrupalDateTime $start_date */
    +        $end_date = $item->end_date;
    

    And here again.

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -0,0 +1,159 @@
    +      '#description' => $this->t('Choose a format for displaying the dates. Be sure to set a format appropriate for the field, i.e. omitting time for a field that only has a date.'),
    

    The last part of this description was really confusing me until I tried all options. So the issue is that a date-only field will also display a time value if the format defines it? Shouldn't we just be saying that in the description?

pguillard’s picture

Status: Needs work » Needs review
FileSize
107.43 KB
2.29 KB

From #168

  • Corrected nitpicks 1 and 2 and a 3rd one that I found
  • I couldn't reproduce the warning (Date and time, Date only, and All Day options).
Anonymous’s picture

Apparently, the branch I was working on didn't start from 8.2.x. Judging from the comments above, this is probably the cause of that warning, so we don't need to look into that. Sorry for the noise.

mpdonadio’s picture

Status: Needs work » Needs review

Putting this back to NR b/c of an apparent TestBot fluke.

Re #168-3, that is nearly identical to the text in DateTimeDefaultFormatter. Since all of these fields are in memory as DrupalDateTime objects, the date-only values do have a time associated with them. And, the same formatters are used for both datetime and date-only variants. I'm open to suggestions here, but struggling for anything better.

jonathanshaw’s picture

Re #168.3 / 173 I agree the message is not great, but if it's the same as the default formatter, let's make it a separate issue. That helps keep this issue moving forward.

gambry’s picture

#169 works for me, just a note:

+++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
@@ -0,0 +1,115 @@
+        if ($this->getFieldSetting('daterange_type') == 'date') {

It should probably use the constant DateRangeItem::DATERANGE_TYPE_DATE on every condition you check $this->getFieldSetting('daterange_type'), but I can see constants are not used on the DateTime formatters too so I can see consistency on leaving everything as it is.

Great work!

DuneBL’s picture

I have created a date range fields before patch #169
If I run drush entup (after applying it), I receive a message to update the field:

The following updates are pending:

node entity type : 
  The field node.field_my_date_range should be updated.

Then I get some errors:
Exception thrown while performing a schema update. SQLSTATE[42S22]: Column not found: 1054 Unknown column 'field_my_date_range_value'

mpdonadio’s picture

#176, do you know when you made these? I think this may be because renamed the columns back to value/value2 to be consistent with the singular date and the Drupal 7 Date module.

DuneBL’s picture

It was very early in the patch live...
I assume I will have to recreate this field

jhedstrom’s picture

I started reviewing but ran out of time. I think this is very close.

Note several of these items are to pick up fixes from #2739290: UTC+12 is broken on date only fields, so if it becomes too burdensome, we could postpone on that (probably not necessary now, but if it goes back to needs work, then perhaps).

  1. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -1,5 +1,7 @@
    +# datetime
    
    @@ -83,3 +85,84 @@ field.widget.settings.datetime_datelist:
    +# daterange
    

    Coding standards nit pick: Should capitalize first letter, and add a period.

  2. +++ b/core/modules/datetime/src/DateTimeComputed.php
    @@ -40,10 +40,12 @@ public function getValue($langcode = NULL) {
    +    $storage_format = $item->getFieldDefinition()->getSetting($type . '_type') == 'date' ? DATETIME_DATE_STORAGE_FORMAT : DATETIME_DATETIME_STORAGE_FORMAT;
    
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -0,0 +1,115 @@
    + *)
    

    Needs a space between * and ).

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -0,0 +1,115 @@
    +        $this->setTimeZone($start_date);
    +        $this->setTimeZone($end_date);
    

    I think this will inherit the fix over in #2739290: UTC+12 is broken on date only fields, so hopefully no follow-up work will be needed when that lands.

  4. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -0,0 +1,115 @@
    +    $format = $this->getSetting('date_format');
    +    $timezone = $this->getSetting('timezone_override');
    +    return $this->dateFormatter->format($date->getTimestamp(), 'custom', $format, $timezone != '' ? $timezone : NULL);
    

    This should be re-worked as the fix over in #2739290: UTC+12 is broken on date only fields does (basically avoid setting timezone to null here, and instead inherit the date timezone if there isn't an override)

    		   protected function formatDate($date) {
         $format_type = $this->getSetting('format_type');
    -    $timezone = $this->getSetting('timezone_override');
    +    $timezone = $this->getSetting('timezone_override') ?: $date->getTimezone()->getName();
         return $this->dateFormatter->format($date->getTimestamp(), $format_type, '', $timezone != '' ? $timezone : NULL);
       }
    
  5. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -0,0 +1,159 @@
    +    $format_type = $this->getSetting('format_type');
    +    $timezone = $this->getSetting('timezone_override');
    +    return $this->dateFormatter->format($date->getTimestamp(), $format_type, '', $timezone != '' ? $timezone : NULL);
    

    Same change here as noted above.

  6. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -0,0 +1,159 @@
    +      $format = $this->dateFormatter->format(REQUEST_TIME, $type);
    

    Rather than use the global REQUEST_TIME constant, in other formatters (and views plugins) in the datetime module, we've injected the request (or in some cases the request stack) service to the formatter, and then used:

    $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME')

    I think that will be preferable here.

  7. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -0,0 +1,159 @@
    +    $date = DrupalDateTime::createFromTimestamp(REQUEST_TIME);
    

    Same as above.

  8. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeFormatterBase.php
    @@ -0,0 +1,174 @@
    +    $date->setTimeZone(timezone_open(drupal_get_user_timezone()));
    

    This needs a fix similar to the one over in #2739290: UTC+12 is broken on date only fields:

    -    $date->setTimeZone(timezone_open(drupal_get_user_timezone()));
    +    if ($this->getFieldSetting('datetime_type') === DateTimeItem::DATETIME_TYPE_DATE) {
    +      // A date without time has no timezone conversion.
    +      $timezone = DATETIME_STORAGE_TIMEZONE;
    +    }
    +    else {
    +      $timezone = drupal_get_user_timezone();
    +    }
    +    $date->setTimeZone(timezone_open($timezone));
    
  9. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangePlainFormatter.php
    @@ -0,0 +1,75 @@
    + *)
    

    Spacing nit here.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs review » Needs work
Issue tags: -Needs tests

NW to backport #2739290: UTC+12 is broken on date only fields to this patch. Unassigning myself in case I can't get to this in the next few days.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
110.25 KB
10.78 KB

Nits, before moving on to the date-only timezome change, so we can look at a cleaner interdiff.

mpdonadio’s picture

TZ change w/o test change. Still comes up green for me. Onto the test change itself.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
112.19 KB
14.75 KB

Possible spurious in #182?

This is green for me locally. If same, I can has reveiw?

mpdonadio’s picture

Issue summary: View changes
jhedstrom’s picture

Quick review here.

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangePlainFormatter.php
    @@ -0,0 +1,76 @@
    +class DateRangePlainFormatter extends DateRangeFormatterBase {
    
    +++ b/core/modules/datetime/src/Tests/DateRangeFieldTest.php
    @@ -0,0 +1,1220 @@
    +      // A timezone with an offset greater than UTC+12 is used.
    

    Nit, I think this comment went in accidentally with the UTC timezone fix, so shouldn't be copied here.

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangePlainFormatter.php
    @@ -0,0 +1,76 @@
    +    $timezone = $this->getSetting('timezone_override');
    +    return $this->dateFormatter->format($date->getTimestamp(), 'custom', $format, $timezone != '' ? $timezone : NULL);
    

    This one appears to be missing the UTC timezone fix.

dkre’s picture

#184 works perfectly (almost) applied to 8.1.7

The only issue I have found is when you have a multiple value field with a select list formatter. If the ampm value is unselected/unset an error message isn't displayed when 'add another item' is clicked but a validation error is shown on form submission.

Really great work.

jhedstrom’s picture

The only issue I have found is when you have a multiple value field with a select list formatter. If the ampm value is unselected/unset an error message isn't displayed when 'add another item' is clicked but a validation error is shown on form submission.

@dkre.one Does this happen with multivalue date fields without end dates (eg, without this patch)? Meaning, is that a bug introduced in this patch, or an existing bug? If the latter, then we should open a new issue to address that.

jonathanshaw’s picture

Can we plan a strategy for getting this into 8.2 before window closes august 3?

How about:
1) fix #186
2) investigate #187
3) move widget theming to a follow-up (may be eligible to go in during 8.2 beta period?)
4) mark rtbc (or is more in depth code review needed?)
5) create documentation follow ups?

mpdonadio’s picture

Anyone feel free to tackle #186 and #187, and/or the styling thing. I won't have time until Saturday. @jhedstrom shouldn't work on this, so he can review. I will also ping people at DrupalGovCon.

jonathanshaw’s picture

I cannot replicate #187 on a simplytest.me clean install of 8.2 with this patch. What I did:
Add date field, unlimited, to article content type
Select "select list" as widget for the field, choose "12 hour time" as time type, update, save.
Add a new article, select values for year, month, day, hour and minute but leave am/pm unselected.
Press "add another".
Then I see a validation error on the AM/PM, just as I do without this patch.
Possibly #187 arises because of applying patch #184 to 8.1.7 which does not have #2739290: UTC+12 is broken on date only fields.
I suggest ignore #187.

jonathanshaw’s picture

FileSize
113.43 KB
64.9 KB

I have tested widget/field combinations systematically and can see no theming issues with the widgets that are not also basically present with regular date fields. See screenshots below for a comparison.

Given this, and that theming can easily be done in a follow-up, and that theming can be done during beta period, and that theming should be postponed on #1918994: Improve Datetime and Daterange Widget accessibility which looks unlikely to make 8.2, I suggest we leave theming out of this issue.

If there are theming problems we should fix in this issue, please would someone specify them exactly.


mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Will have #186 as soon as I get back. Expect patch tonight.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Issue summary: View changes
FileSize
112.18 KB
1.76 KB

#184 addressed.

jhedstrom’s picture

I am trying to rally a few more folks to review.

move widget theming to a follow-up (may be eligible to go in during 8.2 beta period?)

Let's open that issue and reference it here.

mpdonadio’s picture

Instead of a followup, I think #195, I think we can revive #1918994: Improve Datetime and Daterange Widget accessibility as the solution for any theming issues, as that is the better for the long term and a11y. That is in the IS right now.

generalredneck’s picture

Just a note... it appears that there has been some back and forth on which (if any) should allow NULL. Currently from my testing neither start or end date are allowed to be NULL.

mpdonadio’s picture

#197, that is expected now that this is a separate field.

jhedstrom’s picture

I spent some time going over this very closely. It is looking good.

One tiny nit, and one architectural question/comment:

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -0,0 +1,116 @@
    + *)
    

    Tiny nit, missing a space between the '*' and the ')'.

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeFormatterBase.php
    @@ -0,0 +1,194 @@
    +abstract class DateRangeFormatterBase extends FormatterBase implements ContainerFactoryPluginInterface {
    ...
    +   * Constructs a new DateTimeDefaultFormatter.
    

    Code comment is out of whack with the class name.

    Looking at this, does it make sense to extend FormatterBase and replicate all of the code in DateTimeFormatterBase?
    It looks like if that were extended, much of this code wouldn't need to be duplicated...

    Extending the existing formatter base will make follow-up easier (for instance the timezone support issue).

mpdonadio’s picture

#199 addressed.

Architecture. A while ago, we decided for expediency (ha!) to keep these on separate tracks w/o a common base in case we ran into problem or rebase hell with the other issues, get it in, and then refactor for commonality. I think at this point we just need to get this done, and then create a followup.

dawehner’s picture

This is looking really great, IMHO. I think most points from below can be totally solved in follow ups.

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -0,0 +1,116 @@
    +    $date = DrupalDateTime::createFromTimestamp($this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME'));
    
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -0,0 +1,160 @@
    +      $format = $this->dateFormatter->format(REQUEST_TIME, $type);
    ...
    +    $date = DrupalDateTime::createFromTimestamp($this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME'));
    

    Follow up work: Use the request stack twice

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -0,0 +1,160 @@
    +    foreach ($items as $delta => $item) {
    +      if ($item->start_date && $item->end_date) {
    +        /** @var \Drupal\Core\Datetime\DrupalDateTime $start_date */
    +        $start_date = $item->start_date;
    +        /** @var \Drupal\Core\Datetime\DrupalDateTime $end_date */
    +        $end_date = $item->end_date;
    +
    +        if ($this->getFieldSetting('daterange_type') == DateRangeItem::DATERANGE_TYPE_DATE) {
    +          // A date without time will pick up the current time, use the default.
    +          datetime_date_default_time($start_date);
    +          datetime_date_default_time($end_date);
    +        }
    +
    +        // Create the ISO dates in Universal Time.
    +        $start_iso_date = $start_date->format("Y-m-d\TH:i:s") . 'Z';
    +        $end_iso_date = $end_date->format("Y-m-d\TH:i:s") . 'Z';
    +
    +        $this->setTimeZone($start_date);
    +        $this->setTimeZone($end_date);
    +
    
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangePlainFormatter.php
    @@ -0,0 +1,76 @@
    +    foreach ($items as $delta => $item) {
    +      if ($item->start_date && $item->end_date) {
    +        /** @var \Drupal\Core\Datetime\DrupalDateTime $start_date */
    +        $start_date = $item->start_date;
    +        /** @var \Drupal\Core\Datetime\DrupalDateTime $end_date */
    +        $end_date = $item->end_date;
    +
    +        if ($this->getFieldSetting('daterange_type') == DateRangeItem::DATERANGE_TYPE_DATE) {
    +          // A date without time will pick up the current time, use the default.
    +          datetime_date_default_time($start_date);
    +          datetime_date_default_time($end_date);
    +        }
    +
    +        $this->setTimeZone($start_date);
    +        $this->setTimeZone($end_date);
    

    Possible follow up: A lot of this code is really similar, I could imagine this could be refactored a bit.

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangePlainFormatter.php
    @@ -0,0 +1,76 @@
    +/**
    + * Plugin implementation of the 'Plain' formatter for 'daterange' fields.
    + *
    

    For future reference, IMHO it could be helpful that these documentations explain what is special about those formatters ...

  4. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangePlainFormatter.php
    @@ -0,0 +1,76 @@
    diff --git a/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    
    diff --git a/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    index 678ddc8..c4abd2d 100644
    
    index 678ddc8..c4abd2d 100644
    --- a/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    
    --- a/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -15,7 +15,7 @@
    
    @@ -15,7 +15,7 @@
      *   field_types = {
      *     "datetime"
      *   }
    - *)
    + * )
      */
     class DateTimeCustomFormatter extends DateTimeFormatterBase {
     
    diff --git a/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    
    diff --git a/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    index 683d75c..a74810a 100644
    
    index 683d75c..a74810a 100644
    --- a/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    
    --- a/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    @@ -14,7 +14,7 @@
    
    @@ -14,7 +14,7 @@
      *   field_types = {
      *     "datetime"
      *   }
    - *)
    + * )
    

    This could be moved to a possible follow up

  5. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateRangeFieldItemList.php
    @@ -0,0 +1,149 @@
    +/**
    + * Represents a configurable entity daterange field.
    + */
    +class DateRangeFieldItemList extends FieldItemList {
    +
    

    Possible follow up/future reference: Describe why we need this class. What is special about date ranges, that it needs its own field item class.

  6. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateRangeFieldItemList.php
    @@ -0,0 +1,149 @@
    +              ':input[id="edit-default-value-input-default-start-date-type"]' => ['value' => static::DEFAULT_VALUE_CUSTOM],
    ...
    +              ':input[id="edit-default-value-input-default-end-date-type"]' => ['value' => static::DEFAULT_VALUE_CUSTOM],
    

    Possible followup: IMHO we shouldn't rely on IDs for the #states API, as they might change in ajax requests. Use name instead?

  7. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateRangeItem.php
    @@ -0,0 +1,172 @@
    +class DateRangeItem extends FieldItemBase {
    

    Shouldn't this class override mainPropertyName()? Otherwise this method returns 'value' which isn't right, that's for sure

  8. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateRangeItem.php
    @@ -0,0 +1,172 @@
    +      ->setClass('\Drupal\datetime\DateTimeComputed')
    ...
    +      ->setClass('\Drupal\datetime\DateTimeComputed')
    

    Follow up, nitpick: Use DateTimeComputed::class

  9. +++ b/core/modules/datetime/src/Tests/DateRangeFieldTest.php
    @@ -0,0 +1,1219 @@
    +  function testAlldayRangeField() {
    ...
    +  function testDatelistWidget() {
    

    Nitpick followup: Add public to those methods

  10. +++ b/core/modules/datetime/src/Tests/DateRangeFieldTest.php
    @@ -0,0 +1,1219 @@
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-year\"]", NULL, 'Year element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value-year", '', 'No year selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value-year", t('Year'));
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-month\"]", NULL, 'Month element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value-month", '', 'No month selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value-month", t('Month'));
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-day\"]", NULL, 'Day element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value-day", '', 'No day selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value-day", t('Day'));
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-hour\"]", NULL, 'Hour element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value-hour", '', 'No hour selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value-hour", t('Hour'));
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-minute\"]", NULL, 'Minute element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value-minute", '', 'No minute selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value-minute", t('Minute'));
    +    $this->assertNoFieldByXPath("//*[@id=\"edit-$field_name-0-value-second\"]", NULL, 'Second element not found.');
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-ampm\"]", NULL, 'AMPM element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value-ampm", '', 'No ampm selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value-ampm", t('AM/PM'));
    +
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-year\"]", NULL, 'Year element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value2-year", '', 'No year selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value2-year", t('Year'));
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-month\"]", NULL, 'Month element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value2-month", '', 'No month selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value2-month", t('Month'));
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-day\"]", NULL, 'Day element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value2-day", '', 'No day selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value2-day", t('Day'));
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-hour\"]", NULL, 'Hour element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value2-hour", '', 'No hour selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value2-hour", t('Hour'));
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-minute\"]", NULL, 'Minute element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value2-minute", '', 'No minute selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value2-minute", t('Minute'));
    +    $this->assertNoFieldByXPath("//*[@id=\"edit-$field_name-0-value2-second\"]", NULL, 'Second element not found.');
    +    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-ampm\"]", NULL, 'AMPM element found.');
    +    $this->assertOptionSelected("edit-$field_name-0-value2-ampm", '', 'No ampm selected.');
    +    $this->assertOptionByText("edit-$field_name-0-value2-ampm", t('AM/PM'));
    

    It almost seems to be that we should better have some foreach loop over ['year', 'month' ...] and ['value', 'value2']

mpdonadio’s picture

Non f/up items from #201.

mpdonadio’s picture

Not sure what proper fix is for the FieldTypePluginManagerTest::testMainProperty() fail? DateRangeItem is a compound value, so it doesn't have a main value column?

swentel’s picture

@mpdonadio just leave out the mainPropertyName() method. DateTimeItem doesn't implement it either (and thus falls back to 'value'). Should be fine for this one too, you can argue that 'value' is the 'mainest' property here :)

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
109.56 KB

Thanks @swentel.

mpdonadio’s picture

FileSize
3.78 KB

Interdiff didn't attach first time around.

dawehner’s picture

@mpdonadio just leave out the mainPropertyName() method. DateTimeItem doesn't implement it either (and thus falls back to 'value'). Should be fine for this one too, you can argue that 'value' is the 'mainest' property here :)

But isn't the problem that we don't store any value in the first place in 'value' and by that would always return NULL?

mpdonadio’s picture

#209, we reverted back to 'value' and 'value2' for the actual schema to make ports easier, and to potentially allow some base classes between datetime and daterange. We use better variable names in PHP instead.

mpdonadio’s picture

Status: Needs work » Needs review

Looks like some spurious update fails. Back to NR.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

I think this is ready to go!

Could somebody add follow up issues for #201?

Thanks everybody!

+++ b/core/modules/datetime/src/Tests/DateRangeFieldTest.php
@@ -537,45 +537,12 @@ function testDatelistWidget() {
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-year\"]", NULL, 'Year element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value-year", '', 'No year selected.');
-    $this->assertOptionByText("edit-$field_name-0-value-year", t('Year'));
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-month\"]", NULL, 'Month element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value-month", '', 'No month selected.');
-    $this->assertOptionByText("edit-$field_name-0-value-month", t('Month'));
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-day\"]", NULL, 'Day element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value-day", '', 'No day selected.');
-    $this->assertOptionByText("edit-$field_name-0-value-day", t('Day'));
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-hour\"]", NULL, 'Hour element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value-hour", '', 'No hour selected.');
-    $this->assertOptionByText("edit-$field_name-0-value-hour", t('Hour'));
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-minute\"]", NULL, 'Minute element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value-minute", '', 'No minute selected.');
-    $this->assertOptionByText("edit-$field_name-0-value-minute", t('Minute'));
-    $this->assertNoFieldByXPath("//*[@id=\"edit-$field_name-0-value-second\"]", NULL, 'Second element not found.');
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value-ampm\"]", NULL, 'AMPM element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value-ampm", '', 'No ampm selected.');
-    $this->assertOptionByText("edit-$field_name-0-value-ampm", t('AM/PM'));
-
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-year\"]", NULL, 'Year element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value2-year", '', 'No year selected.');
-    $this->assertOptionByText("edit-$field_name-0-value2-year", t('Year'));
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-month\"]", NULL, 'Month element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value2-month", '', 'No month selected.');
-    $this->assertOptionByText("edit-$field_name-0-value2-month", t('Month'));
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-day\"]", NULL, 'Day element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value2-day", '', 'No day selected.');
-    $this->assertOptionByText("edit-$field_name-0-value2-day", t('Day'));
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-hour\"]", NULL, 'Hour element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value2-hour", '', 'No hour selected.');
-    $this->assertOptionByText("edit-$field_name-0-value2-hour", t('Hour'));
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-minute\"]", NULL, 'Minute element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value2-minute", '', 'No minute selected.');
-    $this->assertOptionByText("edit-$field_name-0-value2-minute", t('Minute'));
-    $this->assertNoFieldByXPath("//*[@id=\"edit-$field_name-0-value2-second\"]", NULL, 'Second element not found.');
-    $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-value2-ampm\"]", NULL, 'AMPM element found.');
-    $this->assertOptionSelected("edit-$field_name-0-value2-ampm", '', 'No ampm selected.');
-    $this->assertOptionByText("edit-$field_name-0-value2-ampm", t('AM/PM'));
+    foreach (['value','value2'] as $column) {
+      foreach (['year', 'month', 'day', 'hour', 'minute', 'ampm'] as $element) {
+        $this->assertFieldByXPath("//*[@id=\"edit-$field_name-0-$column-$element\"]", NULL, $element . ' element found.');
+        $this->assertOptionSelected("edit-$field_name-0-value-year", '', 'No ' . $element . ' selected.');
+      }
+    }

I love this refactor!

mpdonadio’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
109.57 KB
829 bytes

Just found one oopsie in the last refactor. Passes locally.

xjm’s picture

Title: Support for end date » Add a Date Range field type with support for end date

Great work on this issue; very clean and good test coverage. A few comments scanning through the patch.

  1. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -1,5 +1,7 @@
    +# Datetime file type.
    

    I think this should say "field type" rather than file type?

  2. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -83,3 +85,84 @@ field.widget.settings.datetime_datelist:
    +# Daterange field type.
    ...
    +  label: 'Daterange settings'
    ...
    +  label: 'Daterange settings'
    ...
    +  label: 'Daterange default display format settings'
    ...
    +  label: 'Daterange plain display format settings'
    ...
    +  label: 'Daterange custom display format settings'
    ...
    +  label: 'Daterange select list display format settings'
    ...
    +  label: 'Daterange default display format settings'
    

    "Daterange" is not an English word, so I think it should be two words everywhere in strings. (I'm fine with daterange as a machine name though. An underscore might be better but I have no strong opinion.)

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -15,7 +15,7 @@
    - *)
    + * )
    
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimePlainFormatter.php
    @@ -14,7 +14,7 @@
    - *)
    + * )
    

    I think these changes are out of scope.

xjm’s picture

Issue tags: +beta deadline

@mpdonadio reached out asking me whether this could have some exception to the beta deadline. We're discussing whether that would be okay, but in the meanwhile, adding the beta deadline tag for visibility so that this gets prioritized in the RTBC queue once it is back to RTBC.

mpdonadio’s picture

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC assuming #216 goes green.

dawehner’s picture

Title: Add a Date Range field type with support for end date » Support for end date
Issue tags: -beta deadline

I'm also really happy. It would be great if someone take the time to look at some of the follow ups I propose, but IMHO this totally don't block the RTBC.

dawehner’s picture

Title: Support for end date » Add a Date Range field type with support for end date
Issue tags: +beta deadline

I didn't meant to change things!

generalredneck’s picture

So the major schema change was a big headache for sites that already have data and using this... But I know that's what happens when using patches :P

With that said, This is working pretty well minus the comments I (and others) have made above about the inconsistancies of features between this and D7.

One other Nit Pick... Because of the schema change, Views now shows the following:

http://content.screencast.com/users/allan4k/folders/Jing/media/e15d5481-...

but again... none of this stoping me from saying SHIP IT :P (and please no more schema changes without updates)

colan’s picture

Issue tags: +Needs followup

Additional follow-ups can be created for missing D7-like things.

Adding tag so that we remember to add the follow-ups we know we need, discussed earlier.

jhedstrom’s picture

xjm’s picture

I cannot reproduce the Views duplication @generalredneck indicates. Does this only occur on a site where an earlier version of the patch has been applied? (Also, there is not, and will not ever be, any expectation that patch authors need to provide an upgrade path between different versions of a patch. If you want an upgrade path for that, write one.) It also does not make sense to provide an upgrade path from an existing field type to a new one, because we have no way of knowing whether the site author wants an end date. So I think we are all good on that front.

I did a bit of manual testing and it works well! I am first presented with these options in the field selection:

One concern I had about the approach in this patch was that adding an additional field type would make the already overwhelming field selection interaction more overwhelming, but I don't think that is the case. It's easy for me to understand the difference between "Date" and "Date range".

Then I configured the default value:

All good so far. Next I tested the field:

This is the first thing that is a little funky. "Release window Start" is goofy capitalization for English. This makes me suspect either a string like @foo Start that should be @foo start for the English default, or that two strings might be being concatenated together in a less translation-friendly way. (I am not sure which because I haven't done a code review yet.) I also couldn't figure out any way to configure these labels either; it didn't seem to be an option on (e.g.) the form display settings for the field.

Next I tested the validation for an end date before a start date:

Worked as I expected. Edit: Except the validation message could be a little better. How about "The @label end date cannot be before the start date"?

I entered a valid date range and then looked at the default formatter:

Looks good to me!

Finally I tested changing the formatter:

I found the differences between "Plain", "Custom", and "Default" non-intuitive; "Default" provided a lot more options than I expected a default to; and it seemed to me like Default and Custom were partly redundant. But I checked and confirmed these things are all true of the existing Date field type as well, so not introduced here.

Leaving at RTBC for now until I've had a chance to do a code review, but the bit about the start date label (edit: and the validation message) might be actionable.

xjm’s picture

Issue summary: View changes
FileSize
57.17 KB

(Uploading correct screenshot.)

xjm’s picture

One other thing I've just noticed. The UI label in the screenshot above when choosing the field type appears to be "Date Range" (title case) but our text standards specify sentence case (like "Taxonomy term" in the screenshot).

xjm’s picture

Hmm, so, I've started a code review of the patch, and I'm a bit concerned about the amount of code duplication between this and the existing Date field type and its widgets/formatters. I see there is #2775669: Clean up redundant methods in datetime field formatters and #2775671: Datetime field items can extend from a single base class but I'm not sure I'm comfortable with those as followups given the sheer scope of the duplication, especially to add this as a stable feature. Every time we have a bug, we have to remember to check whether it needs to be fixed in both places, etc. This adds a lot of potential technical debt.

My feeling at the moment is that we would need to take care of the bulk of this code duplication now, before commit. In other circumstances we might commit something as an experimental module instead and make the refactoring a required part of the path to stable, but I don't think that is a good fit in this case, because then we are painting ourselves into a corner in terms of moving it back into datetime once it's stable. It's not like a UI that can just be moved into another module later without too much disruption; it includes a storage and so we'd get into the same situation needing an update path.

I'll get a second opinion from another committer on whether the issues to refactor away the code duplication can be followups.

mpdonadio’s picture

Thanks for taking the time to look at this issue.

My biggest concern is that the refactoring may not be as straightforward as it appears because of potential subtleties between the two types. For example, if the date range FieldItem extends the existing one instead of FieldItem, then the range item will have both sets of constants. Internally, there are naming convention differences (date vs start_date/end_date). There may be other things. This could potentially spill over into needing test changes, and then we are back to chasing our own tail with refactoring and fixing tests at the same time. I'm not saying this shouldn't be explored, but I think it may be a bigger exercise than we think to do all in one step to get into 8.2.0 on time; smaller refactors through separate issues may be safer.

However, I will defer to committers on what is best here.

generalredneck’s picture

@xjm

I cannot reproduce the Views duplication @generalredneck indicates. Does this only occur on a site where an earlier version of the patch has been applied?

Negative
First you misunderstood the issue. There isn't a "views duplication". I was simply stating that the views field names were not... super which is why I didn't make a big deal out of it.

To reproduce:

  • create a date range field on a content type
  • create a new view
  • click add next to "fields" or "sort" and search for the name of your field

In my test I have a daterange field called "Date" with the machine name of "field_date"

Notice that it shows the following when searching for handlers as shown in the screenshot above:

Date (field_date)
Date (field_date:value2)

What would be more correct would be something along the lines of

Date:Start Date (field_date)
Date:End Date (field_date:value2)

or something to indicate that "value2" is in fact the end date as it would take a developer to understand this.

This is my mistake for not giving the proper format for reporting

  • What is happening
  • Steps to Reproduce
  • Expected Results

(Also, there is not, and will not ever be, any expectation that patch authors need to provide an upgrade path between different versions of a patch. If you want an upgrade path for that, write one.)

Understood

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for clarifying @generalredneck; I was able to reproduce that too. Because of the schema itself for the field:

+++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateRangeItem.php
@@ -0,0 +1,173 @@
+  public static function schema(FieldStorageDefinitionInterface $field_definition) {
+    return [
+      'columns' => [
+        'value' => [
+          'description' => 'The start date value.',
+          'type' => 'varchar',
+          'length' => 20,
+        ],
+        'value2' => [
+          'description' => 'The end date value.',
+          'type' => 'varchar',
+          'length' => 20,
+        ],
+      ],
+      'indexes' => [
+        'value' => ['value'],
+        'value2' => ['value2'],
+      ],
+    ];

I agree that value and value2 are not sufficiently clear key names. (I see also that this was raised earlier on the issue, e.g. by @Berdir.)

So I think this patch needs work at least for that point as well as the string changes suggested in #223. Still waiting for a second opinion on whether the refactor should also be blocking.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
112.12 KB
6.22 KB

Ok, string updates are in this version.

Re the schema, we decided to go back to value/value2 for a few reasons (see #146). That is consistent with how the Drupal 7 Date module did things, so that may may migrations easier. Using value/value2 instead of start_date/end_date (a few patches had this) will allow for potential refactoring to allow reuse between DateTime and DateRange classes. It will also likely allow the same thing to happen when we do real views integration (which was decided to not be part of this because of the added complexity, some views related work that is still ongoing, and the potential desire to wrap in the timezone support into views at the same time). I do think this is the proper decision here, so I did not redo that.

Views. Nice catch. See #156. For the time being, I put that hunk back in, so the handlers don't try to get wired in, but you can at least display the field value in a view (since field values default to use the formatters and not plugins).

xjm’s picture

Issue summary: View changes

Thanks @mpdonadio.

I'm not sure I agree with #146. For me, what contrib did in D7 is not a reason to introduce a similar vagueness in the schema in D8. Let's get an entity or framework manager's review on that too. I'll add these two things to the remaining tasks in the summary.

Meanwhile, a couple of very small nits with the latest interdiff:

+++ b/core/modules/datetime/src/Tests/DateRangeFieldTest.php
@@ -1089,7 +1089,7 @@ public function testInvalidField() {
-    $this->assertText('Start date should be equal to, or before, end date', 'End date before start date has been caught.');
+    $this->assertText(t('The @title end date cannot be before the start date', ['@title' => $field_name]), 'End date before start date has been caught.');

@@ -1098,7 +1098,7 @@ public function testInvalidField() {
-    $this->assertText('Start date should be equal to, or before, end date', 'End time before start time has been caught.');
+    $this->assertText(t('The @title end date cannot be before the start date', ['@title' => $field_name]), 'End time before start time has been caught.');

These should use FormattableMarkup instead of t().

mpdonadio’s picture

Oops, thanks for the reminder. Fixed, and passes locally.

I don't think parity with Drupal 7 is the primary factor with the schema key names, but more with the DateTime classes to allow extension, where possible (eg, inherit the value behavior from the the DateTime class and then add support for the value2 in the DateRange class, where possible). That said, I do agree value2 is a bad name, but maybe not as bad as using value/end_value because that option wouldn't be symmetrically descriptive.

mpdonadio’s picture

Status: Needs work » Needs review
catch’s picture

What's wrong with start_value and end_value?

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
110.19 KB
37.94 KB

Fixed a bunch of code style issues, no logic changes.

As someone who spent a lot of time dealing with D7 Date module, I think that keeping value/value2 is beneficial. For example, when handling a large set of dates with and without ranges, you can rely on ->value always being there (also when doing Views or direct DB access).

I'm going to tentatively set this back to RTBC on those grounds.

xjm’s picture

Thanks @tim.plunkett.

Even if value is the name for the start date, I still think value2 is not a good name for the end date.

xjm’s picture

As someone who spent a lot of time dealing with D7 Date module, I think that keeping value/value2 is beneficial. For example, when handling a large set of dates with and without ranges, you can rely on ->value always being there (also when doing Views or direct DB access).

Another difference AFAIK is that with this field both start and end values are required. Neither is allowed to be NULL.

mpdonadio’s picture

#237: Even if value is the name for the start date, I still think value2 is not a good name for the end date.

Keeping 'value' and picking something new for 'value2' would (1) alleviate my concerns with a refactor (2) be mostly easy to implement. Open to suggestions; think 'end_value' would be fine. If nobody else come up with some better, I can implement that tonight.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -beta deadline +beta target

Discussed further with @effulgentsia and @catch.

We would like to see refactoring the date field, widget, and formatter code to possibly use some base classes and/or traits explored before we commit this. In turn, we agreed to make the issue a beta target, since it is RTBC now otherwise. That means we'd like to consider adding this feature during the beta if it is ready within the next few weeks, ideally before we create a second beta.

That way, if the refactor turns out to be problematic, we can also go back to this version of the path.

If the issue turns out to not be RTBC within the beta phase, it can still be one of the first features committed to 8.3.x this month or next, which will also be beneficial to site owners and contrib authors looking forward to this feature.

Thanks @mpdonadio, @jhedstrom, @tim.plunkett et al!

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Many thanks to the various managers and others who have been looking into this. I really appreciate it.

I have a plan worked out for this in my head. Will be posting incremental patches, starting with the schema key change (value2 => end_value), then the field items, then widgets, then formatters (there is a tricky part here that I want to eventually brainstorm). And we will consider #236 the reference patch.

Timing should be OK.

mpdonadio’s picture

OK. Schema key change. value2 => end_value. DateRangeFieldTest passes locally.

mpdonadio’s picture

Ok, here are refactors of DateRangeItem and DateRangeFieldItemList. DateRangeFieldTest passes locally. This is best read applied.

Changes to DateRangeItem are mostly straightforward. The constants are inherited, and the new one is added in the same style. It also changes the settings key to the inherited value, which should make more of the widgets and formatters reusable.

Changes to DateRangeFieldItemList are a balance between proper reuse and forcing things to be reusable. For example, the form elements can be inherited w/ minor alterations, but using the parent submit and validate functions would require shenanigans. I also uncovered a bug in the default values, where you could not set only one; that logic made this not a good candidate for reuse.

Comments appreciated. Will start widgets next, likely tomorrow night.

joelpittet’s picture

Status: Needs work » Needs review

Random test failure, setting status back to needs review.

mpdonadio’s picture

Ok, here are some small additional changes to the field items, and a pass at some refactoring the widget bases.

The widgets themselves are a conundrum. The default widget is different enough b/c all-day that any real changes seem forced. The list widget has common code, but it this gets moved to a trait, then you lose the connection to WidgetBase. Ideas?

Note that in the process of doing this, I uncovered #2778083: Default value for Date-Only fields is broken when UTC date is different than user current date.

This passes locally. Will start formatters next, likely tomorrow night but may not finish until Thursday or Friday. The plan for those is

- ditch DateRangeFormatterBase
- refactor the DateTime formatters to have a method that builds the render array for a single item, and use this in viewElements
- have the DateRange formatters extend the equivalent DateTime
- the DateRange viewElements would then call the method to build a single item (which would be inherited), and then just glue them together with the separator

I think that approach will work, and will be good in the long run.

As always, comments appreciated.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
FileSize
109.98 KB
105 KB
31.03 KB

OK, I think we are cooking with gas now. This includes some small cleanups of previous patches, plus the refactored formatters. I really like how these turned out. I had to make some changes to the DateTime formatters to make this possible, but no changes were made to DateTimeFieldTest.

DateTimeFieldTest and DateRangeFieldTest pass locally.

Tagging out. Think this is ready for review. Interdiffs against the #246 and #236 are included. This is probably best reviewed applied, as the interdiffs are pretty messy.

Berdir’s picture

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -0,0 +1,95 @@
    +          $elements[$delta] = [
    +            $this->buildDate($start_date),
    +            ['#plain_text' => ' ' . $separator . ' '],
    +            $this->buildDate($end_date),
    +            $this->defaultCacheContext(),
    +          ];
    

    this confused me a bit, non-explicit array keys are a bit uncommon with render arrays. with the cache contexts on the top element. There is also a risk that cache contexts are lost, if people print out the dates separately for customized output.

    Might make sense to have this as its own template so it is easier to customize. Also, what about RTL languages?

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeDefaultFormatter.php
    @@ -35,45 +36,17 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
    +        // Display the date using theme datetime.
    +        $elements[$delta] = $this->buildDate($item->date) + $this->defaultCacheContext();
    

    is there a case where the cache context is *not* added? maybe just do it in buildDate(), the range fields have it twice then but that's not really an issue. Skipping them as described above should also not be an issue then.

  3. +++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateRangeItem.php
    @@ -0,0 +1,131 @@
    +  public function isEmpty() {
    +    $start_value = $this->get('value')->getValue();
    +    $end_value = $this->get('end_value')->getValue();
    +    return ($start_value === NULL || $start_value === '') && ($end_value === NULL || $end_value === '');
    +  }
    

    that means the start date is only stored if there is an end date. is that not a supported use case (I think not filling out the end date if it is the same is pretty common).

    maybe both fields are required in the widget anyway, didn't get there yet.

  4. +++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateRangeDatelistWidget.php
    @@ -0,0 +1,157 @@
    +    $element['end_value'] = [
    +      '#type' => 'datelist',
    +      '#date_increment' => $increment,
    +      '#date_part_order' => $date_part_order,
    +    ] + $element['end_value'];
    

    hm, not seeing any special logic. how does this work, also with a required vs non-reqiured field exactly?

mpdonadio’s picture

Thanks @Berdir, I'll fix up 1,2 with the keys and contexts. I went back and forth on them anyway. Have to think about templates and RTL support.

3, We have both as mandatory here since they are separate fields now; widgets enforce this (later patches have test coverage for this). If you don't want an end date, then use a Datetime field instead.

4. That is a near lift from the way the existing DateTimeDatelistWidget works. The top of the method calls DateRangeWidgetBase, the top of which calls DateTimeWidgetBase. If you trace through those, you will see how the wrapper, required, values, etc, all get set up. That particular hunk is just merging in the parts to make the end date work list a select list. The rest of the $element has been taken care of through the parent calls.

markabur’s picture

3, We have both as mandatory here since they are separate fields now; widgets enforce this (later patches have test coverage for this). If you don't want an end date, then use a Datetime field instead.

In my experience, something like an event calendar will have a mix of single-day and multi-day events. In the past I've always been able to use one date field for this, and my clients have never needed to fill out the end date for all-day single-day events.

Would it be possible for the widget to work like date module for D7, where the end date is optional and there is a "Collect an end date" checkbox to show/hide it? If the checkbox is left unchecked, the end date widget would be hidden, but behind the scenes maybe it could automatically use the same value as the start date?

mpdonadio’s picture

#250, it is definitely possible, but I am hesitant to add this if we want to hit the 8.2.0 deadline, especially after having an RTBC version of this that we are just refactoring now.

Both widgets would need to be updated, along with test coverage and test changes for how end date being required is handled. If the start date and end date are the same, then the formatters already only show one (mostly, all-day is really a date+time from 00:00 to 23:59, so it is always range in storage).

This is something that could definitely be tackled in https://www.drupal.org/project/datetime_extras, especially since we aren't trying to exactly replicate the Drupal 7 Date module 1-for-1 feature-wise in core. One of the reasons we made that module is so we could have a place for special formatters, widgets, and things that are going to be tough (like proper recurrence support).

colan’s picture

Agreed. Let's make it optional in that contrib module or in another follow-up so we can make the deadline here. It won't change the data model so we shouldn't run into too many problems.

mpdonadio’s picture

This should take care of #248 1 and 2, but not using a separate template; 3 and 4 were answered earlier.

Going to play the Stupid American card, and ask what the proper way to handle RTL is? Inject LanguageManager, then check $languageManager->getCurrentLanguage()->getDirection() and change the element order if the result is DIRECTION_RTL? Not really seeing anything in core as an example (may not be tracing this out right or fully).

jhedstrom’s picture

Inject LanguageManager, then check $languageManager->getCurrentLanguage()->getDirection() and change the element order if the result is DIRECTION_RTL?

This seems like a sound approach. In ckeditor.admin.inc there is some use of this, but it didn't look like it was swapping the order of anything, but rather using a different image button based on RTL.

I will be out most of next week, so am going to say this is again RTBC from my perspective at this point.

mpdonadio’s picture

#254 do you think RTL is a RTBC blocker here, or can we do that in a followup? I would want to make sure we do that properly and add test coverage if it is a blocker.

jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

I don't see it as a blocker, so let's add a follow-up. Back to RTBC!

mpdonadio’s picture

Status: Needs work » Reviewed & tested by the community

Back to RTBC; sporadic fail up updates tests, https://www.drupal.org/pift-ci-job/408095.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mpdonadio’s picture

Version: 8.3.x-dev » 8.2.x-dev

Setting this back to 8.2.x since @xjm added this as Beta Target.

mpdonadio’s picture

Sent a msg to @Mixlogic to try to see whether the latest fails are patch related or bot related. Not sure why we have passes, and multiple versions of fails here.

mpdonadio’s picture

Per @Mixlogic, this looks like a general problem with those tests (happening elsewhere). Will set this back to RTBC when we know the nightly test runs will be OK.

mpdonadio’s picture

Status: Needs work » Reviewed & tested by the community

@Mixlogic suggested resetting the status and this looks like a problem with the migrate tests in the CI environment and may take some time to hash out.

sylus’s picture

Just attaching a 8.x-1.x compatible backport for composer / drush make / testing purposes.

mpdonadio’s picture

Since #2739290: UTC+12 is broken on date only fields did not get a backport to 8.1.x, I am not sure the patch in #267 will work reliably.

sylus’s picture

Thanks for the tip! I haven't run across that issue during my prototyping yet but good to know!

While looking at this code I noticed I needed to make a slight tweak to get date_ranges to properly work with arguments in views. Since date_range is essentially a datetime when the two fields are taken into account the simple fix for me seems to work.

+++ b/core/modules/datetime/datetime.views.inc
@@ -11,6 +11,10 @@
+  if ($field_storage->getType() !== 'datetime') {
+    return [];
+  }
+

I was wondering if this should instead be:

  if ($field_storage->getType() !== 'datetime' &&
      $field_storage->getType() !== 'daterange') {
    return [];
  }
mpdonadio’s picture

#269, thank hunk of code is to disable the views plugins for daterange fields, but allow them for datetime fields. The plugins somewhat work with views right now, but not fully enough to warrant not having that code (see #228 and the few comments that follow it). We will handle proper support in a followup. The original patch for this was a bear (the refactoring we did should help this), and we may want to wait for #2627512: Datetime Views plugins don't support timezones to be committed and #2632040: [PP-1] Add ability to select a timezone for datetime field to be wrapped up before tacking them for datetime ranges. That is why we are pushing to get this into 8.2.x, as it does block a lot of planned work.

Zythyr’s picture

I have been following this issue and it is my understanding you guys have a possible solution but it will take some time to test and make it stable. I am not a developer so I don't completely everything that was discussed. I have a question regarding implementation of a date field on my live D8 site.

I plan to launch a live D8 site which has an event content type with a date field. I want to require users to input an end date. The current stable version of D8 doesn't have an option to input an end date. Shall I create another date field for the end date as a temporary solution? If I use a second date field to collect the end date, how will this affect my site when a stable solution is published for collecting an end date in a single date field? Will there be a way to merge two separate date fields (start and end) into one?

I want to thank you guys for working hard on finding a solution for the end date in D8.

dkre’s picture

@Zythyr

I'm in a similar position with a project currently in development which requires this feature. Even though I have quite a bit of experience with D7 and some basic coding skills, it took quite a bit upskilling to get my project on track with the changes that came with D8. Not the coding but the development environment that is required to work effectively with D8 which is really what you need.

Git (or some sort of version control) is critical now with the new release schedule of minor/patch (8.minor.patch, 8.1.8). While learning to use git, branching, patching etc might be a hassle it has too many benefits to overlook. Using git you can branch (make copies) of your site prior to testing any new patches/updates while working with the beta releases of 8.2.x and patches. It makes the whole process easy and stress free.

This feature/issue has been promoted to 8.2.x beta (see #260) so this means that we know when this will be released which is fantastic, so the approach I've adopted is to work with core 8.2.x and look to go live after the 8.2.0 release date (Oct 8, from memory) to ensure I'm going live with stable releases.

You won't be able to merge the field data between Date Time field and this Date Time Range fields at a later date because they are distinctly different field types. You could migrate the data through an import/export process but this requires a bit of coding. Once the data has been merged you will need to then alter your views etc to work with the new field however.

Hope this helps.

xjm’s picture

Hmm, I am concerned that the repeated failed tests on this issue do not actually appear to be happening in HEAD in the same way at the moment. Compare:
https://www.drupal.org/pift-ci-job/422159
https://www.drupal.org/pift-ci-job/415739

I'm going to reupload the patch and retest it against the three databases. If it is somehow exacerbating an existing random fail that becomes a blocker. If all the test runs pass, we are probably okay.

xjm’s picture

Status: Needs work » Reviewed & tested by the community

02:22:38 The disk hosting /var/lib/drupalci/web/jenkins-default-196180/vendor is full, this may be the cause of the following exception

That's not likely to be happening because of this issue. Let's try rerunning the tests attached to the patch above once CI is stable.

mpdonadio’s picture

@xjm, I added PHP 5.6 and PHP 7 against MySQL to the list in case this gives more clues, but I will refrain from retesting. @Mixlogic found another test that had the same fail (PHPUnit_Framework_Exception: zend_mm_heap corrupted), and was investigating. Unfortunately, I don't have the IRC log saved from when we were talking about this.

This may shed some light on this, though: http://stackoverflow.com/questions/14597468/how-to-diagnose-these-php-co...

effulgentsia’s picture

I've been reviewing this patch, and overall, I really like it a lot. I'm not totally done reviewing it yet, but here's the only things I've found so far:

  1. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    @@ -83,3 +85,84 @@ field.widget.settings.datetime_datelist:
    +field.storage_settings.daterange:
    +  type: mapping
    +  label: 'Date range settings'
    +  mapping:
    +    datetime_type:
    +      type: string
    +      label: 'Date type'
    

    Here and similar, I think we should make the 'type' hierarchy match the class hierarchy. I did so in this patch.

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -129,6 +129,33 @@ public function settingsSummary() {
    +  /**
    +   * Returns the default cache context for the formatter.
    +   *
    +   * @return array
    +   *   A render array.
    +   */
    +  protected function defaultCacheContext() {
    +    return [
    +      '#cache' => [
    +        'contexts' => [
    +          'timezone',
    +        ],
    +      ],
    +    ];
    +  }
    

    I don't like this function. For one, its name is misleading, since what can be returned includes anything within '#cache', not just contexts, and for that matter, other top-level properties, not just #cache. If we really want to separate out cache information from the rest of buildDate(), then it would be more appropriate for the function to return a CacheableMetadata object instead and have the caller use the applyTo() method, or something like that, but that can be a followup. In the meantime, I just inlined this into the callers. Those callers already inline, and therefore amongst themselves duplicate, the call to ->setTimeZone(), and the addition of the cache context is required due to that, so having both lines of code near each other has its advantages.

Leaving at RTBC since these are minor changes.

effulgentsia’s picture

Issue summary: View changes
FileSize
67.83 KB

Also, I reviewed @xjm's points in #223 and confirmed they're all addressed except this one:

"Release window Start" is goofy capitalization for English. This makes me suspect either a string like @foo Start that should be @foo start for the English default, or that two strings might be being concatenated together in a less translation-friendly way. (I am not sure which because I haven't done a code review yet.)

The reason for this is that "Release window" is the label of the element representing the field item overall, and "Start" is the label of the element for the start date property. They just appear near each other due to I presume some CSS that inlines the two h4 elements. See screenshot:

I'm fine with punting any improvements of this to a followup.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
108.95 KB
2.07 KB

From #281's interdiff:

+++ b/core/modules/datetime/config/schema/datetime.schema.yml
@@ -117,37 +113,17 @@
-  mapping:
-    separator:
-      type: string
-      label: 'Separator'

Oops. That can't be removed. This patch fixes that, and cleans up the date range formatter defaultSettings() functions to only include what is different from the parent class, which makes it easier to compare the classes to the config schema.

That I messed up on that schema change signals that #281's interdiff, and this one, needs another pair of eyes on it, so setting to "Needs review" for that. I'm still reviewing the full patch as well.

effulgentsia’s picture

+++ b/core/modules/datetime/datetime.views.inc
@@ -11,6 +11,10 @@
+  if ($field_storage->getType() !== 'datetime') {
+    return [];
+  }

What's this code accomplishing? I skimmed through the issue comments about punting Views integration to a follow-up, but I'm not clear on what this code is actually solving. In HEAD, views_views_data() currently does this:

$result = (array) $module_handler->invoke($field_storage->getTypeProvider(), 'field_views_data', array($field_storage));
if (empty($result)) {
  $result = views_field_default_views_data($field_storage);
}

So, by making datetime_field_views_data() return empty for daterange fields, we're not actually removing those daterange fields from anything: not from fields, nor filters, nor sorts, etc. Seems to me all we're doing is forcing those filters/sorts/etc. to use string-based comparison instead of datetime-based ones. What am I missing?

mpdonadio’s picture

#284, if that hook runs for both datetime and daterange fields, the daterange fields get the plugins that datetime.module defines for argument/sort/filter, instead of the default ones provided by views.

The biggest problem is that the end value doesn't get picked up properly (see around #228) and the plugins behave oddly. Doing the field + the proper views plugins w/ proper testing is too big of an issue to tackle at once. Part of the refactor was to make this a little easier, but there is still a decent amount of work to make this work as expected. We can add the plugins with datetime_extras once we have something mostly stable, and then get into core for 8.3.0.

xjm’s picture

Can we add an inline comment for #284?

effulgentsia’s picture

I filed #2786577: The Views integration Datetime Range fields should extend the views integration for regular Datetime fields as a child issue and here's a code comment for it.

I'm still looking for a review/RTBC of my changes from #281 on :)

mpdonadio’s picture

Assigned: Unassigned » jhedstrom
FileSize
6.88 KB

Thanks for the followup; forgot about adding one explicitly to the issue. Nagging^H^H^H^H^H^H^Hpinging @jhedstrom for a review. He is most familiar with this w/o actually touching it. Attaching an interdiff against the last RTBC patch to make review easier.

effulgentsia’s picture

+++ b/core/modules/datetime/src/Tests/DateRangeFieldTest.php
@@ -0,0 +1,1227 @@
+class DateRangeFieldTest extends WebTestBase {

Very nice test coverage in this class. I opened #2786583: Create a base class for DateTimeFieldTest and DateRangeFieldTest and move common code into it as a follow-up issue, and here's trivial docs/whitespace fixes. Since this interdiff is trivial, #289 is still the interdiff that needs real review.

effulgentsia’s picture

I ran a coder check on the patch and it found a couple coding standards violations. Here's a fix for them.

effulgentsia’s picture

+++ b/core/modules/datetime/src/Plugin/Field/FieldType/DateRangeFieldItemList.php
@@ -0,0 +1,128 @@
+    // Allow either the start or end date to have a default, but not the other.

Is this comment correct? Looks to me from both reading the code and clicking around in the UI that both the start date and the end date are allowed to have a default.

Other than that, I'd feel great committing this once #289 is reviewed. Or if another committer beats me to it, that's fine too.

mpdonadio’s picture

What was in my brain, and what I ended up typing were not the same thing. There is coverage for this in DateRangeFieldTest::testDefaultValue(), towards the end of the method.

effulgentsia’s picture

pinging @jhedstrom for a review. He is most familiar with this w/o actually touching it.

A review from him would be awesome if he's available, but I don't think we should block this on him if he's not. We want to tag beta2 soon, so it would be really ideal for this to land today. To assist with either him or someone else reviewing the "253-293" interdiff in #294, here's some info about it:

  1. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeCustomFormatter.php
    @@ -66,7 +66,8 @@ protected function buildDate($date) {
    -    ] + $this->defaultCacheContext();
    +    ];
    +    $build['#cache']['contexts'][] = 'timezone';
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
    @@ -140,22 +140,6 @@ public function settingsSummary() {
    -  protected function defaultCacheContext() {
    

    See #281.2 for the rationale.

  2. +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -27,7 +26,6 @@ class DateRangeCustomFormatter extends DateTimeCustomFormatter {
    -      'date_format' => DATETIME_DATETIME_STORAGE_FORMAT,
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -28,7 +27,6 @@ class DateRangeDefaultFormatter extends DateTimeDefaultFormatter {
    -      'format_type' => 'medium',
    

    These duplicate what is in the parent method, so not needed. Removing the duplication helps maintain a parallel with the config schema definitions.

  3. diff --git a/core/modules/datetime/config/schema/datetime.schema.yml b/core/modules/datetime/config/schema/datetime.schema.yml
    

    This is what is most substantial in the interdiff. I'll post a longer comment shortly explaining it.

Other than the above, the interdiff just contains some docs and coding standards cleanup.

effulgentsia’s picture

I think the best way to review the datetime.schema.yml interdiff is to actually review its complete changes in the full patch. The key for me was, per #281.1, matching up the schema type hierarchy with the PHP class hierarchy. That's not a code-enforced requirement, but I think it makes things much less error-prone both in the present and in keeping up with future changes.

Here's a section-by-section explanation:

  1. +++ b/core/modules/datetime/config/schema/datetime.schema.yml
    +field.storage_settings.daterange:
    +  type: field.storage_settings.datetime
    +  label: 'Date range settings'
    +
    +field.field_settings.daterange:
    +  type: field.field_settings.datetime
    +  label: 'Date range settings'
    +
    

    In the patch, class DateRangeItem extends DateTimeItem and does not implement its own defaultStorageSettings() or defaultFieldSettings().

  2. +field.value.daterange:
    +  type: mapping
    +  label: 'Default value'
    +  mapping:
    +    default_date_type:
    +      type: string
    +      label: 'Default start date type'
    +    default_date:
    +      type: string
    +      label: 'Default start date value'
    +    default_end_date_type:
    +      type: string
    +      label: 'Default end date type'
    +    default_end_date:
    +      type: string
    +      label: 'Default end date value'
    

    In the patch, class DateRangeFieldItemList extends DateTimeFieldItemList and overrides the defaultValuesForm() method. The overridden method invokes the parent one, so I was tempted to make the above field.value.daterange set its type to field.value.datetime instead of to mapping. However, the two child properties that would be inherited by doing so, default_date_type and default_date, both have their label overridden, and I don't know if config schema lets you extend a child property without explicitly redefining the child property's type as well. I'm open to changing the above to be:

    +field.value.daterange:
    +  type: field.value.datetime
    +  label: 'Default value'
    +  mapping:
    +    default_date_type:
    +      label: 'Default start date type'
    +    default_date:
    +      label: 'Default start date value'
    +    default_end_date_type:
    +      type: string
    +      label: 'Default end date type'
    +    default_end_date:
    +      type: string
    +      label: 'Default end date value'
    

    to match what's happening in DateRangeFieldItemList::defaultValuesForm() if whoever reviews this thinks that would be an improvement and knows it (i.e., not specifying type on default_date_type and default_date in order for it to inherit that from the field.value.datetime definition) to be an allowed thing that's supported by config schema.

  3. +field.formatter.settings.daterange_default:
    +  type: field.formatter.settings.datetime_default
    +  label: 'Date range default display format settings'
    +  mapping:
    +    separator:
    +      type: string
    +      label: 'Separator'
    +
    +field.formatter.settings.daterange_plain:
    +  type: field.formatter.settings.datetime_plain
    +  label: 'Date range plain display format settings'
    +  mapping:
    +    separator:
    +      type: string
    +      label: 'Separator'
    +
    +field.formatter.settings.daterange_custom:
    +  type: field.formatter.settings.datetime_custom
    +  label: 'Date range custom display format settings'
    +  mapping:
    +    separator:
    +      type: string
    +      label: 'Separator'
    

    This corresponds to the following code for these formatters from the patch:

    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -0,0 +1,92 @@
    +class DateRangeCustomFormatter extends DateTimeCustomFormatter {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function defaultSettings() {
    +    return [
    +      'separator' => '-',
    +    ] + parent::defaultSettings();
    +  }
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -0,0 +1,100 @@
    +class DateRangeDefaultFormatter extends DateTimeDefaultFormatter {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function defaultSettings() {
    +    return [
    +      'separator' => '-',
    +    ] + parent::defaultSettings();
    +  }
    +++ b/core/modules/datetime/src/Plugin/Field/FieldFormatter/DateRangePlainFormatter.php
    @@ -0,0 +1,92 @@
    +class DateRangePlainFormatter extends DateTimePlainFormatter {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function defaultSettings() {
    +    return [
    +      'separator' => '-',
    +    ] + parent::defaultSettings();
    +  }
    
  4. +field.widget.settings.daterange_datelist:
    +  type: mapping
    +  label: 'Date range select list display format settings'
    +  mapping:
    +    increment:
    +      type: integer
    +      label: 'Time increments'
    +    date_order:
    +      type: string
    +      label: 'Date part order'
    +    time_type:
    +      type: string
    +      label: 'Time type'
    

    This corresponds to:

    +++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateRangeDatelistWidget.php
    @@ -0,0 +1,157 @@
    +class DateRangeDatelistWidget extends DateRangeWidgetBase {
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function defaultSettings() {
    +    return [
    +      'increment' => '15',
    +      'date_order' => 'YMD',
    +      'time_type' => '24',
    +    ] + parent::defaultSettings();
    +  }
    

    Note that the type is set to mapping, not field.widget.settings.datetime_datelist, because the class does not extend DateTimeDatelistWidget. As a result, the schema definition mostly duplicates the one for field.widget.settings.datetime_datelist, just as the defaultSettings() method above mostly duplicates the one in DateTimeDatelistWidget. There is no technical requirement for config schema type hierarchy to follow PHP class hierarchy, so we could reduce the duplication by setting the type to field.widget.settings.datetime_datelist, but I suspect that it's better for long-term maintainability to keep the config schema hierarchy parallel to the PHP class one.

  5. +field.widget.settings.daterange_default:
    +  type: mapping
    +  label: 'Date range default display format settings'
    

    And this corresponds to class DateRangeDefaultWidget extending DateRangeWidgetBase without implementing any defaultSettings().

I hope this helps whoever reviews this.

YesCT’s picture

Issue tags: +MWDS2016

I can also look at this right now.

effulgentsia’s picture

In terms of testing the config schema changes, rather than just relying on human review, here's some information:

  1. Core already has pretty decent automated test coverage testing config schema and ensuring all tested configuration conforms to its schema. Note that the patch in #281 had an error explained in #283, and as a result, it failed tests, which then passed in the fixed patch.
  2. Config schema elements that define themselves as translatable can be tested in the UI by trying to translate them. Unfortunately, the changes here are primarily for formatters and widgets, which do not have a currently functional translation UI due to #2546212: Entity view/form mode formatter/widget settings have no translation UI. Sad, but we shouldn't block this issue on that one.
  3. @Gábor Hojtsy reminded me of https://www.drupal.org/project/config_inspector, but I've never used it so don't know what to say about it. I asked him to comment here about it with information that could be useful.
jhedstrom’s picture

Status: Needs review » Reviewed & tested by the community

These updates look great. I also like that we've made the separator configurable.

I've filed #2787105: Add RTL support for daterange formatters which is a follow-up needed from #255.

I see a few other folks are actively reviewing this now, but I think it is RTBC if they concur :)

Gábor Hojtsy’s picture

@jhedstrom: uhm, well, @effulgentsia raised some concerns with the config schema, so unless that is responded to at least (if not adressed), I don't see how this is RTBC?

Gábor Hojtsy’s picture

Re the config inspector, you install the module and then admin/config/development/configuration/inspect lists all the config files in active storage and also validates them for this screen. So any errors are instantly called out. Then the List inspection mode is a good way to review the concrete errors (if any). I installed the patch with config inspector and set up a sample date range field with default settings. That did not result in any schema errors in the field and storage settings or the display or form modes either. (Not an exhaustive test by any measure, but a spot check at least).

jhedstrom’s picture

Status: Reviewed & tested by the community » Needs review

re #300

I guess I didn't see further suggestions for changing it though? Setting back to NR so more folks can further review the specific interdiff in #294 for those schema changes. I followed through the detailed reasoning and explanations in #296 and the current patch's schema made sense to me following those. If folks using the config inspector do find errors (I haven't had a chance myself), those should probably be addressed here if severe enough (if not, follow up issues seem sufficient).

jhedstrom’s picture

I think the remaining question is #296.2? I don't know the answer to that myself.

#296.4's suggestion of maintaining the class hierarchy makes sense, unless somebody thinks it won't be more maintainable long term.

YesCT’s picture

I also installed the config inspector, made a date field and a date range field, I dont see any errors, and looking at the config inspector form tab things look good. Still looking more.

@effulgentsia thanks for pointing out #254621 in #298 that not translating all the config is not the fault of this patch.

I (also) dont know the answer to 296.2

YesCT’s picture

FileSize
38.45 KB

Translation of date range config, and date range content worked.

It would be nice to add "context" to the start and end strings so that someone translating those interface strings would know they have to deal with dates.

I tested multivalue date range (and compared it to multivalue date) and there didn't seem to be any data problems.

I made content. Here is a screenshot.

Note Birth day is a date field, and Party is a date range field.
Visually, in the content creations, the labels on start and end are a bit concerning, especially that there doesn't seem to be an association with the field name for the "end".

So I'm done testing for now unless someone pings me.

xjm’s picture

Version: 8.2.x-dev » 8.3.x-dev
Issue tags: -beta target

Thanks @effulgentsia, @jhedstrom, @mpdonadio, @Gábor Hojtsy, and @YesCT.

I am really glad to hear that the schema is in order and that translation works correctly. That is a relief!

Unfortunately, the additional presentational/usability issues in #305 combined with the fact that we are well into the beta phase (And already have postponed beta2 more than two days) still make me uncomfortable committing this to 8.2.x. I am going to go ahead and move this to 8.3.x. I do think it's really close.

The current structure also compounds the accessibility issue from #1918994: Improve Datetime and Daterange Widget accessibility. In addition to the h4 for the field label being separate from the semantic label, the "Start" and "End" are separate from the field label. (Fixing #1918994: Improve Datetime and Daterange Widget accessibility will provide us a pattern that can help fix this as well, but is not a hard blocker.)

xjm credited miwayha.

xjm’s picture

(Adding credit for @miwayha who helped with the accessibility testing here at MWDS.)

xjm’s picture

(Also some reviewer credit; not complete though.)

mpdonadio’s picture

Title: Add a Date Range field type with support for end date » [PP-1] Add a Date Range field type with support for end date
Assigned: jhedstrom » Unassigned
Status: Needs review » Postponed

OK, this needs works for the field widget problem (ie, the way DateRangeDefaultWidget renders out).

However, I am postponing this on #1918994: Improve Datetime and Daterange Widget accessibility. Since we refactored DateRangeDefaultWidget -> DateRangeWidgetBase -> DateTimeWidgetBase, the two issues are now tightly coupled with the way ::formElement() gets built up, so I see it as a hard blocker. We need to see how that shakes out to see what the impact will be on this widget (which may require us to rethink how we refactored DateRangeDefaultWidget and DateRangeWidgetBase).

swentel’s picture

Really ... #1918994: Improve Datetime and Daterange Widget accessibility has been open for 3 years ... I don't think it's fair to use that issue to postpone committing this patch on that, given all the work that has been done here. To be clear, I'm not saying that I don't care about presentational/usability, but these exist in the D7 version of date as well, and it's working out fine for many many people.

I strongly suggest won't fixing this and moving it to https://www.drupal.org/project/datetime_extras and come back in a year (or maybe never) while the community can move on laying foundations for good date support in various ways: range, timezone, multi day event views, calendar (really, calendars, multi day events are a pain without this field type (or a self maintained variant)). Committing this to 8.3.x, even early would basically just kill any momentum that people might have gained by getting things forward for better date/time/calendar support in D8 core. The problem space is just to hard and I feel this is a perfect example of something that should and could get a new and better momentum by keeping it it in contrib.

rdworianyn’s picture

I agree with swentel, this should not be blocking, especially since it has been working like this in 7. This is a huge setback for many people if it gets pushed to 8.3...

mpdonadio’s picture

We can unpostpone this particular issue and work independently of #1918994: Improve Datetime and Daterange Widget accessibility, but it would still have to goto Needs Work, b/c #306, which also removed beta target and moved it to 8.3.x.

mpdonadio’s picture

Issue summary: View changes
gambry’s picture

That's a shame.
I've been waiting for this feature for months. Planning projects releases according with the targets in here... and now for a label issue, fixable in a follow up in probably a couple of days and as a bug being committable at any point, I see the great work done so far been postponed to the 5th of April 2017 AKA one year.

I understand the decision but personaly I agree with #311.

heddn’s picture

I've been doing bad things for my D8 sites since December of 2015; like telling them they cannot have events that have an end date. Then justifying it (to myself) by knowing this was coming. But now that this is punted out by another 9 months, I can only hope that datetime_extras get's this functionality sooner, rather than later.

Any chance on revisiting the decision and still including this in 8.2? Pretty please? My conscience will be feel much better. I don't think I'm the only one with a vested interest. With 125+ followers, there's a few others that want to see this brought into core, or somewhere.

b0b’s picture

It's kind of ironic that the date and calendar functionality was what brought me to Drupal way back in version 4.6 and then took my career down the Drupal path. Now it's the only module keeping me from moving that very same site to D8. This is very important functionality.

dalin’s picture

I plan on just applying this as a patch for future D8 builds. The remaining work seems confined enough that running it as a patch isn't likely to be a problem.

Or are other people seeing something that I'm missing???

mpdonadio’s picture

Title: [PP-1] Add a Date Range field type with support for end date » Add a Date Range field type with support for end date
Status: Postponed » Needs review
FileSize
109.29 KB
925 bytes
44.19 KB

Ok, let me preface this by saying that front-end work in Drupal 8 is not my strongest skill. Nonetheless, I am pushing forward on this whether it is for 8.2.x or 8.3.x.

The biggest problem is datetime_wrapper appears to be used for each of the input sets, and then again for the pair (turn on twig debug and take a look, it gets output three times). So, three h4 elements get rendered, and two of them happen to be near each other because they are inlined. I think the fix may just be to introduce a daterange_wrapper and make sure it renders well. The odd thing about this solution is that datetime_wrapper is part of core, and not the module because it is part of the datetime render element and not the datetime field. Maybe just a fieldset?

The problem that I see is that a datetime field actually gets wrapped twice, once by the widget and again by the datetime render element. It isn't evident, though, because the inner one doesn't have a title, so there is only one h4 element. This bug is carried over to the daterange widget b/c the common code.

A solution for #1918994: Improve Datetime and Daterange Widget accessibility is going to complicate matters here, probably for the worse. Per #306 this is still Needs Work, though.

Attached is a start of a fix based on a fieldset. TBH, though, the proper thing to do is fix it in the DateTime widget first...

xjm’s picture

Thanks @heddn, @gambry, @swentel, @b0b, and @ryantollefson for the feedback.

This issue was given 18 days of special consideration beyond the documented deadlines for the minor release, as well as an extensive time investment from multiple core committers. This was precisely because of the value of this feature and how long contrib authors and site owners have been anticipating it. Only two other issues ultimately received the same consideration, and both of those issues introduce optional experimental features (as compared to this, which is being added to an essential stable core module already installed on most sites).

When I made the case for this to be a beta target issue (when @mpdonadio reached out to me shortly before the announced deadline three weeks ago), other core committers rightfully pushed back and said the feature could live in contrib until 8.3.0. I advocated to the other committers to avoid that because of the need for an upgrade path, but it is still an option.

If you need this functionality, help the datetime maintainers resolve the outstanding issues here, and consider a contrib module that will provide an upgrade path to 8.3.x core. The best way to advocate for a feature is to test it, document the results of the testing, and assist the patch authors with resolving issues large and small that are uncovered.

I do believe this important and basic functionality does belong in Drupal core, once it is ready.

mpdonadio’s picture

Was pretty much expecting that run to fail, but it also shows that we are missing similar coverage in the DateRangeFieldTest::testDatetimeRangeField() method (and also DateTimeFieldTest::testDatetimeRangeField)

Would appreciate some front-end expertise to figure out how we can wrap up this for 8.3.x ASAP, potentially taking into consideration that we may need a side issue to resolve the double datetime_wrapper bug in the datetime widget(s) first.

Also, if someone wants to add one of these patches to datetime_extras asa sub-module and do the update path / migration to the core field, I can grant maintainer access.

@xjm, would you entertain committing the patch in #294 to 8.3.x, front-end warts and all, so we can continue the bugfixes in followups, and also move forward on rerolling the in-progress patches to take into account the new field (like the ones with the timezone support and fixes) and move forward on the views plugins?

xjm’s picture

Another option to consider at this point (in addition to those suggested by swentel and mine above) is whether putting this field in a separate Core 8.3.x experimental module would be beneficial. swentel pointed out the risks to innovation for this functionality being added to core before the hard problems are fully solved. If we add it as an experimental module, however, we can commit it to 8.3.x even with outstanding accessibility bugs, as well as make improvements to it iteratively not only before but also after the April 5 release of 8.3.0. It would require some contained changes to the current patch.

Experimental modules can receive significant changes even in patch releases, providing something closer to the contrib rate of innovation but with the added assurance of a much wider testing audience, core review and QA, and core test coverage to reduce the risk of regressions due to other core changes.

xjm’s picture

I crossposted with #322. @mpdonadio, this would be a completely viable 8.3.x experimental module as-is. Then the followup issues become part of the experimental module roadmap, which is sort of a contract between maintainers and the community to make the module stable within two minor releases. That also helps with some of the concerns about release predictability raised above. It would take some changes to the patch (to move the code to a separate module) as well as a bit of organizational work to make the plan issue for an experimental module's roadmap. What do you think?

gambry’s picture

Between the two options - having this work (+ bugfixes and improvements) in core 8.3.x as-it-is OR adding it to a separated experimental in core or contrib module - I would choose the first one only because that means I can potentially start using the patch from tomorrow untill 8.3 release.

As separated module we'll need to re-re-factor @mpdonadio merge work, which means more work now and resulting in two separated points of failure for datetime related bugs.

Said that I'm happy with whatever the community decides.

Thanks @xjm for the time spent on explaing the whole decision process.

DuneBL’s picture

I am completely lost...
The only question (on my side) is: Can I use this component in a production environment (I agree to patch it on a regular basis) or should I avoid this component and start using using 2 regular date field to mimic the start/end date behavior?
Is there anybody with enough knowledge to answer this simple question... I am sorry if the answer is obvious, but I can not answer it myself after reading all the posts...

jhedstrom’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Needs work » Needs review

I've been thinking this over since yesterday evening when it was announced this would not make 8.2. I'd like to make the case that it should still be eligible.

As others have noted, this is imperative because 8.3 is 9 months out, 1.5 years after 8.0.0 was released. When the date module was put into core, that sent out a signal to the world of Drupal users that core would support the functionality that had previously resided in contrib. Indeed, any 8.x efforts on the Date module halted around that time (October 20012 to be exact).

Here's a brief history of the patch since it was first marked RTBC:

  • First patch RTBC'd July 28, well before initial beta cutoff. That same day, we got to an RTBC patch (#216).
  • There was then a flurry of activity, resulting in another RTBC patch (#253) on August 5.
  • This sat for nearly 2 weeks at RTBC (minus random test failures), and I think everybody assumed it was good-to-go.
  • After those 2 weeks, the additional reviews pointed out some problems, but it was repeatedly stated that follow-up issues would be sufficient.

My proposal for inclusion in 8.2:

  • Commit the patch from #253.
  • File major bugs for everything past that which was discovered. The really great thing about bugs is they can be fixed at any time. We'll try to tackle as many as possible before 8.2.0, and the ones we miss can be done in minor point releases.

Other reasoning:

  • I really do not want to fork core datetime efforts into contrib for such a fundamental feature. It will be more work, and distract from improvements here.
  • This is a new field. We don't have to worry about breaking anybody's existing site (not that I think this would break anything).
  • This issue is a major contrib blocker.
  • People are already running patches, let's avoid the uncertainty of unsupported update paths. The maintainers are comfortable standing by this data model, and writing any database or config/schema updates if they should arise in the future.
  • In order for other major features in datetime to move forward, this would be really great to have committed ASAP.
  • With regards to an experimental module, in retrospect the datetime module probably should have been marked experimental for 8.0.0. It won't be seen any other way by folks trying to use it until it is much closer in feature-parity to the Drupal 7 Date module. See #2543958: [META] DateTime Module Improvements.

I'm leaving this at needs review, but if others think this is a way forward, please mark RTBC. I am bumping it back to 8.2 for now.

colan’s picture

Status: Needs review » Reviewed & tested by the community

Agreed. Other options would create more work needlessly, and yes, it's a new field that won't interfere with anything.

yoroy’s picture

Issue tags: +Usability

Without this tag issues won't get on the ux team radar :-(

webchick’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Needs work

@xjm and I discussed this at length at MWDS2016. I can see the difficulty with the fact that this issue sat at RTBC for a couple of weeks during beta1 without review (during which time there were people ready and willing to work on it), and when those RTBC reviews were provided a couple of days before beta2, they found problems for which there was then not sufficient time to fix (despite an additional ~72 hours being granted on the beta2 deadline).

I think we're in a tough spot because if this goes in as-is, it's a patch against a stable core feature, and that means we need to resolve the accessibility and cosmetic issues ahead of time. If we refactor it into an experimental module, this gives us the breathing room we need to make those issues follow-ups, because they can be further improved during patch releases. But beta2 has already shipped at this point. Additionally, it seems the UX team hasn't been looped in here at all yet, per #329.

I think at this juncture, the most logical next step is to use #294 to create an experimental module in 8.3.x (not #253, which doesn't have the required refactoring). Then it's in core, at least. Part of this is a roadmap that includes the issues unaddressed by this patch, such as the accessibility and any usability issues. That way, those things can be handled as follow-ups as @jhedstrom suggests.

And then, we can discuss what's best solution for 8.2 site builders that doesn't create a horrific upgrade path situation.

sukanya.ramakrishnan’s picture

This is really sad news. People who have been following this issue know how hard folks have been working for getting this feature in 8.2.

In my opinion, if this cant get into 8.2, i would be comfortable applying this as a patch for 8.1 as well as another patch for 8.2 if need be , instead of burdening the maintainers to create a contrib module or follow another route. That would be too much maintenance work for the folks.

regards,
Sukanya

swentel’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Needs work » Needs review
FileSize
103.59 KB

So yes, this is kind of sad. I anticipated a bit that it would be pushed back, so I took a beer and chips, and went crazy on the code .. and tadaaaa .. a patch with an experimental module.

If you look at the code, it's really really simple. I had to add a trait for the buildDate() function that was introduced, which was the only part that was a bit hard to move out. I also added a hook_help, which is almost 90% the same of the datetime module.

Now. This is a really honest opinion: I think (I didn't want to say "I am certain" because I don't have scientific proof) it would be a bad idea to commit this on 8.3.x only. As an experimental in 8.2.x this would make a ton of people happy, and immediately allow contrib (especially calendar) to regain momentum to get multidays events working.

I know it's a extremely long shot, but one can always try, but imagine a release for 8.2.0 including end dates ? :)

mpdonadio’s picture

If an experimental module means this getting into 8.2.x, then I am for it. It would means that we have two datetime modules in the long run, but I can live with that. However, we already have a contrib module called "datetime_extras" for stuff we want for Drupal 8 but don't necessarily want in core. So, there is a potential namespace problem there. It may also complicate some of the planned views and timezone work, as well as some in progress patches (haven't thought about the last part fully).

However, if 8.2.x doesn't work out, then I don't see the point of adding this to 8.3.x as an experimental module (essentially at the beginning of the 8.3.x cycle). In that case, I am strongly suggesting that we commit #294 to 8.3.x, so we can free up that planned work, reroll some patches that fix some other bugs, and move on. One, a framework manager was already OK with that patch having followups, and two, the root cause of the pushback in #306 is due an existing bug (or bugs) in DateTime DefaultWidget and/or the Datetime element.

gambry’s picture

I totally agree with #334, and the sooner this go to 8.3.x the closer the codebase will be with a potential 8.2.0 + patch #294, as 8.3.x branch has been open recently and shouldn't be *a lot* different than current 8.2.x.

Then the way I see it is: site builders can use the 8.2 stable release + adding #294 patch, we promise schema and config won't change but only UI and small optimisations and when 8.3 is out there won't be any pain for them, but worse scenario just an upgrade task.

swentel’s picture

Status: Needs work » Needs review
FileSize
103.5 KB

Good point about the namespace, renamed to datetime_range. hook_help failure should be ok now too.

swentel’s picture

Status: Needs work » Needs review
FileSize
103.5 KB

Ugh

swentel’s picture

FileSize
15.66 KB

Summary of changes compared to the patch - was a bit hard to create an interdiff which would be very noisy. Important also, nothing changes any more in the original datetime.module, in general, it simply moved the all new files there to this new module. I did attach an interdiff after my local rename from datetime_extras to datetime_range. The only real new piece in the patch then is the DateTimeRangeTrait class. Hope that helps.

- Added experimental module called 'datetime_range': provides a field type, widget and formatters for storing start and end dates
- Added hook_help: 90% equal to datetime
- Added entry in core/composer.json
- Added entry in Maintainers.txt
- Added DateTimeRangeTrait which contains two methods for building a render array for dates. Originally there was only one method called 'buildDate' in the original patch which was implemented by the formatters. This cleaned up some code and made them a bit easier to read. To get this patch forward fast I opted for a trait because otherwise I had to duplicate the method in all formatters.

borisson_’s picture

Status: Needs review » Needs work

Mostly nitpicks, overall this is look great.

  1. +++ b/core/modules/datetime_range/src/DateTimeRangeTrait.php
    @@ -0,0 +1,67 @@
    + * Provides friendly methods for datetime range.
    

    I love that these methods are friendly :)

  2. +++ b/core/modules/datetime_range/src/DateTimeRangeTrait.php
    @@ -0,0 +1,67 @@
    +    $build = ['#plain_text' => $this->formatDate($date)];
    +    $build['#cache']['contexts'][] = 'timezone';
    +
    +    return $build;
    

    We can just return a new array here instead?

  3. +++ b/core/modules/datetime_range/src/DateTimeRangeTrait.php
    @@ -0,0 +1,67 @@
    +   * Creates a render array from a date object with iso date attribute.
    

    /s/iso/ISO/

  4. +++ b/core/modules/datetime_range/src/DateTimeRangeTrait.php
    @@ -0,0 +1,67 @@
    +    ];
    +    $build['#cache']['contexts'][] = 'timezone';
    

    I don't see the need to add this as a seperate line, we can do this while creating the array.

  5. +++ b/core/modules/datetime_range/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -0,0 +1,96 @@
    + * This formatter renders the data range as plain text, with fully
    + * configurable a date format using the PHP date syntax and separator.
    

    /s/with fully configurable a date/with a fully configurable date/

  6. +++ b/core/modules/datetime_range/src/Plugin/Field/FieldFormatter/DateRangeCustomFormatter.php
    @@ -0,0 +1,96 @@
    +            'separator' => ['#plain_text' => ' ' . $separator . ' '],
    
    +++ b/core/modules/datetime_range/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -0,0 +1,104 @@
    +            'separator' => ['#plain_text' => ' ' . $separator . ' '],
    

    This hardcodes the spaces surrounding the separator, should we move those into the seperator variable instead?

  7. +++ b/core/modules/datetime_range/src/Plugin/Field/FieldType/DateRangeFieldItemList.php
    @@ -0,0 +1,130 @@
    +    // Explicitly call the base class so tht we can get the default value types.
    

    /s/tht/that/, this also means that types will end up on the next line.

  8. +++ b/core/modules/datetime_range/src/Plugin/Field/FieldType/DateRangeItem.php
    @@ -0,0 +1,132 @@
    +    $start = REQUEST_TIME - mt_rand(0, 86400 * 365) - 86400;
    +    $end = $start + 86400;
    

    I think this can lead to a negative value for $start, the min value should be 237 instead of 0?

  9. +++ b/core/modules/datetime_range/src/Tests/DateRangeFieldTest.php
    @@ -0,0 +1,1227 @@
    +class DateRangeFieldTest extends WebTestBase {
    

    This adds a new Simpletest, since we're phasing those out, should we change this test to BTB?

  10. +++ b/core/modules/datetime_range/src/Tests/DateRangeFieldTest.php
    @@ -0,0 +1,1227 @@
    +  public static $modules = ['node', 'entity_test', 'datetime', 'datetime_range', 'field_ui'];
    

    Needs reformatting to be within 80 cols.

  11. +++ b/core/modules/datetime_range/src/Tests/DateRangeFieldTest.php
    @@ -0,0 +1,1227 @@
    +  /**
    +   * The default display settings to use for the formatters.
    +   */
    +  protected $defaultSettings;
    

    I think this needs an @var array?

mpdonadio’s picture

Quick comments on #340.

#6 was functionality that has test coverage from the RTBC patch. I am very hesitant to introduce changes like this at this point. I also wonder about the UX of having to enter the spaces. If someone doesn't need the space, then they can do their own alter.

#8 REQUEST_TIME is current north of 1.4 billion, so I don't think there is any danger here. It is also just fake, sample data.

#9 See #2780063: Convert web tests to browser tests for datetime and datetime_range modules. BTB is missing some key methods right now for a quick conversion (but, I want to do this as soon as we can).

#10 We frequently let this slip. Search for `public static $modules` in core and see all of the examples, even with new tests.

borisson_’s picture

#341
.6: Yeah - I wasn't sure about that either. The alter is fine.
.8: Oh, yeah, I didn't consider that. Sorry.
.9: Sure.
.10: There's a phpcs rule for that and I'm assuming that will get in at one point. This will be changed for those tests as well in the future. Let's not knowingly introduce things that break the code style rules, even if core doesn't do it right in other places.

YesCT’s picture

@borisson_ Thanks for taking the time to look over this code. Questions are great feedback too.

I can make a new patch and interdiff to address some of those points, now.

Please keep in mind that the requirements for an experimental module are different than for going into core directly, and coding standards (that dont have automatic checks) are not required. https://www.drupal.org/core/experimental#requirements

Personally, I often notice things like that too, and will mention them, but making the changes is nice to do at the same time for nits.

alexpott’s picture

Version: 8.2.x-dev » 8.3.x-dev

Putting this back to 8.3.x-dev because both @xjm and @webchick decided in #306 and #330 to do it and as the release manager and product manager for Drupal 8 it's their call. It's also their call as to whether going the experimental module route makes this eligible for 8.2.x-dev again since adding will necessitate another beta release and more work. I know this issue is on their radar, they've spent part of their weekend looking at the issue, so I'm sure that they will be make a decision if the experimental module can be part of 8.2.0 soon.

xjm’s picture

Agreed, thanks @alexpott. We can discuss what to do for 8.2.x once this is committed to 8.3.x, as @webchick said.

Many thanks to @swentel for getting the experimental module direction moving. I think this is a direction that will help not only for this feature but also for other core datetime work (to @jhedstrom's points about the outstanding overall roadmap for datetime).

I can help with the plan issue for the experimental module tomorrow. We can include parts of @borrison_'s review as issues on it, as well as @YesCT's feedback about string context, the accessibility issues, etc.

Both @effulgentsia and I think the code overall is in great shape.

YesCT’s picture

344a addresses #340 3. 5. 7. 11.

This leaves questions in 2. and 4. to be answered. I might try and do that later.

I found some other nits as I was reading.

a. module name
could not find the standard for module naming (looked in 1354 and https://www.drupal.org/coding-standards#naming)
but grepping in core
ag --file-search-regex "info.yml" "name: " | grep -v test | grep "name: [A-Za-z]\+ "
All the multiword (non test) modules upper case each word in the name.
Changes only in info file, and .module

b. in the move from datetime extras to datetime range, the schema file name (and first line comment) didn't get renamed.

c. And other nits, see the interdiff a-b.

I tried not to do any changes that might break tests.

I'm gonna try some other changes to do with type hinting, next. and maybe take a look at 2. 4.

mpdonadio’s picture

Before we do any real heavy lifting on this, can we pause for a moment?

The current plan is to add this as an experimental to 8.3.x and then make a decision about whether we can get it into 8.2.x? And it may not make it into 8.2.x anyway? And this is because of a bug that is in 8.2.x that just happens to be more evident in this patch, and we are currently spinning in circles on that issue? That issue also skirts the true problem, which I am going to post an issue about tonight.

If that is the plan, then I am not for it.

I would like one of two things to happen.

- The experimental gets into 8.2.x first
- or we commit 294 to 8.3.x, start fixing things as followups, and move on with everything in one module. People can use the patch against 8.2.x if they want. I don't see the data model changing at this point.

I thought the idea of an experimental module was so that we could get this into 8.2.x, so people could use it while we iron out the widget problem. I don't see what an experimental in 8.3.x buys us in the long run, especially since we are at the beginning of that development cycle. Long term, we either end up with two date-related modules, or having to do a nasty update path to get everything back into one module.

YesCT’s picture

@borisson_ I looked at #340 2. and 4.

And it looks like there are a bunch of examples in core where $build is made, and then altered with context in a separate statement after... maybe cause it takes less lines of code? But there are also examples where it is set all at once.

I did make it all in one assignment statement, (and got help in irc), and checked first that the array structure is exactly the same by doing a var_dump on both ways.

I also grep'd in core to see about making a $build and then (with no code in between) returning it. And this seems to be *very* common, and I think makes it easier to read.

So not returning just an array.

This is the end of changes from me for now, and addresses all of #340,

Next I'm gonna make a plan issue and work on documenting the experimental requirements as remaining tasks.

---
note 1 (just to document the info, no change), tim mentioned that usually methods like

+  public function validateStartEnd(array &$element, FormStateInterface $form_state, array &$complete_form) {

are static ... like

core/lib/Drupal/Core/Render/Element/FormElement.php
108:  public static function processPattern(&$element, FormStateInterface $form_state, &$complete_form) {

---
note 2 (in case it is useful for future reviewers)
in the trait, phpstorm says $this->getFieldSetting() setTimeZone() formatDate() method not found
This is because DrupalDateTime extends DateTimePlus and DateTimePlus passes method not known to it with function __call()

YesCT’s picture

Created #2788253: Plan for DateTime Range experimental module
[edit: doh, copy and pasted the id of this issue. fixed]

dkre’s picture

I really need to concur with mpdonadio #347. We really need a final decision made on this.

While all of the decisions have made complete sense and there has been an incredible the amount of work put in this is a really painful issue for any site builder requiring this functionality.

I don't understand how D8 shipped with 'date in core' without clear documentation, or a huge disclaimer stating the limited implementation.

ndf’s picture

@YesCT: great spirit, you share an opportunity to validate the experimental module!

@mpdonadio: +1 #347
Is an upgrade path impossible at this point? Is it only the widget issue?

@alexpott: +1 #344
The current patch is rejected for 8.x-2.x
Is should be in in 8.x-3.x
One alternative is an experimental module in 8.x-2.x
Or in 8.x-3.x

gambry’s picture

+1 for every single word in #347.
Please let's have a decision first and coding later if needed.

xjm’s picture

Thanks everyone for your ongoing interest in this issue.

For everyone asking for a decision, the decision was already made. To reiterate: This will not be considered for backport to 8.2.x unless it is an experimental module, and it will not be considered for backport until it is committed to 8.3.x. Three separate core committers have commented to this effect.

It's not helpful to continue commenting about how the issue is painful for site builders or how it's a thorny contrib blocker. We are very aware. The past 100 comments on this issue exist because we are very aware. Please limit your comments on this issue to reviewing and testing the current patch (aside from the subsystem maintainers who do need to give feedback about what works for them).

I've already signed off on the current patch being committable as an experimental module once it is reviewed, etc. (no more heavy lifting needed; swentel already did that). It just needs to be reviewed and the datetime maintainers needs to sign off on the approach. Then it's ready to go and people can plan for it.

It is not currently committable as a non-experimental module and it will take an unknown amount of time to get it to get it to a point that it is. That's bad for contrib, bad for core, bad for site builders.

xjm’s picture

Ah. I think this got missed somewhere which might have led to the confusion: At MWDS @webchick and I discussed whether it was to have both datetime and datetime_range as separate modules for the duration of 8.x, and we agreed that it was unless the datetime maintainers disagree. So @swentel's refactor would be the basis for a stable core module, and no additional upgrade path or anything would be required to support contrib or sites. @mpdonadio, does that make sense?

I commented to this effect on #2788253: Plan for DateTime Range experimental module.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

If getting this into core the fastest means starting out in 8.3.x as an experimental, and there is a glimmer of hope of getting it into 8.2.x this way, then let's do it. If we run into problems down the road, we'll deal with them then.

Do you agree @jhedstrom ?

Going to spend an hour with this patch tonight to see if there are any problems. In the meantime, someone else needs to review this. A lot of hands have been in this, so the number of people who can RTBC it are dwindling. I think we may want a

function daterange_field_views_data(FieldStorageConfigInterface $field_storage) {
  return [];
}

so at least there is basic Views functionality for the field with the standard plugins from the get go (tbh, though, I forget if these will work without the empty hook, hence some manual testing).

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Issue tags: +Needs release manager review
FileSize
10.01 KB

Ok, verified we don't need a hook_field_views_data(); basic Views functionality will be there with the default plugins.

The problem I see with the patch in #348 is that a fair amount of the refactoring from #293 is gone.

#293 touched some of the DateTime classes to make the refactoring possible (eg, the buildDate method, createDefaultValue, and some more), but didn't touch any existing tests. So, we have duplicate code again between DateTime and DateRange, which was one of the reasons this got push back initially (the new trait partially addresses this w/in DateRange itself). Attached are the missing hunks, as far as I can tell.

Not sure what we want or can do about this with an experimental module. Can we touch the DateTime classes, or does that need to be another followup (or a precursor to this getting in)? Or since part of the experimental plan is "verify removing the module without can be done without lasting disruption" can we not do this?

@swentel, which patch did you start with?

@xjm / @webchick / @alexpott, what is possible or not possible with the changes to the DateTime classes if this is going in as an experimental?

xjm’s picture

@xjm / @webchick / @alexpott, what is possible or not possible with the changes to the DateTime classes if this is going in as an experimental?

I discussed this a bit with @mpdonadio in IRC and also posted on #2788253: Plan for DateTime Range experimental module. I think the refactoring in #356 can go into stable datetime for 8.3.x after this lands; it's part of the roadmap to make the field stable, but enough of the code duplication is resolved still in this issue that it should be fine for the initial experimental module commit with those outstanding.

mpdonadio’s picture

I read the diff between DateRangeFieldTest in #293 and #348. The changes are trivial. That makes me confident that we have a good patch to commit. Nothing turned up in my manual testing, either.

#348 has my blessing.

swentel’s picture

@mpdonadio I started from #294 - it was basically applying the patch, moving all the new files to the experimental module and then resetting the changes in datetime module itself. The trait is the only real new thing (and the info and module file too of course)

Thanks @all for considering and getting this forward :)

effulgentsia’s picture

#348 looks great to me as well. A couple small points that could potentially be follow-ups, but maybe are straightforward enough to just reroll for?

  1. +++ b/core/modules/datetime_range/src/DateTimeRangeTrait.php
    @@ -0,0 +1,78 @@
    +trait DateTimeRangeTrait {
    

    This trait is specific to formatters, so perhaps rename to DateTimeRangeFormatterTrait? And maybe also move into the Drupal\datetime_range\Plugin\Field\FieldFormatter namespace?

  2. +++ b/core/modules/datetime_range/src/Plugin/Field/FieldFormatter/DateRangeDefaultFormatter.php
    @@ -0,0 +1,104 @@
    +        if ($start_date->format('U') !== $end_date->format('U')) {
    +          $elements[$delta] = [
    +            'start_date' => $this->buildDateWithIsoAttribute($start_date),
    +            'separator' => ['#plain_text' => ' ' . $separator . ' '],
    +            'end_date' => $this->buildDateWithIsoAttribute($end_date),
    +          ];
    +        }
    +        else {
    +          $elements[$delta] = $this->buildDate($start_date);
    +        }
    

    That last line should be buildDateWithIsoAttribute, not buildDate. Which points to a couple things. 1) We probably need a test added for this. 2) What's the point of a buildDateWithIsoAttribute() method on the trait? Why not just have DateRangeDefaultFormatter override the trait's buildDate() method?

  3. Finally, if we're rerolling this patch anyway with any changes to the trait for the above, should we also move defaultSettings(), settingsForm(), and settingsSummary() to the trait, since those are identical between the 3 formatters? Even viewElements() could almost be moved there, so long as DateRangeDefaultFormatter could extend the trait's method and additionally handle the $item->_attributes logic.
swentel’s picture

Assigned: Unassigned » swentel

1. Makes sense
2. Hmm, I copy pasted that from the original patch. I guess @mpdonadio knows more about the different between those two. There are tests that effectively test that Iso attribute, but I have to be honest, I'm a not 100% sure what goes on here :)
3. Makes sense too.

I'm starting to wonder whether we shouldn't create a baseFormatter here with all those methods in, then we can even drop the trait ?

Assigning to myself, should be able to roll quickly.

mpdonadio’s picture

Assigned: swentel » Unassigned

[I think I lost a post]

#360-1,3: When talking to @xjm last night, we decided to make a followup to redo some of the refactoring that fell out of this. Part of this was some rework to the DateTime classes to support better reuse in the DateRange classes. I would be hesitant to improve the DateRange classes/traits until we hash this out, as the end goal is to get closer to what #293 looks like, which would undo the suggestions above.

#360-2, yup. Looked at patch and we are missing formatter coverage when start==end. Per ^^, think buildDate() is a better name there.

#361-2, The default formatter outputs a HTML5 time element, which has an attribute with an ISO date; for DateRange is is <time> - <time> for the pair of them. The other two formatters just output text.

swentel’s picture

Ok, here's a patch with everything besides viewElements moved out. Ran out of time, so couldn't do evertyhing.

xjm’s picture

For me #360.1 and .3 as well as #361 would also be worth exploring as followup issues if needed.

gambry’s picture

I've done some manual tests and patch looks good to me.

I just want to flag this field is affected by #2778083: Default value for Date-Only fields is broken when UTC date is different than user current date too.
Not sure if we want this to fix it with the DateTime follow-ups mentioned at #362, with a proper DateRange follow-up or now before this patched is committed.

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

#365, a lot of the inprogress patches are going to affect this. Half tempted to postpone all of our in-progress work for a few days until this is in.

Right now, I am going to take #348 and fix #360-2 before I begin start paying work. Then I suggest

- commit new patch 8.3.x
- address the refactoring issues to get this closer to (which would possible make #360-1,3 and #361 not needed), and get this closer to #294
- touchup our bugfixes to account for this (though, if we do the refactoring properly and we are closer to #294, this may just be test coverage)
- move on with other work

On a side track, the committers can decide the fate of new patch for 8.2.x.

Once I post new patch, I still want @jhedstrom's blessing on everything.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
FileSize
114.39 KB
13.07 KB

#360-2 fixed based on #348; passed locally. Gotta clock in, but will be on IRC today.

jhedstrom’s picture

Thank you so much to everybody that has been working on this!

This looks good. I've reviewed the patch in #367 quite extensively and have not found anything outstanding. A manual test was also successful.

I'm still not entirely happy with the separate module approach, but @xjm has addressed most of my concerns in #2788253-25: Plan for DateTime Range experimental module.

I hope to see this in 8.2.0, and also hope to see the same level of enthusiasm in the follow-up issues as we move to make this an official module that is enabled by default in the standard profile.

Leaving at NR for now, but I consider this RTBC and will mark it as such this evening (somebody else, feel free to do so as well) if there are no further issues with the current code that isn't already addressed by follow-ups.

effulgentsia’s picture

Assigned: Unassigned » xjm
Status: Needs review » Reviewed & tested by the community

Looks great to me too, so RTBC.

This issue still is tagged "Needs release manager review", so assigning to @xjm.

xjm’s picture

I'm in the process of doing a final review on the patch. Some collected notes in case d.o melts:

  1. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -0,0 +1,28 @@
    +      $output .= '<p>' . t('The Datetime Range module provides a Date field that stores start dates and times, as well as end dates and times. See the <a href=":field">Field module help</a> and the <a href=":field_ui">Field UI module help</a> pages for general information on fields and how to create and manage them. For more information, see the <a href=":datetime_do">online documentation for the Datetime Range module</a>.', array(':field' => \Drupal::url('help.page', array('name' => 'field')), ':field_ui' => (\Drupal::moduleHandler()->moduleExists('field_ui')) ? \Drupal::url('help.page', array('name' => 'field_ui')) : '#', ':datetime_do' => 'https://www.drupal.org/documentation/modules/datetime_range')) . '</p>';
    +      $output .= '<h3>' . t('Uses') . '</h3>';
    +      $output .= '<dl>';
    +      $output .= '<dt>' . t('Managing and displaying date fields') . '</dt>';
    +      $output .= '<dd>' . t('The <em>settings</em> and the <em>display</em> of the Date field can be configured separately. See the <a href=":field_ui">Field UI help</a> for more information on how to manage fields and their display.', array(':field_ui' => (\Drupal::moduleHandler()->moduleExists('field_ui')) ? \Drupal::url('help.page', array('name' => 'field_ui')) : '#')) . '</dd>';
    +      $output .= '<dt>' . t('Displaying dates') . '</dt>';
    +      $output .= '<dd>' . t('Dates can be displayed using the <em>Plain</em> or the <em>Default</em> formatter. The <em>Plain</em> formatter displays the date in the <a href="http://en.wikipedia.org/wiki/ISO_8601">ISO 8601</a> format. If you choose the <em>Default</em> formatter, you can choose a format from a predefined list that can be managed on the <a href=":date_format_list">Date and time formats</a> page.', array(':date_format_list' => \Drupal::url('entity.date_format.collection'))) . '</dd>';
    +      $output .= '</dl>';
    

    This should probably just reference the main datetime help and handbook page (rather than an nonexistent datetime_range handbook page). That's docs gate, so I've added it to the summary of #2788253: Plan for DateTime Range experimental module.

  2. @borisson_ said:

    I love that these methods are friendly :)

    I had the same thought when reading that docblock. ;) There are a few docblocks that could be better, but since this is an experimental module, I'm going to take advantage of the loosened core gates and not post a bunch of minor docs feedback.

  3. I added #339.6 (about whether the space should be included or concatenated with the separator) to #2788253: Plan for DateTime Range experimental module.
  4. That also led me to notice that the separator does not support HTML entities. (E.g., you cannot use an &emdash; or whatever some typography stickler would prefer over a hyphen.) This might also impact translatability. Adding that to the "should" as well. On the upshot, though, at least it's not vulnerable to XSS:

    (Thanks, Twig!)

xjm’s picture

I would've thought the separator should be translatable, but I could not figure out how to translate it if so. It's possible I missed something since the config translation UI is a touch arcane, but added it to the plan issue in case I didn't.

xjm’s picture

Issue summary: View changes
FileSize
48.73 KB

(Just reuploading the file attachment since it went somewhere strange.)

xjm’s picture

Another thing I've noticed reading through the updated patch is that there is still some code duplication we could refactor away besides what ended up in the formatter trait. Many methods on the various plugins override the parent method to more or less repeat what it did, but on both the start and end dates. We could refactor away a lot of this by creating protected helper methods for things that are used internally in the parent and then reused in the overridden child method. However, some of this is probably also affected by the ongoing work to refactor and improve timezone handling, so I'll leave it to the subsystem maintainers to decide how to best fit it on their roadmap. That also would have been potentially commit-blocking for me with non-experimental code, so I'll add it as a note on the plan summary as well.

xjm’s picture

xjm’s picture

Saving additional reviewer credit (which BTW is really annoying when the issue is two pages). :P

  • xjm committed b165600 on 8.3.x
    Issue #2161337 by mpdonadio, darrick, swentel, effulgentsia, YesCT, xjm...
xjm’s picture

Version: 8.3.x-dev » 8.2.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs release manager review

Alright. I have:

  • Confirmed that mpdonadio, jhedstrom, and all the core committers are comfortable with this as an experimental module.
  • Manually tested the patch again as an experimental module
  • Fiddled with various settings both unique to this field and shared with the parent
  • Tested installing the module, creating fields using it, deleting the fields using it, uninstalling the module, and reinstalling the module.
  • Done a code review of the whole patch (though not a line-by-line review), excluding the tests (to which I said "Look at all the nice tests".)
+++ b/core/modules/datetime_range/src/Plugin/Field/FieldWidget/DateRangeWidgetBase.php
@@ -0,0 +1,167 @@
+  protected function createDefaultValue($date, $timezone) {

+++ b/core/modules/datetime_range/src/Tests/DateRangeFieldTest.php
@@ -0,0 +1,1432 @@
+    'Pacific/Kwajalein',

I did not know that was a place!

@effulgentsia has already signed off on this as well. Based on that and my own review, I'm comfortable committing this as an experimental module.

@catch, @effulgentsia, @alexpott, @Cottser, and I also discussed today whether everyone was comfortable backporting #367 at least in principle. (Only @effulgentsia and I have actually reviewed the patch itself.) We decided to backport it, for the following reasons:

  1. Migrating data from a contributed module to a core module in a minor update is very awkward for both site owners and contrib authors. We talked through several scenarios for accomplishing this, and none of them were pretty. (D8 migrate sources will make this easier, but those are a ways off as the Migrate initiative is still working on stabilizing D7.)
  2. Contributed modules that are very important in the ecosystem (like Calendar) are stalled on this for D8, and to be viable they would have to choose between requiring 8.3.x (and therefore being considered "not ready" themselves) or backporting the core changes to contrib, leading again to all the problems with #1.
  3. The module is optional, experimental, and disabled by default, so there is no disruption to existing modules or sites, and users are warned that there is outstanding work.
  4. We have almost two weeks before the release window for the RC.

It's important to remember that this backport is also entirely a one-off. None of those reasons mean that we will break our release cycle schedule or policy in the future for a different patch. I should also note for posterity that the following things are not factors in deciding to backport the issue:

  • The impact on the site builder audience. This is definitely a major feature, but lots of other features are too. With the 6-month core release cycle, we do not risk any release date on any feature, no matter how important. The beta and RC phases are part of being able to ship a reasonably reliable minor, and while sometimes an individual feature might seem low-risk, the cumulative impact of rushing in features and deferring their technical debt is regressions, broken websites, and an unmanageable issue queue. (Plus, site builders could choose to run the 8.3.x patch on their sites once committed, and even have some confidence that it would be safe in the long term precisely because it was in the upcoming branch already.)
  • When the issue was RTBC. While all the teamwork that went into this prior to and during the beta is stellar, an RTBC patch isn't necessarily done and being RTBC does not mean the work will necessarily be committed. (Also, it was RTBCed right before the weekend that was right before the published commit freeze, and it's over 100K with lots of moving parts and user-facing functionality. Committers are also human and a committer review can take a long time and surface many things that need consideration.)
  • Making people happy. :) It is really rewarding when Drupal's features make people happy, but release management policies are about ensuring stability (which hopefully also makes people happy at the end of the day).

If you are following this issue and completely lost as to what gets into core when and why, you can read the Release cycle overview and Allowed changes during the Drupal 8 release cycle. Policies are never perfect and never cover every contingency, but those are the result of lots of thought and contribution across the community. The committers are also discussing some small improvements to clarify expectations for the next beta when it comes next year.

TLDR, disclaimers etc., blah blah blah. With the backport, barring some critical issue we haven't discovered, this feature will be available in 8.2.0, its release candidate in a couple weeks, and a beta that I will probably release tomorrow.

The work that has gone into this feature is awesome, and I've found the maintainers' responsiveness very helpful in navigating a difficult series of decisions. Thank you to everyone who contributed thoughtfully to this feature. I look forward to us completing the work outlined in #2788253: Plan for DateTime Range experimental module so that we can eventually enable this feature in standard.

  • xjm committed 6c985db on 8.2.x
    Issue #2161337 by mpdonadio, darrick, swentel, effulgentsia, YesCT, xjm...
xjm’s picture

Issue tags: +8.2.0 release notes
heddn’s picture

Thank you everyone for all your effort here. I know a lot of pain, blood, sweet and tears went into this. Too many folks to thank and I'm sure I'd miss someone, so thank you Drupal community. This is why we are the best open source community on the planet.

ryantollefson’s picture

Thanks for getting this in dev as experimental!

Where should we track issues at this point?

Gábor Hojtsy’s picture

Component: datetime.module » datetime_range.module

@ryantollefson: Let's use the module's own component.

xjm’s picture

This module was supposed to be treated as part of the datetime component. Not sure who created the datetime_range component.

xjm’s picture

Component: datetime_range.module » datetime.module

Merging the issue back to the datetime component after checking with the subsystem maintainers in #2788253: Plan for DateTime Range experimental module. So please file issues with this module (or any datetime functionality) against the datetime component. Thanks!

xjm’s picture

Assigned: xjm » Unassigned

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Tsjippy’s picture

So to make it clear:
I have installed drupal 8.2.4.
Which patch do I need to apply to make an enddate available in the date field?

dpi’s picture

@Tsjippy no patch, install "Datetime Range" module. Its a new field.

Tsjippy’s picture

Thanks, totally overlooked this!

It works

Tsjippy’s picture

Hmm, I cannot get it to work in a view, for instance the from the Calendar module nor the Fullcalendar module.

It also lacks the recurring option.

ANy ideas how to use it?

MrPeanut’s picture

@Tsjippy — Those are out of the scope of this issue. Here are some issues for recurring/repeating events:

#2305073: D8: Date repeat feature requests
#2775237: Repeating dates
#2775249: Provide a field for repeating / recuring dates

Or maybe check out Date Recur Field (repeating dates).

dankegel’s picture

In case anyone didn't notice the many references to https://www.drupal.org/node/2788253 above, it's a happening place at the moment :-)