The Views integration Datetime Range fields should extend the views integration for regular Datetime fields

As I've been working on this bit for work, I felt I should submit my code as initial work.
I'm aware probably this is not the best approach, but it was the less disruptive while easily extendible way.

Also I wasn't sure about Tests: should these be covered in datetime_range too or the ones on datetime component are enough? If they should, are we going to create new tests or extend (and maybe refactor) datetime ones?

PS: Shouldn't this issue target version 8.3.x?
PPS: Current patch applies nicely on 8.2.x as well, with or without #2627512: Datetime Views plugins don't support timezones as long as this patch is applied after.

+++ b/core/modules/datetime/datetime.views.inc
@@ -11,18 +11,38 @@
+/**
+ * Helper for datetime based fields.
+ *
+ * Override the default Views data for a datetime based fields,
+ * adding datetime views plugins.
+ *
+ * @param FieldStorageConfigInterface $field_storage
+ *   The field storage config entity.
+ * @param array $data
+ *   Field view data or views_field_default_views_data($field_storage) if empty.
+ * @param string $column_name
+ *   The schema column name with the datetime value or 'value' if empty.
+ *
+ * @return array
+ *   The array of field views data with the datetime plugin.
+ */
+function _datetime_type_field_views_data(FieldStorageConfigInterface $field_storage, $data = [], $column_name = 'value') {

Should probably add an @internal to this.

Think this is a good approach.

Not sure about the best way to add test coverage. We want to use as much as possible from the existing tests, but not copy them and maintain two nearly identical versions.

Wow that's super clean.

@dawehner, what do you think the best approach for testing is? We have coverage that the plugins function as expected in datetime. Do we need coverage that the plugins work as expected in daterange (which would maybe need to clone the existing tests, ug), or can we just make some test views and ensure that the proper plugins will be used?

or can we just make some test views and ensure that the proper plugins will be used?

Yeah, I would actually test the behaviour/expected view result etc. Just testing the views_data is a bit too less, IMHO.

This patch just adds @internal tag as per #6.

I'll have a look at the tests soon, testing the expected results with filter / argument / sort against 3 nodes. More or less a shorter version of current datetime tests without timezones loop.

From limited testing this seems to work well with 8.2.x-dev but doesn't provide the additional datetime arguments for daterange value or end_value when installed under 8.2.0-rc1.

The steps I took:
* 8.2.0-rc1 installed from download. Patched with #10, drush cr, added a daterange field, added a view with daterange and daterange end_value as filters. Operators available are string based: 'Is equal to', 'Start with', 'Contains string' etc.

* 8.2.0-dev installed from git. Patched with #10, drush cr, added a daterange field, added a view with daterange and daterange end_value as filters. Operators available are numeric as expected: 'Is less than', 'Is less than or greater to' etc.

Bit strange, it doesn't look like there were any dev commits to datetime or daterange since rc1's release.

@dkre I'm currently using the patch on 8.2.0-rc1 and it works fine.
To be honest I had the same issue, datetime views plugin not loading, but after a Views filters clean up (removing all filters/sort making use of the datetime_range fields) and a drush cr everything started working properly.

I'll have a look anyway to see if the issue a replicable pattern.

UPDATE: I confirm it works with 8.2.0-rc1 and I couldn't replicate the issue as a standard drush cr was enough.

Yeah sorry you're right, not sure what I kept missing - I tested this 3 times!

Update:
Following up on this, I originally wasn't able to get this patch to work on my current site in development which led me to testing on various installations.

It appears that this won't work with end_values if the field instance was created before this patch is applied. Within the same view a filter applied on a pre-existing datetime_range:end_value will result in being handled like a string as mentioned above, whereas a filter applied to a newly created datetime_range:end_value will work perfectly.

Is this the expected way D8 handles configuration or is this a bug? I haven't run into a similar issue.

#13, did you do a `drush cr` after applying the patch?

I have a sneaking suspicion that the config for the view you created for the patch has standard plugins wired into it, and that we may need an update hook to swap them (this one won't be that hard, though). https://www.drupal.org/project/config_inspector may provide some insight here. Also wondering if just loading and saving the existing view would fix it.

I can't find @dawehner or @timplunkett online right now; I'll ping one of them tomorrow for an opinion.

Will try to test this myself tonight or tomorrow.

@mpdonadio, thank you for the advice/insight.

Yes, I've rebuilt the cache/registry at evey step. I also reinstalled 8.2.0-rc1 core to ensure there weren't any other patches I had forgotten about that may be effecting things.

I'll have a play around now and report the results shortly.

Confirmed. Existing views that have end_value in them will have 'plugin_id' => 'string' in them. We will need an upgrade path to change these to 'datetime', and potentially massage the value for filters to ensure that they are valid.

Raising this to major as I was able to make my system fatal when editing a view that used end_value after applying this patch:

InvalidArgumentException: The configuration property display.default.display_options.filters.field_date_range_1_end_value.value.type doesn't exist. in Drupal\Core\Config\Schema\ArrayElement->get() (line 74 of core/lib/Drupal/Core/Config/Schema/ArrayElement.php).
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.field_date_range_1_end_value.value.type', 'offset') (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.field_date_range_1_end_value.value', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters.field_date_range_1_end_value', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options.filters', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default.display_options', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display.default', Array) (Line: 211)
Drupal\Core\Config\StorableConfigBase->castValue('display', Array) (Line: 212)
Drupal\Core\Config\Config->save() (Line: 280)

To reproduce

- checkout 8.2.x
- do a clean install
- add a daterange field to page
- add a node w/ this field populated
- make a view over page, and filter on the end_value, use = and set it to something
- save the view
- apply #10
- drush cr
- use config_inspector, not that plugin_id is still string
- edit the filter you added
- change the value to relative, now
- save the view
- see exception

I can confirm #16

This is resolved by importing the view with the filter altered to include a value array and the plugin changed:

Config yml - original

...
          value: now
...
          plugin_id: string
...

Config yml - required changes

...
          value:
            min: ''
            max: ''
            value: now
            type: offset
...
          plugin_id: datetime
...

Shown is with a relative offset of 'now'.

I am still unable to get my existing datetime_range:end_value to use this patch but this is most likely due to the timing of when I created the field which may have been during DateRange development (Issue 2161337). I haven't been able to replicate the the problem on a clean install.

Configuration Inspector is invaluable, thank you again for the tip.

Removing myself from assignment as I need a bit of time to study D8 upgrade paths procedure before providing any work.

What would be the right way?
Scanning and updating all the views having end_date as filter?
What about start date? Have you noticed if it's affected too?

#18, there is a big issue where we swapped out views plugins for base formatter plugins and had to do an update path. This one would be really similar. I need to dig to find the issue, and will post it once I do.

This has a similar update hook, #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter, but we will have to do field type checking instead of just looking at the plugin ID.

Thanks @mpdonadio.

I'm assuming for value and type combinations we use the existing filter value for the first and 'offset' for the second. It seems the safest option.

From further testing it appears that the value type isn't being set correctly install for filters and both label and type for sort on a clean install.

From config_inspector:

Filters:
display.default.display_options.filters.field_date_value.value - Type: undefined, Error: missing schema
display.default.display_options.filters.field_event_date_end_value.value - Type: undefined, Error: missing schema

Sort:
display.default.display_options.sorts.field_date_value.granularity - Label: undefined, Type: undefined, Error: missing schema

This is on a clean install of either 8.2.x-dev or rc2. Steps taken:
Install Drupal > Patch with #10 > enable DateTime Range and Inspector > Add field > Add a node with field > Add view with value and end_value filters + value sort.

I missed this earlier because the view still works but this may explain the upgrade issue?

Here's the view config:

uuid: bc002928-b14b-4e61-9ace-fe2ba30a0f92
langcode: en
status: true
dependencies:
  config:
    - core.entity_view_mode.node.teaser
    - node.type.article
  module:
    - datetime
    - node
    - user
id: date
label: Date
module: views
description: ''
tag: ''
base_table: node_field_data
base_field: nid
core: 8.x
display:
  default:
    display_plugin: default
    id: default
    display_title: Master
    position: 0
    display_options:
      access:
        type: perm
        options:
          perm: 'access content'
      cache:
        type: tag
        options: {  }
      query:
        type: views_query
        options:
          disable_sql_rewrite: false
          distinct: false
          replica: false
          query_comment: ''
          query_tags: {  }
      exposed_form:
        type: basic
        options:
          submit_button: Apply
          reset_button: false
          reset_button_label: Reset
          exposed_sorts_label: 'Sort by'
          expose_sort_order: true
          sort_asc_label: Asc
          sort_desc_label: Desc
      pager:
        type: some
        options:
          items_per_page: 10
          offset: 0
      style:
        type: default
      row:
        type: 'entity:node'
        options:
          view_mode: teaser
      fields:
        title:
          id: title
          table: node_field_data
          field: title
          entity_type: node
          entity_field: title
          label: ''
          alter:
            alter_text: false
            make_link: false
            absolute: false
            trim: false
            word_boundary: false
            ellipsis: false
            strip_tags: false
            html: false
          hide_empty: false
          empty_zero: false
          settings:
            link_to_entity: true
          plugin_id: field
          relationship: none
          group_type: group
          admin_label: ''
          exclude: false
          element_type: ''
          element_class: ''
          element_label_type: ''
          element_label_class: ''
          element_label_colon: true
          element_wrapper_type: ''
          element_wrapper_class: ''
          element_default_classes: true
          empty: ''
          hide_alter_empty: true
          click_sort_column: value
          type: string
          group_column: value
          group_columns: {  }
          group_rows: true
          delta_limit: 0
          delta_offset: 0
          delta_reversed: false
          delta_first_last: false
          multi_type: separator
          separator: ', '
          field_api_classes: false
      filters:
        status:
          value: '1'
          table: node_field_data
          field: status
          plugin_id: boolean
          entity_type: node
          entity_field: status
          id: status
          expose:
            operator: ''
          group: 1
        type:
          id: type
          table: node_field_data
          field: type
          value:
            article: article
          entity_type: node
          entity_field: type
          plugin_id: bundle
        field_date_value:
          id: field_date_value
          table: node__field_date
          field: field_date_value
          relationship: none
          group_type: group
          admin_label: ''
          operator: '='
          value:
            min: ''
            max: ''
            value: now
            type: offset
          group: 1
          exposed: false
          expose:
            operator_id: ''
            label: ''
            description: ''
            use_operator: false
            operator: ''
            identifier: ''
            required: false
            remember: false
            multiple: false
            remember_roles:
              authenticated: authenticated
          is_grouped: false
          group_info:
            label: ''
            description: ''
            identifier: ''
            optional: true
            widget: select
            multiple: false
            remember: false
            default_group: All
            default_group_multiple: {  }
            group_items: {  }
          plugin_id: datetime
        field_date_end_value:
          id: field_date_end_value
          table: node__field_date
          field: field_date_end_value
          relationship: none
          group_type: group
          admin_label: ''
          operator: '='
          value:
            min: ''
            max: ''
            value: now
            type: offset
          group: 1
          exposed: false
          expose:
            operator_id: ''
            label: ''
            description: ''
            use_operator: false
            operator: ''
            identifier: ''
            required: false
            remember: false
            multiple: false
            remember_roles:
              authenticated: authenticated
          is_grouped: false
          group_info:
            label: ''
            description: ''
            identifier: ''
            optional: true
            widget: select
            multiple: false
            remember: false
            default_group: All
            default_group_multiple: {  }
            group_items: {  }
          plugin_id: datetime
      sorts:
        created:
          id: created
          table: node_field_data
          field: created
          order: DESC
          entity_type: node
          entity_field: created
          plugin_id: date
          relationship: none
          group_type: group
          admin_label: ''
          exposed: false
          expose:
            label: ''
          granularity: second
        field_date_value:
          id: field_date_value
          table: node__field_date
          field: field_date_value
          relationship: none
          group_type: group
          admin_label: ''
          order: ASC
          exposed: false
          expose:
            label: ''
          granularity: day
          plugin_id: datetime
      title: Date
      header: {  }
      footer: {  }
      empty: {  }
      relationships: {  }
      arguments: {  }
      display_extenders: {  }
    cache_metadata:
      max-age: -1
      contexts:
        - 'languages:language_content'
        - 'languages:language_interface'
        - 'user.node_grants:view'
        - user.permissions
      tags: {  }
  page_1:
    display_plugin: page
    id: page_1
    display_title: Page
    position: 1
    display_options:
      display_extenders: {  }
      path: date
    cache_metadata:
      max-age: -1
      contexts:
        - 'languages:language_content'
        - 'languages:language_interface'
        - 'user.node_grants:view'
        - user.permissions
      tags: {  }

Update path added to the patch.

It wasn't easy as views_field_default_views_data() doesn't add 'entity_type' and 'field_name' to the filter and the sort plugins instances. I'll find out if this is a bug and open a separated issue if needed.
However @dawehner helped on pulling these details from the info we have.

Something to be flagged:

1. I've spent long time trying to dedupe the code, using helpers to wrap common bits, but although filters and sorts loops are generally similar, the small differences make this really really tricky. So I ended up leaving the code in a single function.
2. The update path set filters values to the current values, if any, and the type to 'offset' in order to be able to work with a wider range of current values than type 'date' would do.
3. The update path set sorts granularity to 'seconds' in order to cover both 'date' and 'datetime' storage formats.
4. The update path outputs a standard message listing the views been changed, however I feel we need a stronger copy suggesting the user to manually review the views.

1. +++ b/core/modules/datetime_range/datetime_range.install
   @@ -0,0 +1,115 @@
   +  foreach ($config_factory->listAll('views.view.') as $view_config_name) {
   +    $view = $config_factory->getEditable($view_config_name);
   

   Is this preferable to using entity storage and loadMultiple, which would return proper Views objects?

2. +++ b/core/modules/datetime_range/datetime_range.install
   @@ -0,0 +1,115 @@
   +              // Make sure we retrieve the right value, even if the user
   +              // already updated the filter.
   +              $value = isset($filter['value']['value'])
   +                ? $filter['value']['value']
   +                : $filter['value'];
   

   What's going on here with the double nested 'value'? Is that a known Views issue? If this is correct, the code comment could perhaps explain the nesting?

1. Is this preferable to using entity storage and loadMultiple, which would return proper Views objects?

I was actually following the suggestion at #20 about the similar update path from #2455125: Update EntityViewsData use of generic timestamp to use Field API formatter.
I leave the answer to who knows the difference deeply.

2. What's going on here with the double nested 'value'? Is that a known Views issue? If this is correct, the code comment could perhaps explain the nesting?

It's the way a datetime plugin value is stored in the configuration:

  'value' => [
    'min' => '',
    'max' => '',
    'value' => '2016-09-29',
    'type' => 'date',
  ],

By default with 'plugin_id' => 'string' the value property looks like 'value' => '2016-09-29', but if the user has updated the view that will look like the structure above.
Then I'm considering both situation.

I didn't want to write all of this in the code comment, it seemed too long.
Maybe change it to something like: "Make sure we retrieve the right value (by default $filter['value']), even if the user already updated the filter (so $filter['value']['value'])." ?

Thanks for clarifying the double 'value' bit.

Over in #2455125-88: Update EntityViewsData use of generic timestamp to use Field API formatter @dawehner states that the config object approach is preferable, so disregard using the entity objects.

As stated on most related issues #2627512: Datetime Views plugins don't support timezones is a blocker for this and I don't think we can't make any real progress on tests until that one is committed.

However I made a PoC for the testing approach, trying to reuse the more existing datetime code I can.
In this PoC I slightly modify the DateTimeHandlerTestBase class in order to make the field type configurable, and from the datetime_range tests I re-use the datetime_test module views adding the daterange end value filter during setUp().

This patch includes only the Filter test.

I'm not sure if either of these falls under the scope of this patch. If you think they're worth making separate issues for, let me know. I'm trying to use this module and these are the problems I'm hitting specific to views:

1) I need to group the results by the month the event starts in. I added the field to the view and formatted it to only show the month and then used the rendered output as the group by. One day events end up grouped by "Month" but multiple day events get grouped by "Month - Month" even when they are in the same month so I end up with "October" and "October - October". There seems to be no way to tell it to just use the start date. When I selected the field, I had a choice of date range or date range (end date). No start date.

2) I need to show only events from this year. This patch improves the filter so I can set the date equal to a date rather than just a textfield but there is no way to set the granularity so it is only comparing the year. (Edit: Realized this isn't possible with a regular date, either, and that I was missing the obvious of simply adding it twice and using less than / greater than.)

#30, is this with or without the latest patch applied?

Both. I applied the patch hoping it would help but it didn't.

#30 I'm seeing the same behavior but I was working with day based groupings.

Essentially views is looking at both values combined (value and end_value) so 1.1.1979 is not equal to 1.1.1979 - 1.1.1979 because the second field has an end value. At a guess this would be because it will be looking at the whole value array as comparison of difference to be considered a unique group?

I'm looking at using TARDIS (see docs here) as a basis for a display format rather than groupings. Groupings does have the problem where by you may not have a month (or date in general) displayed because there aren't any items to list which is important if you need that calendar ux.

I havent yet looked into how I was going to handle the in between start and end date filtering yet however.

I applied patch #28, and did these steps:

1. Enabled Datetime Range module
2. Created new content type and added new datetime range field
3. Added content
4. Created new view
5. Added new date field as filter

Results:

• Appropriate Operations were available, such as "Greater than or equal to", "less than", etc.
• Filter was appropriately filtering views output.

#30, I think I know what is happening, but I don't think it is a Views problem. I think the issue is the logic in the field formatter is outputting 'October - October' because the start and end dates are different, but the output with the 'F' format string is the same. The logic is in:

        if ($start_date->format('U') !== $end_date->format('U')) {
          $elements[$delta] = [
            'start_date' => $this->buildDate($start_date),
            'separator' => ['#plain_text' => ' ' . $separator . ' '],
            'end_date' => $this->buildDate($end_date),
          ];
        }
        else {
          $elements[$delta] = $this->buildDate($start_date);
        }

So, it is comparing actual timestamp and not rendered output. Not sure if we want to call this a bug or expectation; it may be worth an issue so we can at least discuss this.

As far a not being able to select the start vs end value, I am not sure where you are talking about, as with the patch I am seeing both. It is just unfortunate with views right now, that the labeling with multi-valued fields (look at images) is a bit wonky.

I ran into an issue with using the patch from #28. Seemingly by random when loading a view or views-related page, I was greeted by a whitepage:
PHP Fatal error: Call to undefined function _datetime_type_field_views_data() in /srv/www/htdocs/MYPROJECT/html/core/modules/datetime_range/datetime_range.views.inc on line 14
A cache rebuild temporarily fixes this issue.

Apparently, the datetime.views.inc is not always being loaded, so I added \Drupal::service('module_handler')->loadInclude('datetime', 'inc', 'datetime.views'); to the top of datetime_range_field_views_data which seems to work.

That's weird. I'd never had this issue before yesterday, and it's just appeared to me too.

Besides the @internal tag for _datetime_type_field_views_data() does logically create the same issue as extending code should not call these functions or methods directly.

Together with the include from #36 I'm going to remove the @internal tag and rename the function datetime_type_field_views_data, so without the underscore prefix.

I'm not sure this is relevant to this discussion, but I have also noticed that whenever I have a date range consisting of start/end dates of the same day. In other words sometimes the event is just a one-day event rather than multi-day.

In such cases I have found that the normal content type display formatter will render it as "October 27, 2016" (for example) whereas Views formats it as "October 27, 2016 - October 27, 2016". It seems to me the views format should be consistent with the one used on the content type formatter. Happy to open a separate issue if this is out of the scope of this discussion.

@caspervoogt not sure what you mean with "the normal content type display formatter" and "Views formats"

I've just tested using the "HTML Date" standard date format on both Default and Teaser content display modes, and set up a View displaying teasers.
The output of the date fields is the same, single date if start and end dates are the same and multiple with separator if they are not, in both the content full display page and the View.

@gambry I am using a view with fields, rather than content display mode, FWIW.
I had created a custom date formatter (October 27, 2016) and it shows as "October 27, 2016 - October 27, 2016" in Views, but as "October 27, 2016" on the node's Full Content display. I did also try HTML Date but that shows as "2016-10-27 - 2016-10-27" - same issue. It's fine on the Full Content node display. I am running 8.2.1.

Anyway, I just tested on Simplytest.me and it works fine there so whatever it is must be local to my environment (Pantheon, though it may not have anything to do with it).

I don't know if this is specific to DateRange or also applies to Date filters, but if I create an exposed filter and leave the value empty, it defaults to 'now' and I get a notice if I search without a value:

Notice: Undefined index: type in Drupal\views\Plugin\views\filter\Date->acceptExposedInput() (line 125 of core/modules/views/src/Plugin/views/filter/Date.php).
Drupal\views\Plugin\views\filter\Date->acceptExposedInput(Array) (Line: 1353)
Drupal\views\ViewExecutable->_build('filter') (Line: 1249)
Drupal\views\ViewExecutable->build() (Line: 351)
Drupal\views\Plugin\views\display\PathPluginBase->execute() (Line: 168)
Drupal\views\Plugin\views\display\Page->execute() (Line: 1617)
Drupal\views\ViewExecutable->executeDisplay('school_shifts', Array) (Line: 78)
Drupal\views\Element\View::preRenderViewElement(Array)
call_user_func(Array, Array) (Line: 376)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 195)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 226)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 574)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 227)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 117)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.view', Object) (Line: 149)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 64)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 99)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 78)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 50)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 652)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

If I add another filter on the value_end part of a date range, that one does not default to 'now' and shows the notice immediately on viewing the View.

(I'm trying to create basic "From" ... "To" exposed filters for a date range field. The exposed widget isn't very UX friendly but it works for now.)

@TwoD I don't quite understand your scenario.

Are you using this patch? Have you run the provided updates?
If all the answers are yes, can you please provide the steps to reproduce your problem?

Yes, I am using the patch. I did run the updates (which did nothing because I had no existing Views).
I have confirmed this is a problem with regular date fields as well, so this should probably be moved there, but I noticed this patch touches both types.

To reproduce:

1. Create a content type with a date or daterange field.
2. Create a new View of that content type with a page display.
3. Add a filter on that date field, make it exposed, leave the default value empty.
4. The query should become something like this:
   

   SELECT node_field_data.created AS node_field_data_created, node_field_data.nid AS nid
   FROM 
   {node_field_data} node_field_data
   WHERE (( (node_field_data.status = '1') AND (node_field_data.type IN  ('notice')) ))
   ORDER BY node_field_data_created DESC
   LIMIT 10 OFFSET 0

Save and view the page display and you'll get the notice. (Won't show up in the preview.)

To clarify; the notice shows when there is no value for the exposed filter and it's left out of the query.
The notice does not show if the View was cached.

#45, that is definitely not related to this issue. Can you try the patch on #2369119: Fatal error when trying to save a View with grouped filters using other than string values and see if it fixes your problem or possibly the one on #2648950-42: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter? The symptom is the same, but not sure if it is a dup. If neither of those help, then can you open a new issue in datetime.module? Thanks.

Reviewing things that have not gotten attention lately. I think this just needs an update path test and we should be good to go?

Is the patch from #39 supposed to give an option to add just the start date as a field? For example, if my daterange field is 2016-11-27 12:00:00 through 2016-11-28 14:00:00, I'd like my field to only show 2016-11-27.

#48 yes, it should. When you filter on a daterange field, you will see two options:

- The Field Name (field_the_field_name)
- The Field Name (field_the_field_name:end_value)

The first is the start value. I don't think we can make this clearer; I forget which issue that is, but I think it is a general Views quirk with all multi-valued fields.

Is this not working for you? If not, can you post something that we can try to reproduce?

@mpdonadio — Sorry, I'm referring to fields, not filters. The two fields do show up on filters.

#50, views field display uses the field formatters in Drupal 8, and not views plugins anymore (like Drupal 7 did, and pre-release versions of Drupal 8). None of the existing daterange formatters will allow you to just show start or end date. You would need to make your own formatter to do this. There are also no plans for this type of formatter in Drupal 8 since it is really easy to make a formatter and attached it to a field type now.

@TwoD, @mpdonadio #43 & #45 is handled in #2811887: Exposed date filter leads to a notice

Hi,

I'm using patch #39 in a production site with success.

All basic functionality you'd expect from this module works well form me (filtering, ordering, etc).

So far, no issues.

Can I use this patch to see the end date with the Calendar module?

#54 This is probably a question for the Calendar issues queue.

I think the titles of the 6 arguments that this patch creats need to indicate if the argument is for the start date or end date. Right now it's completely ambiguous and site builders have to guess which one corresponds with the start date and which one with the end date:

The title should probably also have the label of the human readable label for the field in it as well, which in my case was just "Date".

@bkosborne I made another patch that solve the problem

@adriancid, what did you diff against to make the patch? This does not apply locally for me against a fresh pull of 8.2.x. Also, remember to post an interdiff w/ each new patch.

@bkosborne, can you confirm which version of the patch you have applied, which version of Drupal you applied it to, and what find or filter/argument you are creating to get that? Is via a relationship? I can't reproduce that, and if it is what I think it is, is a views problem and not directly caused by this patch.

Sorry for the bad patch. I test this and its works

I just changed a line

Output:

For start date:
field_machine_name:value
For end date:
field_machine_name:end_value

+++ b/core/modules/datetime/datetime.views.inc
@@ -33,11 +53,11 @@ function datetime_field_views_data(FieldStorageConfigInterface $field_storage) {
+        'title' => $field_storage->getLabel() . ':' . $column_name . '(' . $argument_type . ')',
         'help' => $help_text,

OK, this is in the contextual filters (and a very good catch!). The problem with this change is that it will also affect the plain datetime fields, which is an out-of-scope change (and probably a UI regression). We need to do two things here:

1. Make sure we only add in the column name for daterange fields.
2. To be consistent with how the rest of views works, we should only add in the column name when it isn't 'value', is that it would look something like

node.field_date_range (day)
node.field_date_range:end_value (day)


And make sure the space is there for readability.  I think we need to hardcode `$column_name !== 'value'`; I don't think we an alternative here?

@mpdonadio.

Here the new patch with your recomendations:

I think that we need as you says:

$column_name !== 'value';

If you see the line 33 is:

function datetime_type_field_views_data(FieldStorageConfigInterface $field_storage, $data = [], $column_name = 'value') {

So we have $column_name = 'value'

The labels of the arguments are really wacky and don't follow any convention.

For example, the label for the month argument appears as:

node.field_date:end_value (month)

When it should be something like:

Date, month only (field_date:end_value)

Should we fix it here? It would fix it for datetime as well as datetime_range. Since it affects datetime it's possibly out of scope of the issue, but if NOT fixing it for datetime but fixing it for datetime_range means we'd be writing extra code that we'd just rip out soon.

We cannot make any changes to datetime functionality or user facing text in this issue. The scope of this issue is

Give daterange fields the same Views integration capabilities that datetime fields have.

I'm fine with the change in #62 (though I haven't properly reviewed it), but any enhancements to this and the datetime labels need to be deferred to a followup.

The primary focus of work for this issue needs to be getting the update path and update path test working. We cannot commit this patch without those.

I am OK with #62. Created #2842571: Improve Datetime and Daterange Views contextual grouped filter names as a followup.

We just need manual testing of the update path, and an (ug) update path test to wrap this up.

@bkosborne after apply the patch #62 you can apply this patch and you will see the new format

Trying to push this forward, I started reviewing the upgrade path and found a couple issues.

1. +++ b/core/modules/datetime_range/datetime_range.install
   @@ -0,0 +1,115 @@
   +              // Set entity_type and field_name if missing.
   +              if (!isset($filter['entity_type'])) {
   +                $view->set($base . '.entity_type', $filter_views_data['entity_type']);
   +              }
   +              if (!isset($filter['field_name'])) {
   +                $view->set($base . '.field_name', $filter_views_data['field_name']);
   +              }
   

   I'm not experienced in views development, but why is this needed? These two config values are not set on a fresh view when I create a datetime filter, so why set them here?

2. +++ b/core/modules/datetime_range/datetime_range.install
   @@ -0,0 +1,115 @@
   +              // Make sure we retrieve the right value, even if the user
   +              // already updated the filter.
   +              $value = isset($filter['value']['value'])
   +                ? $filter['value']['value']
   +                : $filter['value'];
   

   In what situation would a user have already updated the filter? Even if they did, and we detect that they did, we should back out and not modify the value any further, right?

3. +++ b/core/modules/datetime_range/datetime_range.install
   @@ -0,0 +1,115 @@
   +              // Set datetime value.
   +              $datetime_value = [
   +                'min' => '',
   +                'max' => '',
   +                'value' => $value,
   +                'type' => 'offset',
   +              ];
   +              $view->set($base . '.value', $datetime_value);
   

   The offset should be set to "date", not "offset"; it's unlikely that the string value the user had provided for the filter before this patch would be a date offset, since that wouldn't have been a working filter.

4. +++ b/core/modules/datetime_range/datetime_range.install
   @@ -0,0 +1,115 @@
   +      // Update datetime_range filters.
   +      if (isset($display['display_options']['sorts'])) {
   +        foreach ($display['display_options']['sorts'] as $field_name => $sort) {
   +          if ($sort['plugin_id'] == 'standard') {
   

   Comment should be "Update datetime_range sort handlers"

5. +++ b/core/modules/datetime_range/datetime_range.install
   @@ -0,0 +1,115 @@
   +              // Set entity_type and field_name if missing.
   +              if (!isset($sort['entity_type'])) {
   +                $view->set($base . '.entity_type', $sort_views_data['entity_type']);
   +              }
   +              if (!isset($sort['field_name'])) {
   +                $view->set($base . '.field_name', $sort_views_data['field_name']);
   +              }
   

   same as above

Also, the upgrade path doesn't handle the change to the filter operator. The filter operators available for a string filter are almost completely different than those for a datetime filter. The exception is =. We'll have to create a mapping of string operators to the datetime operator that makes the most sense.

Here's a list of string operators that don't exist as datetime operators:

• contains
• word
• allwords
• starts
• not_starts
• ends
• not_ends
• not
• shorterthan
• longerthan

I could see someone having configured a views filter with "contains" or "start" so they could provide a filter value of "2016-01-18" to match "2017-01-18T18:00:00" (full datetime stored in database).

I don't see how we can provide an upgrade path for those people. Regex? ew

This may just be a breaking change for those configurations?

1. I'm not experienced in views development, but why is this needed? These two config values are not set on a fresh view when I create a datetime filter, so why set them here?

I'm not experienced too, but those values are needed for necessary for the FieldAPIHandlerTrait methods to work properly.
As mentioned above this patch is strongly depending on #2627512: Datetime Views plugins don't support timezones, where these values are set from the datetime_field_views_data() hook.
Not sure what action to do in here. If including those configuration values on this patch setting them from the datetime_range_field_views_data() hook, or postpone this while waiting for #2627512: Datetime Views plugins don't support timezones to be committed.
OR - of course - removing them while knowing FieldAPIHandlerTrait won't work properly.

2. In what situation would a user have already updated the filter? Even if they did, and we detect that they did, we should back out and not modify the value any further, right?

A user updating the filter before running the updates. Yours is a good point.
If an user already updated the filter we should skip it.

3. The offset should be set to "date", not "offset"; it's unlikely that the string value the user had provided for the filter before this patch would be a date offset, since that wouldn't have been a working filter.

As mentioned on #24: "2.The update path set filters values to the current values, if any, and the type to 'offset' in order to be able to work with a wider range of current values than type 'date' would do.". But I don't see any cases why the user would set an offset value while using the String plugin.
I agree to set it to date to avoid any confusion.

4. Comment should be "Update datetime_range sort handlers"

++

5. Also, the upgrade path doesn't handle the change to the filter operator. The filter operators available for a string filter are almost completely different than those for a datetime filter. The exception is =. We'll have to create a mapping of string operators to the datetime operator that makes the most sense.

That's a valid point to, however I'm not sure which assumptions we can make for the other String operators and so what to map to. Suggestions?

This patch addresses 2 (we already skip it, so no changes needed), 3 and 4 from #68. #69 explains what 1 and 5 are and why we need them. May be @mpdonadio and @jhedstrom can share their thoughts. I still think this patch should wait for #2627512: Datetime Views plugins don't support timezones to be committed.

Also, the upgrade path doesn't handle the change to the filter operator. The filter operators available for a string filter are almost completely different than those for a datetime filter. The exception is =. We'll have to create a mapping of string operators to the datetime operator that makes the most sense.

I've added '=' as default operator because we can't leave the string plugin one. I'm aware that this can be a breaking change and I still think a strong message is needed if any filter is updated. We need to let the user know a manual review is needed.

I don't see how we can provide an upgrade path for those people. Regex? ew

I Agree. If we need a map using regexp is the best option. It's a shame that:

Fatal error: Call to undefined method Drupal\datetime\Plugin\views\filter\Date::op_regex() in core/modules/views/src/Plugin/views/filter/NumericFilter.php on line 242

Is 'regex' operator supposed to be available on datetime filters?

So if we want to proceed with the mapping of the current values/operator then #2821112: Views NumericFilter 'regular_expression' operator is broken is a blocker for this issue.

Postponing this until #2821112: Views NumericFilter 'regular_expression' operator is broken is committed.

#2821112: Views NumericFilter 'regular_expression' operator is broken is RTBC this can be postponed now. Re-uploading #71 for testing.

Turns out #2786577: The Views integration Datetime Range fields should extend the views integration for regular Datetime fields is blocked on #2845543: PostgreSQL regular expression match operators works only for text.

I am suffering from a very serious and frightening bug since I used the patch in #60 on Drupal 8.2.5: All of a sudden all special characters are shown as question marks.
I am not sure whether or not the patch is related to this problem, the only hint I have, is

• The problem occurred only after I applied the patch.
• It occurs only on the Drupal 8 instance on my server on which I applied the patch.

That does not mean, however, that both the bug and the application of the patch are more than mere coincidence. However, as this is really the nastiest bug I ever experienced in Drupal, and led to a terrible loss of data, and above all I don't have a clue how to investigate it's cause, I wanted to let you know.

@gunwald if you can provide more details I'm glad to help, however I don't see how this patch can mess up the charset of your data.

In the meanwhile blockers for the update path mapping of existing string values have been committed so we can make progress on that side.

@gambry As a matter of fact, the patch was completely unrelated to my problem. It turned out, that Backup and Migrate has a terrible bug, the which destroys your database.
So it was mere coincidence that the problem showed up after I applied the patch. Sorry for the rumor!

By the way, does the latest patch in #76 work for Drupal 8.2.6?

Yes @gunwald, it should.

patch from #76 looks ok to me.
tested and works as expected on 8.2.6, 8.3.x and 8.4.x
anything else that needs can be done to land this?

This needs some manual testing for the upgrade path; it's dangerous.

We also need a proper upgrade path test to go along with the upgrade path.

I was just re-reading https://www.drupal.org/core/experimental

Datetime Range is still alpha-stage so

No upgrade path is provided from alpha versions. To resolve issues for the module, uninstall and reinstall it (backing up any data).

applies. I think that if we take #76 and remove the upgrade hook we can RTBC this. It may make sense to do this now, as the module may be approaching consideration for beta-stage now that all but one core gate is done, and that is RTBC. We can issue a CR to notify about this, along with some examples of how to fix views that already exist.

?

updated patch removing the update hook.

No upgrade path is provided from alpha versions. To resolve issues for the module, uninstall and reinstall it (backing up any data).

LOL. I had the same thought today while reading updates about other experimental modules.

The CR is a MUST-have, as system will become critical for those instances.

I've created a draft of CR. I think this are both for you to review @mpdonadio.
Thanks for the hard work to get this module in!

OK, #85 is a good version of #76 w/o the update hook.

I'm tentatively setting RTBC on this, but tagging for @xjm to make the call on the approach.

@xjm, since datetime_range is still at alpha status (not sure when this will be re-evaluated now that a whole bunch of patches landed), there is this

No upgrade path is provided from alpha versions. To resolve issues for the module, uninstall and reinstall it (backing up any data).

I am asking for your opinion on this because

(1) Not having the upgrade path can lead to nasty fatals once this applied for people who have the module enabled and views over daterange fields. We have a CR to this effect.

(2) The upgrade path is complicated (datetime + daterange + views === 'Fun!')

(3) She is smart and may have an idea about how to handle this situation that is eluding us.

As I'm typing this out, I'm wondering if we should add a bc setting that defaults to not enabling this functionality by default for existing installs(maybe we can not enable if we detect if daterange fields are in use; use some of the logic from the update hook?), and the people explicitly have to opt into. We can enable it on new installs.

?

Well why not keep the update hook that changes the views filters to the = operator? That will break some views outputs but isn't it worse to cause a WSOD? We'll need a CR either way.

I guess the benefit is we don't have to provide test coverage for the update hook

Looks like HEAD is failing right now. Will retrigger and reset RTBC once that is addressed.

Per #85 we are back to RTBC. Fails were from the brief broken HEAD situation.

I can confirm that this patch did indeed work for me (so far, so good)

I'm using Drupal 8.2.6 and applied the patch from #85

I was able to create a view which filters on end date from the DateRange field, using the "Is greater than on equal to" with a Value type of "An offset from the current time..." and a value of "0 minutes"

I will post more updates if I come across anything else of value.

This patch just touches filter/argument/sort handlers, but we're missing a use case of having separate field handlers for the start and end dates of a range. Right now the only field handler available applies to both the start and end, so there's no way to create a view that handles them separately, at least not elegantly.

So we'd add two additional field handlers, one that applies to start and one that applies to end.

Given the maturity of this issue, should I just open a new one for that? Not interesting in derailing anything.

Enhancing the DateRange should be its own issue considering how long this one has gone on, and the potential additional complexity. I would rather get this in so people have an improved experience, and then work on more functionality incrementally.

1. +++ b/core/modules/datetime/datetime.views.inc
   @@ -11,18 +11,38 @@
   +function datetime_type_field_views_data(FieldStorageConfigInterface $field_storage, $data = [], $column_name = 'value') {
      // @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);
   +  $data = (empty($data)) ? views_field_default_views_data($field_storage) : $data;
   

   Let's move this bit into the implementations and make $data required - it helps be more explicit. I'd also be tempted to make $column_name required and not have a default to. Just makes the code more explicit. If we don't do that then their @param docs need to have (optional) added to the beginning.

2. +++ b/core/modules/datetime_range/datetime_range.views.inc
   @@ -0,0 +1,19 @@
   +  \Drupal::service('module_handler')->loadInclude('datetime', 'inc', 'datetime.views');
   

   We could use \Drupal::moduleHandler() here.

3. +++ b/core/modules/datetime_range/datetime_range.views.inc
   @@ -0,0 +1,19 @@
   +  $data = datetime_type_field_views_data($field_storage);
   +  $data = datetime_type_field_views_data($field_storage, $data, 'end_value');
   

   I think it is worth adding some commentary about why this is being called twice like this and also I think it is worth adding the values to the first call.

Discussed with @xjm. Given that datetime_range will be marked as beta (either for 8.3.x or 8.4.x - that's not decided yet) - and the patch makes changes to the datetime module I think we need to write an upgrade path since this patch is only going to get into 8.4.x. Sorry I realise this is extra work.

and the patch makes changes to the datetime module I think we need to write an upgrade path since this patch is only going to get into 8.4.x.

Just so there is no confusion when we work on this, the datetime.module changes should be fully BC and are just refactors to allow reused inside datetime_range.module. So, you are talking about an upgrade path for datetime_range fields in existing views because it will be beta status (which required upgrade paths), correct?

So, it looks like we need to take the feedback from #96 (which is pretty straightforward), put back the hunks between #76 and #85, and then add the upgrade path test (we may need a new fixture for this, will track down the page that describes how to make this).

@mpdonadio yep - you've hit the nail on the head - I am talking about:

an upgrade path for datetime_range fields in existing views because it will be beta status

@alexpott for me this bug seams essential to make datetime_range beta, but lets not get hold up on that.

attached new version of the patch addressing the feedback from #96 + test for upgrade path with fixture

The test still gets 3 errors from the update path about missing schema, not sure how to fix that.

Fail Other UpdatePathTestBas 287 Drupal\system\Tests\Update\UpdatePa
Schema key
views.view.test_datetime_range_filter_values:display.default.display_options.filters.field_range_value.field_name
failed with: missing schema
Fail Other UpdatePathTestBas 287 Drupal\system\Tests\Update\UpdatePa
Schema key
views.view.test_datetime_range_filter_values:display.default.display_options.sorts.field_range_value.field_name
failed with: missing schema
Fail Other UpdatePathTestBas 287 Drupal\system\Tests\Update\UpdatePa
Schema key
views.view.test_datetime_range_filter_values:display.default.display_options.sorts.field_range_value.granularity
failed with: missing schema

not setting those 3 values in the upgrade, the filter still looks ok. new version of patch attached.

Here are interdiffs. Setting NR for last patch.

@j1n you're patches include a gzip file

@bkosborne thats the fixture, can be gzip format.

@bkosborne but i get you point, the fixture is to big, instead I rewrote it on top of drupal-8.bare.standard.php.
that was quite tidious to do, would be happy to figure out easier ways to create fixtures in the future, please let me know if you know any.

@j1n, thanks for your help with this. Update paths and tests are a pain. With your next patch, can you put the comment number in the filename (eg, 2786577-107.patch) and also post an interdiff? It's the only sane way we can review everything. Thanks.

adding interdiff for between #102 and #106
(the newly added file tests/fixtures/update/datetime_range-filter-values.php is not included in diff)

@j1n I guess @mpdonadio means an interdiff between the patches, and not just a diff of the .patch files.
It may looks similar, but with a diff you details like the file been changed by the patch.

Just applied #106 to 8.3.0-rc2 and was able to successfully get a view to filter on a DateRange field using a relative date ( i.e. '>= today' ) on both a start_date and an end_date. I'm a little late to the party so I'm not sure about all the ramifications of the patch but I it looks like it does what it says on the tin.

Thanks to all who are working on this.

Here is the interdiff between #85 (the last RTBC patch) and the current green one (#106) for review.

I haven't manually tested the patch yet and additionally I've never written/reviewed an upgrade path test so I can't say anything about it.

What looks still missing is one of the remaining tasks "map current 'string' view plugin values/operators to new 'datetime' plugin on the update path using regular_expression operator" flagged from #68. If this is still a required task than I'm presuming we need to add it to the patch and test it on the upgrade path test.

With the last patch (#106), when I expose a date_range filter, the exposed filter doesn't show at all in the view.

I have been successfully using the patch in #106 for a few weeks now.

I installed patch #106 and was able to create the filter I wanted - show only entities with date field end value of today or greater:

The view accepted this setting and previewed fine, but threw errors:

and crashed when I tried to save with the following error:

InvalidArgumentException: The configuration property display.month_p.display_options.filters.field_when_end_value.value.min doesn't exist. in Drupal\Core\Config\Schema\ArrayElement->get() (line 74 of /data/mcic/docroot/core/lib/Drupal/Core/Config/Schema/ArrayElement.php).

Okay, the string errors were to do with a *previously configured* exposed date filter, so at the moment this patch is a breaking change, probably more suited to 8.x-4.x unless that can be resolved.

The InvalidArgumentException only happens for end_value filters. I'll try to figure out what's going on but I'm getting the edge of my ability to understand these modules for now.

Never mind the fatal error, I didn't read back far enough. The patch for core/modules/options/tests/src/Kernel/Views/OptionsListFilterTest.php and hunk #5 of core/modules/views/src/Tests/Handler/FilterDateTest.php in #2369119: Fatal error when trying to save a View with grouped filters using other than string values fixed this one for me, though the rest of the patch failed.

The other errors might still be an issue requiring some notification on update.

Okay, the string errors were to do with a *previously configured* exposed date filter, so at the moment this patch is a breaking change, probably more suited to 8.x-4.x unless that can be resolved.

As this is still an experimental module in alpha state, this BC-breaks are OK. The update path should also take care of views that already exist, though.

I use this patch #106 happily with an handful of date range filters. Did not test upgrade path though.

I am testing this issue, #DrupalConBaltimore2017.

I am testing this issue, #DrupalConBaltimore2017.

Tested #106 at #baltimore2017 with all combinations of filters with start and end dates using Date-only datetime_range fields. All worked correctly.

#122, can you test out the update path? In 8.4.x HEAD, make views w/ daterange fields and make sure they work. Then apply patch, run updates, and make sure the views still work?

adding tags

As #123 requested, I tested the update path using both DateRange (date-only) and DateRange (date+time) values and it all looks good. Some of the patterns that it may import could be non-sensical / non-working depending on what was being searched for, but I think that it works sufficiently for an alpha/beta update function. Screenshots attached.

I only tested the equality and starts with/contains operators as most of the other operators would not have worked in the pre-patch version.

No notices appeared in the Watchdog logs from any related operations.

Looks RTBC to me.

You rock. Seriously.

Sorry to be late. I made this patch few days ago but I can't get the upgrade path test running on my local machine so I couldn't add test coverage to it. That's why the delay.
(I know I missed the opportunity for it to be reviewed and tested during DrupalCon Baltimore, and I beg pardon for this.)

The patch introduces mapping between string to numeric plugin operators/values, and specifically:

• = maps to =, value is unchanged
• != maps to !=, value is unchanged
• not maps to !=, value is unchanged
• starts maps to regular_expression, value is prefixed with '^'
• ends maps to regular_expression, value is suffixed with '$'
• any other operator (contains, word, allwords, not_starts, not_ends, shorterthan, longerthan, regular_expression) maps to regular_expression, value is unchanged

It should be green as it doesn't really change that match for classic scenarios. Manual test works.

Removing the tag Needs upgrade path as with this change we covered all requirement in IS.

The mappings described in #127 are working properly as shown in the attached screenshots (see patch 127 working.png), with the following caveats:

1. The regex's, after created by the UPDB, can't be updated because the Regular Expression filter UI is not allowing non-date values to be entered for the regex through the UI (though the values put in the fields by the UPDB work correctly. This may be a general Views or Date bug not specific to this module. (See patch 127 regex ui.png)
2. The UPDB process changes the EMPTY and NOT EMPTY conditions to a REGEX with an empty value field, which causes a SQL error (see patch 127 db error.png). This can be remedied by deleting the blank filters upon which the view will work properly. The EMPTY and NOT EMPTY states are not mentioned in the #127 post and should be added as these are still valid states that could be used on daterange fields.

Patch to add the EMPTY and NOT EMPTY operators and set a value for any blank regex.

Tested the #129 patch, no SQL error from EMPTY / NOT EMPTY and converts to proper operators.

1. +++ b/core/modules/datetime_range/src/Tests/Views/DateRangeHandlerTestBase.php
   @@ -0,0 +1,24 @@
   +abstract class DateRangeHandlerTestBase extends DateTimeHandlerTestBase {
   

   Why are we creating an abstract class that is only extended by another class. Could we just move the $modules and $field_type to the FilterDateTest class?

2. +++ b/core/modules/datetime_range/tests/fixtures/update/datetime_range-filter-values.php
   @@ -0,0 +1,322 @@
   +// @codingStandardsIgnoreFile
   

   We can use https://github.com/thomasbachem/php-short-array-syntax-converter to convert the arrays to [] here

3. Do we want to implement a hook_ENTITY_TYPE_presave for contrib fields too? See #2870724: Define and document best practices for configuration entity updates/bc layers|

Need to review this patch, but...

#131-1, we are going to be building on this patch so I would rather have the abstract class now rather than adding it later.

#131-2, a lot of our other test fixtures have this, so I am not sure if it is a big deal. Will deter to committers.

#131-3, hmmm. Pinged @Lendude for an opinion. Since this is an experimental (and this issue is lingering), I am not sure what is really best here.

And based on #129, do we need to additional update path testing?

@mpdonadio I think that if we care about existing views in active config, we should also care about existing views in contrib, so yes, I think we need hook_ENTITY_TYPE_presave here. If we use the pattern that @catch describes in #2870724: Define and document best practices for configuration entity updates/bc layers the change doesn't need to be too big.
Move all the existing update logic to hook_view_presave and just do $view->save() in the update hook when appropriate, that will then run the presave logic. So existing update tests should still work and already provide the needed test coverage.

On another note though:

+++ b/core/modules/datetime/datetime.views.inc
index 0f068ab..73b453f 100644
--- a/core/modules/datetime/src/Tests/Views/DateTimeHandlerTestBase.php

--- a/core/modules/datetime/src/Tests/Views/DateTimeHandlerTestBase.php
+++ b/core/modules/datetime/src/Tests/Views/DateTimeHandlerTestBase.php

+++ b/core/modules/datetime_range/src/Tests/Views/DateRangeHandlerTestBase.php
@@ -0,0 +1,24 @@
+namespace Drupal\datetime_range\Tests\Views;
...
+abstract class DateRangeHandlerTestBase extends DateTimeHandlerTestBase {

+++ b/core/modules/datetime_range/src/Tests/Views/FilterDateTest.php
@@ -0,0 +1,136 @@
+namespace Drupal\datetime_range\Tests\Views;
...
+class FilterDateTest extends DateRangeHandlerTestBase {

This is all Webtestbase based testing and this should really be moved to Browsertestbase. This is building on deprecated classes since #2863267: Convert web tests of views and #2780063: Convert web tests to browser tests for datetime and datetime_range modules landed.

Presently not in the place to roll it into a patch (and unsure if it meets any sort of standard), but I was running into issues with daterange fields in Views. The patch from #106 was a big help in 8.3.x for a client site, but the "current date" default argument for Contextual Filters wasn't working on account of a missing trait.

The attached file (got renamed, but it's a tarball) has an argument_default plugin that takes a textual option for the format used for the request time default argument. This works for my very narrow use case, but probably needs a lot of work to be a generalized solution. I also had to add a `use` statement that didn't feel quite right. The format could probably be inherited anyway.

#134 your issue sounds like a datetime views plugin generic issue and not related to this patch, which just makes those plugins available to datetime_range fields. I can't find anything in the issue queue RE current date argument and errors related to missing trait, so try to collect more info and open an issue.

I've updated the IS remaining tasks with requirements from #132-#133

I'll try to tackle the remaining requirements one by one.

This patch:

• Fixes some coding standards
• Add tests coverage for operators/values mapping, testing the case '=' and the default handler with an fixture value blank. So we make sure the switch works and we cover the potential errors mentioned in #128 and fixed in #129.

Patch still uses Webtestbase tests. I'll upgrade them in the next batch of works.

Setting Needs review only to trigger testbot. I'll switch back to Needs work after the results, but it would be good if anybody can review at least the upgrade path test coverage.

The failure is a known random problem, I've reported it in #2859704: Intermittent segfaults on DrupalCI (some "did not complete due to a fatal error" with no additional info).

Leaving Needs work back to tackle the remaining requirements.

This patch covers "take care of contrib views too. Move all the existing update logic to hook_view_presave and just do $view->save() in the update hook when appropriate as described in #133" requirement.

I followed the examples on #2870724: Define and document best practices for configuration entity updates/bc layers and related issues and it wasn't an easy change. All those update paths process views from \Drupal::entityTypeManager()->getStorage('view')->loadMultiple() while in here we processed configuration entities from \Drupal::configFactory()->listAll('views.view.').
Then to be consistent I've made the following changes:

• Update path has been moved to .post_update.php. .install file has been deleted.
• Code has been migrated from processing configuration entities to processing views entities. The logic tries to understand if a BC Layer check is needed and if it is just runs $view->save(), delegating the actual updates to the hook_view_presave().
• A hook_view_presave() has been added to the .module file with the code to migrate datetime_range plugins to the right plugin_id/operator/value.

Tests are green locally.

PS: I do understand what #2870724: Define and document best practices for configuration entity updates/bc layers is trying to do, however won't keep adding BC layers to pre-save hooks affect performances?

Tests still need to be converted to Browsertestbase, so setting status back to Needs work.
However as #140 introduced quite few changes it would be good to have some feedback (or even a thumbs up).

This patch converts Webtestbase tests to Browsertestbase, and my understanding is covers all feedbacks from #131 and #133 (leaving only #131.2 open as mentioned in #132).

+1 for #129

hiding the .gz from #134 and older patches to de-clutter

Sorry, cross posted my last comment.

+1 for #142

Also looks like I exposed a bug in issue file attachments.
oy vey.

@aaronbauman not sure what happened. You've hidden #142 too.
However here a new patch with just a small CS fix.

tangential: #2877025: Files not displayed in file attachment list after cross-posting comments