Follow-up to #2161337: Add a Date Range field type with support for end date

Problem/Motivation

datetime_field_views_data() performs some nice improvements for the Views integration for datetime fields, but has not yet been updated to cover daterange fields, so daterange fields only receive the integration provided by the views_field_default_views_data() fallback. This means that for example, the year-only, month-only, etc. formats aren't available as arguments for daterange fields like they are for datetime fields.

Proposed resolution

The proposed resolution is to allow datetime views plugin to be used for datetime_range fields too. Together with this we have to make sure: extending/reusing the current datetime implementation becomes an easy task for developers, any existing views making use of datetime_range fields is correctly updated, default views shipped with contrib modules using old plugins are automatically migrated on save, etc.
In order to achieve that the solution should include the following tasks:

  • Extending the datetime fields Views integration in order to allow any datetime-based fields to make use of it. #4
  • Letting daterange fields use the same Views integration capabilities that datetime fields have, using the work from previous point. #4
  • Providing an upgrade path for views having old datetime_range fields string/standard views plugins to new plugins types. #24 ~ #139
  • Updating contrib modules provided views having old datetime_range fields string/standard to new plugins types. #133 ~ #142

Remaining tasks

  • map current 'string' view plugin values/operators to new 'datetime' plugin on the update path using regular_expression operator
  • 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
  • update path test for #127 and #129 changes
  • Convert Webtestbase based testing to Browsertestbase
  • Deciding if we need a stronger message alerting the user to manually review the views being converted (and/or may be a Change record?). Change Record added.
  • Update the issue summary:
    • User interface changes: the IS should mention this issue is neither adding nor removing UI elements, instead we are moving from string/standard views plugins UI to datetime ones. A screenshot with "before - after" should help.
    • API changes: "Extending the datetime fields Views integration in order to allow any datetime-based fields to make use of it" solution task introduce a new API helper for datetime-based fields who want to integrate datetime views plugins easily.
    • Data model: we are not changing any data model. However existing and contrib modules shipped views may be altered. Developers and site builders should be aware of that.

  • UI/API/Data Model changes above need a place in the CR - together with a good clean up - as flagged by #192.7
  • Upgrade test path is still using old simpletest. We should move it to use Drupal\FunctionalTests\Update\UpdatePathTestBase

User interface changes

This issue doesn't introduce new UI elements, but it converts Views filter and sort plugins from string/standard formats to datetime based ones. The change is automatic, site-builders and developers don't need to do any action.
See the example below of how the UI will change for a Views filter plugin against a datetime_range field:
Before:
before
After:
before

API changes

datetime_type_field_views_data_helper - new API helper to integrate datetime views plugins easily for any datetime-based fields. Modules defying datetime-based fields can now benefit of datetime Views plugins easily.

Example usage in core/modules/datetime_range/datetime_range.views.inc:

/**
 * Implements hook_field_views_data().
 */
function datetime_range_field_views_data(FieldStorageConfigInterface $field_storage) {
  \Drupal::moduleHandler()->loadInclude('datetime', 'inc', 'datetime.views');

  // Get datetime field data for value and end_value.
  $data = datetime_type_field_views_data_helper($field_storage, [], 'value');
  $data = datetime_type_field_views_data_helper($field_storage, $data, 'end_value');

  return $data;
}

Data model changes

There are no Data model changes. However existing and contrib modules' shipped Views - using datetime_range fields/plugins - may be altered. See update message about the ids of changed views like (view id is datetime_test):
update info

The update will map existing datetime_range views string plugins operators/values according with this list:

  • = 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 if not empty otherwise *. is used

Example, filter Starts with with 'value' will be converted into Regualr expression with '^value';

CommentFileSizeAuthor
#273 2786577-8.4.4.patch45.13 KBjcontreras
#270 interdiff-264-270.txt601 bytesvaplas
#270 2786577-270.patch47.81 KBvaplas
#268 2786577-8.4.3.patch42.81 KBjibran
#266 Evaluate Drupal projects online simplytest me.png121.8 KBpvsureshmca
#264 interdiff-255-264.txt646 bytesvaplas
#264 the_views_integration-2786577-264.patch47.86 KBvaplas
#255 the_views_integration-2786577-255.patch47.84 KBgambry
#255 interdiff-250-255.txt6.23 KBgambry
#250 interdiff-248-250.txt1.15 KBvaplas
#250 the_views_integration-2786577-250.patch44.22 KBvaplas
#248 the_views_integration-2786577-248.patch43.97 KBvaplas
#248 interdiff-241-248.txt1.29 KBvaplas
#241 interdiff-patches-229-241.txt5.91 KBvaplas
#241 the_views_integration-2786577-241.patch43.94 KBvaplas
#229 datetime_range-views-filter-titles.png16.95 KBgambry
#229 the_views_integration-2786577-229.patch43.83 KBgambry
#229 interdiff-202-229.txt6.83 KBgambry
#207 datetime_range_update_progress.png7.05 KBvaplas
#207 datetime_range_update_notice.png10.78 KBvaplas
#205 datetime_range_update_info.png13.79 KBvaplas
#204 ui_filter_after.png13.23 KBvaplas
#204 ui_filter_before.png13.81 KBvaplas
#202 interdiff-196-202.txt3.8 KBvaplas
#202 the_views_integration-2786577-202.patch44.16 KBvaplas
#196 the_views_integration-2786577-196.patch44.16 KBgambry
#196 interdiff-177-196.txt3.98 KBgambry
#184 2786577-184-8-3-x-test-do-not-test.patch12.99 KBartis
#183 2786577-183-8-3-x-test-do-not-test.patch12.16 KBartis
#177 2786577-177.patch43.69 KBjhedstrom
#177 interdiff-2786577-167-177.txt5.02 KBjhedstrom
#167 improve_the_views-2786577-167.patch43.23 KBgambry
#167 interdiff-159-167.txt718 bytesgambry
#165 improve_the_views-2786577-165.patch42.27 KBsebi
#159 improve_the_views-2786577-159.patch43.23 KBgambry
#159 interdiff-157-159.txt20.78 KBgambry
#157 interdiff-151-157.txt1.45 KBgambry
#157 improve_the_views-2786577-157.patch40.48 KBgambry
#151 improve_the_views-2786577-151.patch40.32 KBgambry
#151 interdiff-145-151.txt2.46 KBgambry
#145 inderdiff-142-145.txt411 bytesgambry
#145 improve_the_views-2786577-145.patch42.32 KBgambry
#134 improve-the-views-2786577-argument_default.tar_.gz740 bytesao5357
#130 patch 129 working.png274.54 KBdiamondsea
#129 interdiff-2786577-127-129.txt933 bytesdiamondsea
#129 improve_the_views-2786577-129.patch35.92 KBdiamondsea
#128 patch 127 regex ui.png260.94 KBdiamondsea
#128 patch 127 db error.png448.38 KBdiamondsea
#128 patch 127 working.png244.13 KBdiamondsea
#128 patch 127 updatedb.png325.6 KBdiamondsea
#127 interdif-106-127.txt1.87 KBgambry
#127 improve_the_views-2786577-127.patch35.54 KBgambry
#125 daterange5-view-post-update-working.png179.97 KBdiamondsea
#125 daterange4-view-post-update-nonsensical.png224.1 KBdiamondsea
#125 daterange3-view-post-update.png204.57 KBdiamondsea
#125 daterange2-updb.png213.44 KBdiamondsea
#125 daterange1-view-pre-update.png208.7 KBdiamondsea
#115 error.JPG74.77 KBDarvanen
#115 settings.JPG44.33 KBDarvanen
#111 interdiff-85-106.txt26.32 KBmpdonadio
#108 interdiff-102-106.txt1.01 KBj1n
#106 0001-Issue-2786577-Improve-the-Views-integration-for-Date.patch36.08 KBj1n
#103 interdiff-100-102.txt1.25 KBmpdonadio
#103 interdiff-85-100.txt73.77 KBmpdonadio
#102 0001-Issue-2786577-Improve-the-Views-integration-for-Date.patch83.78 KBj1n
#100 0001-Issue-2786577-Improve-the-Views-integration-for-Date.patch84.18 KBj1n
#85 0001-Issue-2786577-Improve-the-Views-integration-for-Date.patch10.63 KBj1n
#4 improve_the_views-2786577-4.patch3.95 KBgambry
#10 interdiff.txt631 bytesgambry
#10 improve_the_views-2786577-10.patch3.97 KBgambry
#24 improve_the_views-2786577-24.patch8.45 KBgambry
#24 interdiff.txt4.48 KBgambry
#28 improve_the_views-2786577-28.patch14.84 KBgambry
#28 interdiff.txt6.39 KBgambry
#39 improve_the_views-2786577-39.patch14.9 KBgambry
#39 interdiff.txt1.92 KBgambry
#56 ambiguous_argument_titles.png54.72 KBbkosborne
#57 improve_the_views_integration_for_daterange_fields-2786577-57.patch14.92 KBadriancid
#60 improve_the_views_integration_for_daterange_fields-2786577-60.patch15 KBadriancid
#60 interdiff.txt677 bytesadriancid
#60 now.png205.24 KBadriancid
#62 improve_the_views_integration_for_daterange_fields-2786577-62.patch15.08 KBadriancid
#62 interdiff.txt834 bytesadriancid
#71 improve_the_views-2786577-71.patch14.93 KBgambry
#71 interdiff.txt1.58 KBgambry
#76 improve_the_views-2786577-76-core.patch14.98 KBjibran
#76 improve_the_views-2786577-76.patch14.93 KBjibran
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

effulgentsia created an issue. See original summary.

effulgentsia’s picture

Status: Needs review » Active
xjm’s picture

gambry’s picture

Version: 8.2.x-dev » 8.3.x-dev
Status: Active » Needs work
FileSize
3.95 KB

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.

effulgentsia’s picture

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

datetime_range module is experimental, so feature improvements to it can go into 8.2. The #4 patch changes datetime code as well, so we need to be careful there, but from what I can see so far, does it in a way that would be acceptable for a patch release (e.g., 8.2.1).

mpdonadio’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
+++ 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.

dawehner’s picture

Wow that's super clean.

mpdonadio’s picture

@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?

dawehner’s picture

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.

gambry’s picture

Assigned: Unassigned » gambry
FileSize
631 bytes
3.97 KB

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.

dkre’s picture

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.

gambry’s picture

@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.

dkre’s picture

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.

mpdonadio’s picture

#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.

dkre’s picture

@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.

mpdonadio’s picture

Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: +Needs upgrade path

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

dkre’s picture

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.

gambry’s picture

Assigned: gambry » Unassigned

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?

mpdonadio’s picture

#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.

mpdonadio’s picture

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.

gambry’s picture

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.

dkre’s picture

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: {  }
gambry’s picture

Assigned: Unassigned » gambry
Issue tags: +Dublin2016
gambry’s picture

Status: Needs work » Needs review
FileSize
8.45 KB
4.48 KB

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.
jhedstrom’s picture

  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?

gambry’s picture

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'])." ?

jhedstrom’s picture

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.

gambry’s picture

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.

gambry’s picture

Assigned: gambry » Unassigned
Michelle’s picture

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.)

mpdonadio’s picture

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

Michelle’s picture

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

dkre’s picture

#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.

Shawn DeArmond’s picture

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.
mpdonadio’s picture

#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.

ckaotik’s picture

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.

ckaotik’s picture

Status: Needs review » Needs work
gambry’s picture

Assigned: Unassigned » gambry

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.

gambry’s picture

Assigned: gambry » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs upgrade path
FileSize
14.9 KB
1.92 KB
caspervoogt’s picture

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.

gambry’s picture

@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.

caspervoogt’s picture

@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).

TwoD’s picture

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.)

gambry’s picture

@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?

TwoD’s picture

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
  5. 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.

mpdonadio’s picture

#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: 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.

mpdonadio’s picture

Status: Needs review » Needs work

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

MrPeanut’s picture

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.

mpdonadio’s picture

#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?

MrPeanut’s picture

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

mpdonadio’s picture

#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.

tacituseu’s picture

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

bmunslow’s picture

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.

adriancid’s picture

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

gambry’s picture

bkosborne’s picture

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".

adriancid’s picture

@bkosborne I made another patch that solve the problem

adriancid’s picture

Status: Needs work » Needs review
mpdonadio’s picture

Status: Needs review » Needs work

@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.

adriancid’s picture

Status: Needs work » Needs review
FileSize
15 KB
677 bytes
205.24 KB

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

I just changed a line

Output:

output

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

mpdonadio’s picture

Status: Needs review » Needs work
+++ 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?
adriancid’s picture

@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'

bkosborne’s picture

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.

mpdonadio’s picture

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.

mpdonadio’s picture

Status: Needs review » Needs work

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.

adriancid’s picture

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

bkosborne’s picture

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?

gambry’s picture

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?

gambry’s picture

Assigned: Unassigned » gambry
gambry’s picture

Assigned: gambry » Unassigned
FileSize
14.93 KB
1.58 KB

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?

gambry’s picture

gambry’s picture

Issue summary: View changes

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.

gambry’s picture

Issue summary: View changes
Status: Needs work » Postponed

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

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

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

jibran’s picture

Title: Improve the Views integration for DateRange fields » [PP-2] Improve the Views integration for DateRange fields
gunwald’s picture

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.

gambry’s picture

Title: [PP-2] Improve the Views integration for DateRange fields » Improve the Views integration for DateRange fields
Status: Needs review » Needs work

@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.

gunwald’s picture

@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?

gambry’s picture

Yes @gunwald, it should.

j1n’s picture

Status: Needs work » Reviewed & tested by the community

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?

mpdonadio’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs manual testing, +Needs upgrade path tests

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.

mpdonadio’s picture

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.

?

j1n’s picture

updated patch removing the update hook.

gambry’s picture

Assigned: Unassigned » gambry

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.

gambry’s picture

Assigned: gambry » Unassigned

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!

mpdonadio’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs release manager review

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.

?

bkosborne’s picture

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

mpdonadio’s picture

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

mpdonadio’s picture

Status: Needs work » Reviewed & tested by the community

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

B.Friddy’s picture

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.

bkosborne’s picture

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.

mpdonadio’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

alexpott’s picture

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.

mpdonadio’s picture

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).

alexpott’s picture

@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

j1n’s picture

@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

j1n’s picture

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

mpdonadio’s picture

Here are interdiffs. Setting NR for last patch.

bkosborne’s picture

@j1n you're patches include a gzip file

j1n’s picture

@bkosborne thats the fixture, can be gzip format.

j1n’s picture

@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.

mpdonadio’s picture

@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.

j1n’s picture

FileSize
1.01 KB

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

gambry’s picture

@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.

Dave Kopecek’s picture

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.

mpdonadio’s picture

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

gambry’s picture

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.

Igor Bruneli’s picture

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

kmonty’s picture

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

Darvanen’s picture

Status: Needs review » Needs work
FileSize
44.33 KB
74.77 KB

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:

settings

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

error

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).

Darvanen’s picture

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.

Darvanen’s picture

Status: Needs work » Needs review

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.

mpdonadio’s picture

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.

axel.rutz’s picture

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

abiyub’s picture

I am testing this issue, #DrupalConBaltimore2017.

diamondsea’s picture

I am testing this issue, #DrupalConBaltimore2017.

diamondsea’s picture

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

mpdonadio’s picture

#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?

diamondsea’s picture

Issue tags: +Baltimore2017

adding tags

diamondsea’s picture

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.

mpdonadio’s picture

You rock. Seriously.

gambry’s picture

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.

diamondsea’s picture

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.
diamondsea’s picture

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

diamondsea’s picture

FileSize
274.54 KB

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

dagmar’s picture

  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|
mpdonadio’s picture

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?

Lendude’s picture

Status: Needs review » Needs work

@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.

ao5357’s picture

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.

gambry’s picture

Issue summary: View changes

#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

gambry’s picture

FileSize
14.71 KB
37.12 KB

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.

gambry’s picture

Status: Needs work » Needs review

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.

gambry’s picture

Issue summary: View changes

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.

gambry’s picture

Status: Needs work » Needs review
FileSize
13.54 KB
39.83 KB

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?

gambry’s picture

Status: Needs review » Needs work

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).

gambry’s picture

Status: Needs work » Needs review
FileSize
5.47 KB
42.34 KB

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).

aaronbauman’s picture

+1 for #129

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

aaronbauman’s picture

Issue summary: View changes
Status: Needs work » Needs review

Sorry, cross posted my last comment.

+1 for #142

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

gambry’s picture

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

aaronbauman’s picture

mpdonadio’s picture

+++ b/core/modules/datetime_range/datetime_range.module
@@ -6,6 +6,8 @@
+use Drupal\views\ViewEntityInterface;
+use Drupal\views\Views;
 

Nit, should be alphabetical.

I think I am OK with this, but would like @jhedstrom and/or @Lendude to look at the update path and update path tests, too.

The new update path can also use a round of manual testing:

- Make views in 8.4.x
- Apply patch
- Run update.php
- Make sure views still work

I also pinged @xjm to double check that daterange is going beta soon, which is the main reason for this update path.

Lendude’s picture

@gambry really nice work on this!

The update path and update path test look good to me.

+++ b/core/modules/datetime_range/src/Tests/Views/DateRangeHandlerTestBase.php
@@ -0,0 +1,29 @@
+/**
+ * Base class for testing datetime handlers.
+ *
+ * @deprecated in Drupal 8.4.0 and will be removed before Drupal 9.0.0.
+ *   Use \Drupal\Tests\BrowserTestBase.
+ */
+abstract class DateRangeHandlerTestBase extends DateTimeHandlerTestBase {

Why is this being added? It's a deprecated base class that is never used. It feels like this can just be left out or am I missing something?

gambry’s picture

Issue summary: View changes
Status: Needs review » Needs work

@mpdonadio @Lendude thanks for the feedbacks!

Why is this being added?

To be honest my concern was developers using this patch - or contrib suggesting to use it - and then extending that class.
I understand if someone uses an experimental module and patches it he/she is on his/her own, but in doubt I opted for keeping it as @deprecated.

But probably that just me being too gentle.

And I'm assuming changes on core/modules/datetime/src/Tests/Views/DateTimeHandlerTestBase.php can be removed as well, as the only reason for those is the tests on DateRangeHandlerTestBase().

Setting as Needs work to address #147 and #148.
Also striking-through completed IS Remaining tasks.

mpdonadio’s picture

Confirmed with @xjm that we need the update path, regardless of alpha/beta status. Will try to do some manual testing tonight.

gambry’s picture

Status: Needs work » Needs review
FileSize
2.46 KB
40.32 KB

This patch:

  • Sorts core/modules/datetime_range/datetime_range.module use statements alphabetically. [#147]
  • Removes core/modules/datetime_range/src/Tests/Views/DateRangeHandlerTestBase.php file and changes to core/modules/datetime/src/Tests/Views/DateTimeHandlerTestBase.php from the previous patch(es) as tests are now deprecated in favour of BrowserTests. [#148, #149]
ibuildit’s picture

151 doesn't work for me.

patching file core/modules/datetime/datetime.views.inc
can't find file to patch at input line 77
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|diff --git a/core/modules/datetime/tests/src/Kernel/Views/DateTimeHandlerTestBase.php b/core/modules/datetime/tests/src/Kernel/Views/DateTimeHandlerTestBase.php
|index f0c9c78..e102c2f 100644
|--- a/core/modules/datetime/tests/src/Kernel/Views/DateTimeHandlerTestBase.php
|+++ b/core/modules/datetime/tests/src/Kernel/Views/DateTimeHandlerTestBase.php

mpdonadio’s picture

#152, the patch is against 8.4.x and still applies cleanly. This will likely not get a backport to 8.3.x.

mpdonadio’s picture

Status: Needs review » Reviewed & tested by the community

I think the update path could potentially use some more manual testing, but I did some again tonight, and I think the patch in #151 is good to go.

tstoeckler’s picture

Patch looks pretty sweet, nice solution!! Two minor things, but does warrant a "needs work".

  1. +++ b/core/modules/datetime/datetime.views.inc
    @@ -33,11 +53,12 @@ function datetime_field_views_data(FieldStorageConfigInterface $field_storage) {
    +      $column_name_text = $column_name !== 'value' ? ':' . $column_name : '';
    

    I think this should be something like $column_name === $field_storage->getMainPropertyName() instead of explicitly checking for 'value'.

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

    This is not properly translatable, it should be something like t('@label @column (@argument)') with arguments passed appropriately.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

NW per #155.

Also did a quick review:

  1. +++ b/core/modules/datetime_range/src/Tests/Update/DatetimeRangeViewUpdateTest.php
    @@ -0,0 +1,55 @@
    +  public function testViewsPostUpdateDateRangeFilterValues() {
    +
    +    $this->runUpdates();
    +
    +    // Load and initialize our test view.
    +    $view = View::load('test_datetime_range_filter_values');
    +    $data = $view->toArray();
    

    Shouldn't this also test the "before" values?

  2. +++ b/core/modules/datetime_range/tests/fixtures/update/datetime_range-filter-values.php
    @@ -0,0 +1,322 @@
    +  'data' => 'a:16:{s:4:"uuid";s:36:"87dc4221-8d56-4112-8a7f-7a855ac35d08";s:8:"langcode";[…]
    ...
    +  'data' => 'a:16:{s:4:"uuid";s:36:"2190ad8c-39dd-4eb1-b189-1bfc0c244a40";s:8:"langcode";s:2:"[…]
    ...
    +  'data' => 'a:13:{s:4:"uuid";s:36:"d20760b6-7cc4-4844-ae04-96da7225a46f";s:8:"[…]
    

    These would be infinitely more understandable if they actually lived in YML files that you then bring in with file_get_contents(). That's what core/modules/field/tests/fixtures/update/drupal-8.views_entity_reference_plugins-2429191.php and core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php do.

gambry’s picture

Status: Needs work » Needs review
FileSize
40.48 KB
1.45 KB

This patch addresses feedbacks from #155, making use of $field_storage->getMainPropertyName() instead of hardcolded 'value' where possible.

#156.1 'before' values are the ones coming straight from the fixtures, so I thought it was trivial to test those as if test fails the issue is in the import and not in this issue code? But I maybe wrong so please advice best practices.
#156.2 yes, and making changes to improve testing coverage is really hard with these being serialised items. I agree to convert them to YAML.

My local system always fails when running update path tests, so I'm not able to tackle #156 today.
[[The error is always "UpdatePathTestBase 269 Drupal\system\Tests\Update\UpdatePathTestBase - The disable_blocks_with_missing_contexts() update function from the block module did not run.", on docker with php 7.0, maybe some angel will know the reason, I can't find anything in the issue queue.]]

Setting Needs Review to trigger automated tests.

dagmar’s picture

My local system always fails when running update path tests, so I'm not able to tackle #156 today.

If you are having issues with upgrade tests, and you are using php-fpm, you may need to apply also: #2851111: fastcgi_finish_request and shutdown functions (e.g. batch) do not play nicely together

gambry’s picture

This patch addresses remaining feedbacks from #156.
I've also updated the CR according with the new requirements of having an upgrade path released due the module becoming beta or stable anytime soon ( this hope requires a smile :D ), new copy should be included in the review process.

CR title is still "Views integration for experimental Datetime Range module" as I'm not sure if we can remove the 'experimental' word yet.

@dagmar thank you for your help, although I'm sure my issue is totally related to the one you linked, the solution suggested in there by you and other didn't help. I'm assuming running the docker stack nginx + php + mariadb introduced additional point of failures on running update path tests. I'll investigate when possible.

johnpicozzi’s picture

Does an updated 8.3.x patch exist? I'm using the patch from #106 above. however it will not allow me to save a date range contextual filter.

gambry’s picture

@johnpicozzi can you provide more info?
Using patch on #159 let me save Contextual Filters. Patch should apply to 8.3.x (although I haven't tried).

However the code in this issue re-use the Date module views integration, so if there is an issue is likely to be in there. Can you list the steps to reproduce your problems, trying them against a Date field too?

mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Reminder to self for final review.

johnpicozzi’s picture

@gambry

When trying to apply the patch in #159 via composer it says "Could not apply patch! Skipping."

In my view I am not using a date field. I am using a date range field and if I try to add a contextual filter for YYYYMM and set a default value it will not save the contextual filter. So the filter is created but the configuration of the field is defaulted, not what I set it to.

Hope this helps! Let me know if you have any other questions.

mpdonadio’s picture

Assigned: mpdonadio » jhedstrom

Reviewed patch again, and +1 to RTBC, but I want @jhedstrom to look at it one more time since this is a complicated update.

sebi’s picture

I modified patch #159 so this would work with my 8.3.x install for those that need it.

I had to remove the reference to DateTimeHandlerTestBase.php in order to get it to patch properly.

gambry’s picture

Just a tiny Coding Standards change.

+++ b/core/modules/datetime_range/datetime_range.views.inc
@@ -0,0 +1,21 @@
+  // Get datetime field data for value and end_value

Missing period.

johnpicozzi’s picture

@sebi Patch #165 applies cleanly to 8.3.x. However, I'm still having the issue with the contextual filters not saving.

One up side is that I know can set a regular filter with a greater than or equal to of today, couldn't do that before.

mpdonadio’s picture

Right now we are developing this against 8.4.x, and that is what the automated testing is against, and what we are trying to get to RTBC (hopefully so we can land it in 8.4.0). If this gets into 8.4.x, we can discuss a backport.

In the meantime, if anybody posts an 8.3.x patch to help people before 8.4.0, can you add '-8-3-x-test-do-not-test' to the patch name so we don't disturb the issue status and make it clean which patch needs to be reviewed.

Thanks.

jhedstrom’s picture

Assigned: jhedstrom » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing

I've given this another review, and I think it's looking great!

Since this now has tests, I'm removing the manual testing tag.

mpdonadio’s picture

Status: Reviewed & tested by the community » Needs review
Related issues: +#2895544: Datetime module missing view plugin config schema

#2895544: Datetime module missing view plugin config schema just popped up. Think we may need to do the same for daterange.

gambry’s picture

As datetime_range doesn't define its own views plugins I was wondering (and TBH worrying) why we would need schema definition in here.
Below the chat with @berdir in IRC:

berdir, RE https://www.drupal.org/node/2895544 I'm straggling to find a good documentation about why we are doing this (I need to update the datetime_range views plugin config schema too), and the issue doesn't help. Any clue?
[...]
gambry: not sure what your question is exactly :) every configuration needs to have schema, so data types are enforced, things can be translated and validated
gambry: but datetime_range does not have views plugins
berdir, that's exactly what I'm trying to understand. In https://www.drupal.org/node/2786577 we make use of datetime plugins, so does it require a config schema definition too? I understand the requirement of a config schema, but not sure about datetime_range views plugins scenario.
gambry: config schema needs to be defined for views plugins. datetime_range does not add its own plugins, it just uses those from datetime
gambry: we actually discovered those schema issues while adding that patch to a project, it is working fine for us now.

I guess this can go back to RTBC?

mpdonadio’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, I think this is OK.

larowlan’s picture

Status: Reviewed & tested by the community » Needs review
  1. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,126 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    +function datetime_range_view_presave(ViewEntityInterface $view) {
    

    In other cases where we've done this (e.g. #2851293: dblog is using the wrong views field, filter and relationships definitions) we also added an @deprecated tag to the function and used trigger_error in the function body - any reason why we're not doing that here? See dblog_view_presave in HEAD.

  2. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,126 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    +        if ($filter['plugin_id'] == 'string') {
    ...
    +          if ($field_configuration->get('type') == 'daterange') {
    ...
    +        if ($sort['plugin_id'] == 'standard') {
    ...
    +          if ($field_configuration->get('type') == 'daterange') {
    
    +++ b/core/modules/datetime_range/datetime_range.post_update.php
    @@ -5,9 +5,88 @@
    +          if ($filter['plugin_id'] == 'string') {
    ...
    +            if ($field_configuration->get('type') == 'daterange') {
    ...
    +          if ($sort['plugin_id'] == 'standard') {
    ...
    +            if ($field_configuration->get('type') == 'daterange') {
    

    nit ===

  3. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,126 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    +              case '=':
    +                $operator = '=';
    ...
    +              case 'empty':
    +                $operator = 'empty';
    ...
    +              case 'not empty':
    +                $operator = 'not empty';
    

    Feels like we could combine these and just use $operator = $filter['operator']

  4. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,126 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    +                $datetime_value['value'] = '^' . $datetime_value['value'];
    ...
    +                $datetime_value['value'] = $datetime_value['value'] . '$';
    

    Do we need preg_quote here?

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Status: Needs review » Needs work

Needs work for #174.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
5.02 KB
43.69 KB

This should address #174 I think.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as #174 is addressed.

vaplas’s picture

Status: Needs work » Reviewed & tested by the community

Random fails (thanks to @naveenvalecha for checking this). It moved to #2901626: CSS animations cause \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest random fails.

Back to RTBC. Just for clarity:

+++ b/core/modules/datetime_range/datetime_range.module
@@ -26,3 +28,129 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
+              default:
+                $operator = 'regular_expression';
+                // Add .* to prevent blank regexes.
+                if (empty($datetime_value['value'])) {
+                  $datetime_value['value'] = '.*';
+                }
+                else {
+                  $datetime_value['value'] = preg_quote($datetime_value['value']);
+                }

Perhaps we can provide a more compatible update:

  • 'not_starts': '^(?<!' . $value . ').*$'
  • 'not_ends': '.*(?!' . $value . ')$'
  • 'shorterthan': '^.{,' . ($len_value - 1) . '}(?!' . $it_was_value . ')$'
  • 'longerthan': '^.{' . ($len_value + 1) . ',}(?!' . $it_was_value . ')$'
  • 'word': $value = implode('|', preg_split('/ /', $value));

But is it worth it?)

gambry’s picture

To my knowledge MySQL doesn't support lookahead (and I assume lookbehind too) so not_starts and not_ends won't work. For the other three I really don't see the scenarios when someone could use them other than "word" wich is covered on the case "default" logic.

I don't think is worth.

vaplas’s picture

@gambry, you are right! And Postgresql 9.1 (minimum version requirement) supports only loookahead (lookbehind from 9.6).
I also can offer only the ☘ scenarios of using last 3 operators. So I agree to remain without any magic.

artis’s picture

There was a mistake on this patch...ignore. Corrected patch on next comment.

artis’s picture

FileSize
12.99 KB

This is a patch against 8.3.x that excludes all the test code for anyone needing this functionality on D 8.3. I have this patch running on 8.3.7 right now.

Important: Run 'drush updb' or update.php after applying the patch.

benjy’s picture

Using hook_field_views_data() doesn't add the Views data to base fields that are defined in code. This will be a problem for date range fields now but is already an issue in core for datetime fields. I opened #2903770: Base fields miss out on Views data from hook_field_views_data() if anyone has any thoughts.

gambry’s picture

benjy’s picture

@gambry, nope but thanks for sharing, I was hoping someone here might know more.

tacituseu’s picture

yoroy’s picture

From the issue summary:

Deciding if we need a stronger message alerting the user to manually review the views being converted (and/or may be a Change record?)

Was there a decision on this, and if so, what was it? (And if a message was added, when/where does it show?)

gambry’s picture

Issue summary: View changes

@yoroy no, there wasn't a discussion. However as it was me wondering if we required a stronger message on #24.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.

I'm totally happy with the CR [#2857691] where says:

Site builders should review the affected views to check if the new operators and values reflect original views scopes.

Module developers using datetime_range fields in their modules views should update configuration and/or code accordingly.

So strikethrough-ing last remaining task from IS.

yoroy’s picture

Sounds good, thanks @gambry.

xjm’s picture

Title: Improve the Views integration for DateRange fields » The Views integration Datetime Range fields should extend the views integration for regular Datetime fields
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Trying to get my head around this; just a few observations so far...

  1. +++ 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.
    

    "Helper for" is generally pretty useless for documentation. We could simplify these two paragraphs to:

    Adds datetime views plugins to Views data for datetime-based fields.

    Edit: But actually, I'm not sure that's what it does. "Provides Views integration for all datetime-based fields." Or something?
     

  2. +++ b/core/modules/datetime/datetime.views.inc
    @@ -11,18 +11,38 @@
    +function datetime_type_field_views_data(FieldStorageConfigInterface $field_storage, $data, $column_name) {
    

    Wouldn't this collide with hook_field_views_data() for a datetime_type module? I'd suggest coming up with a name for his helper that's not going to collide with hook naming.
     

  3. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,129 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    + * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.
    + *
    + * @see https://www.drupal.org/node/2857691
    

    Deprecation messages should probably say what we'll do instead. I couldn't figure it out from looking at the change record. It's kind of weird to deprecate a hook implementation, also.
     

  4. +++ b/core/modules/datetime/datetime.views.inc
    @@ -11,18 +11,38 @@
    
    +function datetime_type_field_views_data(FieldStorageConfigInterface $field_storage, $data, $column_name) {
    

    $data can probably be typehinted as an array here in the signature, yes?
     

  5. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,129 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    + * Provides BC layer for modules using datetime_range filter/sort plugins.
    

    I think this should probably explain a little more clearly: "When a view is saved using the old string plugin format for Datetime Range filters or sorts, they will automatically be updated to Datetime filters or sorts."
     

  6. An issue summary update that summarizes all the different parts of the patch would be helpful to get this in; I think this is part of why this has lingered in the queue for so long. The User interface changes and API changes and Data model changes sections are all empty, and this patch includes all of them.
     
  7. I also find the change record confusing. I don't think it clearly indicates to readers whether they will be affected by the change or what will happen. It also doesn't explain the before and after and relies a lot on terminology for views internals. I think it should probably start out something like:

    In Drupal 8.2.x through 8.4.x, the Datetime Range module's fields did not use the same views integration as core datetime fields. This meant that they did not support features like year- or month-only formats for Views fields, regular expressions for Views filters,
    and so on. In Drupal 8.5.x, the Views integration for Datetime Range fields is updated to extend that of other datetime fields.

    You are affected by this change if you have a view that configures a field, filter criteria, or sort criteria for a Datetime Range field, or if you have a module that extends the Views integration for Datetime Range fields. When you update to Drupal 8.5.0, existing views that contain Datetime Range fields, filters, or sorts will be automatically updated to use datetime fields instead.

    And then where it has the "important update information". This is not very clear:

    The update will map existing datetime_range views string plugins operators/values according with this list:

    How do I know what this means? How do I know if it affects me? "Views string plugins" isn't going to mean a lot to most people as that's not something they ever saw in the UI; even developers writing their own bit of Views code will not necessarily understand. As far as I can tell, this only affects Datetime Range filters. So maybe:

    If you have a view that filters on a Datetime Range field from Drupal 8.4.x or earlier, the "operator" for the filter will be updated as follows:

    Then a table of the before-and-after (would be easier to understand and more semantically correct than the list), and maybe even a screenshot of where this is in the UI. We don't usually do that for CRs but in this case it's warranted. We can use one of the screenshots we're going to add to the IS. ;)

xjm’s picture

+++ b/core/modules/datetime/datetime.views.inc
@@ -11,18 +11,38 @@
+ * @return array
+ *   The array of field views data with the datetime plugin.

"An array of datetime plugin information for hook_field_views_data() for any datetime-based field." Maybe?

Dave Kopecek’s picture

Just successfully applied #184 against 8.3.7. Thanks @artis!

gambry’s picture

Assigned: Unassigned » gambry
Issue tags: +Vienna2017

I'm going to work on this in Vienna. If anyone want to help will be super appreciated!

gambry’s picture

Status: Needs work » Needs review
FileSize
3.98 KB
44.16 KB

#192:

  1. Changed to Provides Views integration for any datetime-based fields. and Overrides the default Views data for datetime-based fields, adding datetime views plugins. Modules defining new datetime-based fields may use this function to simplify Views integration.
  2. Totally right! ...namings things is alway hard. As this is a helper I've renamed it: datetime_type_field_views_data_helper()
  3. This change has been suggested on #174.1:
    In other cases where we've done this (e.g. #2851293: dblog is using the wrong views field, filter and relationships definitions) we also added an @deprecated tag to the function and used trigger_error in the function body - any reason why we're not doing that here? See dblog_view_presave in HEAD.

    However I understand your worries so I've improved the documentation in order to explain what is deprecated and so to be removed: When a view is saved using the old string or standard plugin format for Datetime Range filters or sorts, they will automatically be updated to Datetime filters or sorts. Old plugins usage must to be considered deprecated and must be converted before 9.0.0, when this updating layer will be removed.
    Update: already noticed must to be considered typo. Will change in next batch of work.

  4. Yep.
  5. See point 3 above

#193:

  1. "An array of views data, in the same format as the return value of hook_views_data()." is the @return description for hook_field_views_data(), which is what datetime_type_field_views_data_helper() should return. "An array of datetime plugin information" looks too vague, i.e. what this information structure is? I'm happy to improve the documentation, however I would stay as closer as possible to hook_field_views_data() description, so developers can easily find their way to the right structure to be returned. Thoughts?

Hiding #184 8.3 roll-up file.
Will go through the rest in next comment. Setting Needs Review in order to trigger the testbot, issue still needs work to cover the rest of the feedback from #192.

gambry’s picture

Issue summary: View changes

Updating the IS in order to address #192 explaining what we've done. This first batch of changes is to trying to explain how a simple views integration become the current patch.

I will update "User interface changes" and "API changes" and "Data model changes" with screenshot and description later.

gambry’s picture

gambry’s picture

Status: Needs review » Needs work

This is supposed to be Needs Work untill all feedbacks have been addressed. Still missing #192 6 (part of) and 7.

gambry’s picture

Assigned: gambry » Unassigned
Issue summary: View changes

Rephrasing one sentence on the IS.
Also stepping out as @jibran may complete the IS and CR changes requests.

gambry’s picture

Issue summary: View changes
Issue tags: +Needs change record updates

Small changes in the IS. Mainly re-phrasing few concepts, neither new additions nor deletions.

Also @jibran pointed out at Drupalcon:

  • User interface changes: the IS should mention this issue is neither adding nor removing UI elements, instead we are moving from string/standard views plugins UI to datetime ones. A screenshot with "before - after" should help.
  • API changes: "Extending the datetime fields Views integration in order to allow any datetime-based fields to make use of it" solution task introduce a new API helper for datetime-based fields who want to integrate datetime views plugins easily.
  • Data model: we are not changing any data model. However existing and contrib modules shipped views may be altered. Developers and site builders should be aware of that.
  • All the points above need a place in the CR - together with a good clean up - as flagged by #192.7 .
  • Upgrade test path is still using old simpletest. We should move it to use Drupal\FunctionalTests\Update\UpdatePathTestBase
vaplas’s picture

#201:

Upgrade test path is still using old simpletest. We should move it to use Drupal\FunctionalTests\Update\UpdatePathTestBase

done + replace assertIdentical() to assertSame() (#2756519: Convert assertIdentical() to assertSame() for some uses in kernel tests that already have assertSame()'s parameter order).

gambry’s picture

Issue summary: View changes
Status: Needs review » Needs work

Thanks @vaplas, code looks good!

Adding #201 tasks to the IS "Remaining Tasks" section. As test bot is now running, I'm setting back the status to Needs Work in order to complete those tasks.

vaplas’s picture

Issue summary: View changes
FileSize
13.81 KB
13.23 KB

@gambry, np)
Done "before - after" screenshots.

vaplas’s picture

Issue summary: View changes
FileSize
13.79 KB

Updated info in API changes and Model data sections. Did not del the tasks, because it need to verification.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs issue summary update, -Needs change record updates

Thanks, @gambry for remembering my verbal feedback. IS update looks good. Change notice shows appropriate changes. I think we can add the screenshot to change notice as well but that is a non-blocker. Coding feedback has been addressed and changes look good so setting it back to RTBC.

vaplas’s picture

jibran’s picture

vaplas’s picture

Issue summary: View changes

Oops, sorry) Done.

jibran’s picture

gambry’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

Better describing the changes.

Also I'm not sure the updates to the CR cover the worries raised by @xjm on #192?

mpdonadio’s picture

Status: Needs review » Reviewed & tested by the community

I tweaked the CR a bit to emphasize that these now work just like datetime fields. I think CR is OK, so I am setting RTBC.

mpdonadio’s picture

Status: Reviewed & tested by the community » Needs review

Going to try to track down a doc reviewer for this for an opinion on the CR.

Jon Pollard’s picture

I've upgraded core to 8.4 expecting this to be fixed but I'm not seeing it working in views. Have I missed something? Does this need patching for 8.4? I've tried applying various patch versions without success. I would love a bit of guidance on this :o)

mpdonadio’s picture

Status: Needs review » Reviewed & tested by the community

#214, no, this isn't in 8.4.0.

#211, setting this back to RTBC. I think a committer needs to weigh in now; I tried to get some outside feedback from those uninvolved with the issue, but wasn't successful.

jibran’s picture

gambry’s picture

Status: Reviewed & tested by the community » Needs review

Just for reference, here a proposal of CR I was thinking about https://pastebin.com/NYwSkh7s .
I'm happy with the current version, but just in case more work is requested at least we have an initial draft.

#214 Using composer I can apply nicely #202 to 8.4.0.

gambry’s picture

Status: Needs review » Reviewed & tested by the community

Sorry, refreshed and commented kept the old status.

Jon Pollard’s picture

As far as I can see I'm applying the patch OK with git, it doesn't change any files, just adds the following files:

core/modules/datetime_range/datetime_range.views.inc
core/modules/datetime_range/tests/fixtures/update/datetime_range-filter-values.php
core/modules/datetime_range/tests/fixtures/update/field.field.node.page.field_range.yml
core/modules/datetime_range/tests/fixtures/update/field.storage.node.field_range.yml
core/modules/datetime_range/tests/fixtures/update/views.view.test_datetime_range_filter_values.yml
core/modules/datetime_range/tests/src/Functional/Update/DatetimeRangeViewUpdateTest.php
core/modules/datetime_range/tests/src/Kernel/Views/DateRangeHandlerTestBase.php

If I run update.php I get errors like this:

Message Error: Call to undefined function datetime_type_field_views_data_helper() in datetime_range_field_views_data() (line 18 of /sitename/core/modules/datetime_range/datetime_range.views.inc) .....

Notice: Undefined variable: data in datetime_range_field_views_data() (line 20 of /sitename/core/modules/datetime_range/datetime_range.views.inc) .....

What I am trying to do is to filter events by date time range to not show events happening before now. Is there a way I can do this with regular expressions?

vaplas’s picture

#219: Most likely the incorrect applying of the patch is the cause of the failure. Because it change 4 files and adds 8 files (vs 0/7 in your post).

'undefined function datetime_type_field_views_data_helper()' appeared in the core/modules/datetime/datetime.views.inc. If it does not work out again, please try to check the work on the fresh website.

Steps to reproduce:

  • Install 8.4
  • Insatll datetime module
  • Create daterange field to any node bundle.
  • Add this field to filter of any views.
  • Save views.
  • Run update.php.
  • Check result, like before/after screenshots in IS.
Jon Pollard’s picture

OK, I've now got this working. The patch had not applied correctly and I needed to chop it up into bits and run them individually to get all the parts applied. Thanks for your help in debugging this.

xjm’s picture

So this issue and #2627512: Datetime Views plugins don't support timezones have both been struggling in the RTBC queue awhile, probably because they're both big and both at the intersection of two of the scariest subsystems. :P It might help if one of the other Views maintainers could review again and give an explicit signoff if everything looks good to go. I'm leaving it at RTBC because I don't think this is 100% a requirement; just something that might help.

Lendude’s picture

@everyone, nice work on this, this is really thorough.

Most importantly: Before adding more views that use the datetime filter/sort/argument plugins, I think it would be good to add the schema for this first per #2895544: Datetime module missing view plugin config schema

And just a bit of nitpicking, the @see to the wrong update is the only thing of importance really:

  1. +++ b/core/modules/datetime/datetime.views.inc
    @@ -11,18 +11,42 @@
    +  $data = (empty($data)) ? views_field_default_views_data($field_storage) : $data;
    

    extra () aren't needed around (empty($data))

  2. +++ b/core/modules/datetime_range/datetime_range.post_update.php
    @@ -5,9 +5,88 @@
    +
    

    extra newline can be removed

  3. +++ b/core/modules/datetime_range/datetime_range.post_update.php
    @@ -5,9 +5,88 @@
    +
    

    extra newline can be removed

  4. +++ b/core/modules/datetime_range/datetime_range.post_update.php
    @@ -5,9 +5,88 @@
    +
    

    extra newline can be removed

  5. +++ b/core/modules/datetime_range/tests/src/Functional/Update/DatetimeRangeViewUpdateTest.php
    @@ -0,0 +1,74 @@
    + * @see datetime_range_update_8001()
    

    this is referencing the wrong update (this is actually the only thing I care about fixing)

  6. +++ b/core/modules/datetime_range/tests/src/Functional/Update/DatetimeRangeViewUpdateTest.php
    @@ -0,0 +1,74 @@
    +
    

    extra newline isn't needed

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

    adding this seems like overkill for the functionality it provides, can't this just be set in the one test that uses this? Might shrink the patch a bit more.

dawehner’s picture

  1. +++ b/core/modules/datetime/datetime.views.inc
    @@ -33,11 +57,16 @@ function datetime_field_views_data(FieldStorageConfigInterface $field_storage) {
    +      $column_name_text = $column_name !== $field_storage->getMainPropertyName() ? ':' . $column_name : '';
    

    🔧 I kinda like to not do negative conditions, but that's really just personal style.

  2. +++ b/core/modules/datetime/datetime.views.inc
    @@ -33,11 +57,16 @@ function datetime_field_views_data(FieldStorageConfigInterface $field_storage) {
    +        'title' => t('@label@column (@argument)', [
    

    I'm a bit confused that this title doesn't have any space in between.

  3. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,133 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    +            // Set entity_type and field_name if missing.
    +            if (!isset($filter['entity_type'])) {
    +              $filter['entity_type'] = $filter_views_data['entity_type'];
    +            }
    +
    +            // Set datetime plugin_id.
    +            $filter['plugin_id'] = 'datetime';
    

    Nice catches! I would have not thought about the second case.

  4. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,133 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    +            @trigger_error('Use of string filters for datetime_range fields is deprecated. Use the datetime filters instead.', E_USER_DEPRECATED);
    ...
    +            @trigger_error('Use of standard sort handlers for datetime_range fields is deprecated. Use the datetime sort handlers instead.', E_USER_DEPRECATED);
    

    Let's link to some change record here ... Maybe module default configs needs to be updated.

  5. +++ b/core/modules/datetime_range/datetime_range.post_update.php
    @@ -5,9 +5,88 @@
    +              continue 2;
    

    Sometimes I have the following the goto statements would be better to read that continue statements :P

  6. +++ b/core/modules/datetime_range/datetime_range.views.inc
    @@ -0,0 +1,21 @@
    +function datetime_range_field_views_data(FieldStorageConfigInterface $field_storage) {
    +  \Drupal::moduleHandler()->loadInclude('datetime', 'inc', 'datetime.views');
    +
    

    Is it just me that the .views.inc should always be loaded when this code is triggered? Maybe we could add a line of documentation, why this includig is needed.

xjm’s picture

Status: Reviewed & tested by the community » Postponed

Sounds like we should postpone on #2895544: Datetime module missing view plugin config schema. I think @larowlan will commit that one soon. After that we can incorporate the above reviews. Thanks @dawehner and @Lendude for responding to the Bat Signal. :)

gambry’s picture

From #224:

+++ b/core/modules/datetime_range/datetime_range.module
@@ -26,3 +28,133 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
+            // Set entity_type and field_name if missing.
+            if (!isset($filter['entity_type'])) {
+              $filter['entity_type'] = $filter_views_data['entity_type'];
+            }
+
+            // Set datetime plugin_id.
+            $filter['plugin_id'] = 'datetime';

Nice catches! I would have not thought about the second case.

@dawehner we've done this bit together at Dublin2016, so you may have!

However the code refers to entity_type and field_name but only entity_type is set.
TL;DR: Either the code comment is wrong and we only need to set entity_type OR we need to restore field_name.

I went back in this issue history and the field_name has been removed by #100 due UpdatePathTestBase failing with

fail: [Other] Line 287 of core/modules/system/src/Tests/Update/UpdatePathTestBase.php:
Schema key views.view.test_datetime_range_filter_values:display.default.display_options.filters.field_range_value.field_name failed with: missing schema

.

These are both needed for the Drupal\views\FieldAPIHandlerTrait to work properly, but we are setting them on pre-saving the view and indeed there is no field_name schema key definition so test fails correctly.

Either the code comment is wrong and we only need to set entity_type OR we need to restore field_name setter in this patch and doing some refactor if tests still fail.

Out of my mind first option is the right one -" code comment is wrong" - and field_name together with entity_type should be set by the hook_field_views_data() if not already existing (as #2627512: Datetime Views plugins don't support timezones does for the custom date argument for example).

mpdonadio’s picture

Status: Postponed » Needs work

Blocker is in, but # 223 and #224 needs to be addressed.

gambry’s picture

I'm working on this. Should post a patch addressing #223 and #224 by today/tomorrow.

gambry’s picture

This patch addresses feedback from #223, #224 and #226. Below few notes:

- #223 .2, .3, .4, .6: I'm not sure which extra lines you are referring to. I can guess but as you said "the @see to the wrong update is the only thing of importance really" I haven't actioned these points.
- #223 .5: done.
- #223 .7: I agree. Removed and FilterDateTest refactored.
- #224 .1: done.
- #224 .2: @column is either empty or starting with :, producing as title either i.e. "Date (field_date)" or "Date (field_date:end_value)". See attached screeshot.
- #224 .3: This comment has actually been changed according with #226 investigation.
- #224 .4: Links added. I'm not sure what the best practices is in terms of final period after links. I've seen trigger_error() instances in core with "See LINK" (without final period) and it think renders better than "See LINK .".
- #224 .5: :p
- #224 .6: that is datetime_range.views.inc lodaing datetime.views.inc, which is not always loaded (there is a comment about the relative error in this issue, just can't find it right now). But a comment explaining what's going on may improve DX. We link to this method from the helper itself after all.

mpdonadio’s picture

Status: Needs review » Reviewed & tested by the community

I think the changes in #229 are good and we are full green across the various bots.

Leaving Needs subsystem maintainer review in case @dawehner or @Lendude want one more look.

jibran’s picture

#224 .6: that is datetime_range.views.inc lodaing datetime.views.inc, which is not always loaded

This is surprising.

gambry’s picture

that is datetime_range.views.inc loading datetime.views.inc, which is not always loaded (there is a comment about the relative error in this issue, just can't find it right now)

The issue has first been spotted on #36and fixed on #39.

This is surprising.

Looking at views_views_data() doesn't surprise me. There is no control on which order hook_field_views_data() (and so *.views.inc) are loaded. Additionally:

foreach ($entity_manager->getStorage('field_storage_config')->loadMultiple() as $field_storage) {...}

If there aren't any datetime fields instances then datetime.views.inc is not loaded.

xjm’s picture

Status: Reviewed & tested by the community » Postponed

I checked and it looks like this and #2627512: Datetime Views plugins don't support timezones don't apply at the same time, but both are currently RTBC. Since that one is a major bug whereas this one is maintainability/refactoring, let's postpone this one on that.

Yaazkal’s picture

Hi, patch in #229 works great.

yi_jiang’s picture

#229 works for me. Curious why it's not in core.

lsabug’s picture

@yi_jiang, I think because of comment #223

lsabug’s picture

@yi_jiang, I think because of comment #233

jibran’s picture

#229 applies cleanly to 8.4.3 even after #2627512: Datetime Views plugins don't support timezones. Please ignore if you are not using it with 8.4.x.

espurnes’s picture

#229 works for me on drupal 8.4.3. Thanks.

jibran’s picture

Status: Postponed » Needs work
Issue tags: +Needs reroll
vaplas’s picture

Status: Needs work » Needs review
FileSize
43.94 KB
5.91 KB

Done. Silent 'Apply Patch' via PhpStorm.

jibran’s picture

Thanks, for the reroll. Subsytem maintainer reviewed it in #223 and #224 so removing the tag. It was RTBC in #233 so setting it back to RTBC.

Darvanen’s picture

Status: Needs work » Reviewed & tested by the community

CI failure, resubmitting for testing.

pvsureshmca’s picture

#229 works for me on Drupal 8.4.3. Thank You

vaplas’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
43.97 KB

added legacy group to update test + CS fix.

vaplas’s picture

Status: Needs work » Needs review
FileSize
44.22 KB
1.15 KB

Fix is based on c/p from Drupal\Tests\datetime\Kernel\Views\FilterDateTest::getUTCEquivalentOfUserNowAsTimestamp(). Maybe it's worth making a parent class or a trait for them?

One more question, is it possible to random fail, if date in setUp() is 23:59:59, but in testDateOffsets() it's becoming a 00:00:00 (new day)?

mpdonadio’s picture

+++ b/core/modules/datetime_range/tests/src/Kernel/Views/FilterDateTest.php
@@ -47,7 +48,9 @@ protected function setUp($import_test_views = TRUE) {
-    static::$date = \Drupal::time()->getRequestTime();
+    $user_now = new DateTimePlus('now', new \DateTimeZone(drupal_get_user_timezone()));
+    $utc_equivalent = new DateTimePlus($user_now->format('Y-m-d H:i:s'), new \DateTimeZone(DATETIME_STORAGE_TIMEZONE));
+    static::$date = $utc_equivalent->getTimestamp();
 

Truly baffled why this particular problem is showing up now, and not also in Drupal\Tests\datetime\Kernel\Views\FilterDateTest, unless there is something from #2627512: Datetime Views plugins don't support timezones that we need to change in these tests.

mpdonadio’s picture

Status: Needs review » Needs work

OK, looked at this in detail.

datetime/FilterDateTest had changes in it b/c #2627512: Datetime Views plugins don't support timezones.

We need to mirror these changes in the new daterange/FilterDateTest (think it is just the way the ::setUp is done). The common functions from datetime/FilterDateTest can move into Drupal\Tests\datetime\Kernel\Views\DateTimeHandlerTestBase

I think that is the best plan for the long term.

gambry’s picture

We kind of knew a refactoring of tests was needed, we just forgot (see #28).

I will try to tackle this during the weekend.

BTW nice job everyone! This and #2627512 were a must-have for any event-based website. I hope this can make his way into core soon too.

gambry’s picture

In this patch:

  • methods getUTCEquivalentOfUserNowAsTimestamp() and getRelativeDateValuesFromTimestamp() have been moved from datetime FilterDateTest to DateTimeHandlerTestBase in order to be easily re-used on datetime_range FilterDateTest.
  • Docblock for DateTimeHandlerTestBase::getRelativeDateValuesFromTimestamp() method has been rephrased to better match its logic
  • Calls to DATETIME_DATE_STORAGE_FORMAT and DATETIME_STORAGE_TIMEZONE have been replaced with DateTimeItemInterface constants now that #2826404: Create DateTimeItemInterface and deprecate global constants in datetime.module is in.
gambry’s picture

Status: Needs work » Needs review
mpdonadio’s picture

Status: Needs review » Reviewed & tested by the community

Read the interdiff and the most recent changes applied and LGTM.

jibran’s picture

Status: Needs work » Reviewed & tested by the community
yi_jiang’s picture

Is this patch merging to core?

jibran’s picture

+++ b/core/modules/datetime_range/datetime_range.module
@@ -26,3 +28,133 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
+ * @deprecated in Drupal 8.4.x and will be removed before 9.0.0.

s/8.4.x/8.5.x can be fixed on commit.

vaplas’s picture

Status: Needs work » Reviewed & tested by the community

#255 Perfect!

#260 Not.

#262: It seems we are already close to catch this annoying random fail.

kclarkson’s picture

#229 worked for me.

Are we supposed to combine another patch with this?

pvsureshmca’s picture

Hi

#264 Patch

I have applied this patch in https://simplytest.me/ (Drupal core 8.4.3). it is displaying error. please look at the attached image.

But it is working for me in Drupal 8.5.X.

Thank you.

jonathanshaw’s picture

This issue now targets 8.5.x; the patch from #229 applies to 8.4.3

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 270: 2786577-270.patch, failed testing. View results

jibran’s picture

Status: Needs work » Reviewed & tested by the community
jcontreras’s picture

FileSize
45.13 KB

Rerolled Patch for 8.4.4 - RTBC patch is still #270.

The last submitted patch, 270: 2786577-270.patch, failed testing. View results

gambry’s picture

RTBC patch is still #270

larowlan’s picture

  1. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,133 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    +                $operator = 'regular_expression';
    ...
    +                  $datetime_value['value'] = '.*';
    

    are we sure this is what we want as a default?

  2. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,133 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    +          if (!isset($sort_views_data['entity_type']) || !isset($sort_views_data['field_name'])) {
    +            continue;
    

    when would this happen?
    should we notify the user that stuff is borked?

jibran’s picture

Thanks, for the review @larowlan

  1. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,133 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    +              default:
    +                $operator = 'regular_expression';
    +                // Add .* to prevent blank regexes.
    +                if (empty($datetime_value['value'])) {
    +                  $datetime_value['value'] = '.*';
    +                }
    +                else {
    +                  $datetime_value['value'] = preg_quote($datetime_value['value']);
    +                }
    

    @larowlan and I discussed this bit and I think we should only keep the else part. If there is no value we should leave it as is without setting the operator. Empty filter value doesn't mean that we show unfiltered results.

  2. +++ b/core/modules/datetime_range/datetime_range.module
    @@ -26,3 +28,133 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
    +          if (!isset($sort_views_data['entity_type']) || !isset($sort_views_data['field_name'])) {
    +            continue;
    +          }
    

    This means views data is faulty. This shouldn't happen but in case it is happening then it is not our fault so skipping it makes sense.

jibran’s picture

Status: Reviewed & tested by the community » Needs work

NW for #276.1 and #277.1 let's see what others(@gambry, @jhedstrom or @mpdonadio) think about.

gambry’s picture

+++ b/core/modules/datetime_range/datetime_range.module
@@ -26,3 +28,133 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
+              default:
+                $operator = 'regular_expression';
+                // Add .* to prevent blank regexes.
+                if (empty($datetime_value['value'])) {
+                  $datetime_value['value'] = '.*';
+                }

This block was introduced by #129 to fix the caveats found by #128. The only examples where you may have empty values in those comments are with empty and not empty operators, not considered up to #127 but now covered since #129.

If there is no value we should leave it as is without setting the operator. Empty filter value doesn't mean that we show unfiltered results.

We require to set an operator. This issue goal is to switch the current datetime_range views filter from type "string" to "date". The default case above covers operators not existing on "date" type, like: contains, word, allwords, not_starts, not_ends, shorterthan, longerthan and regular_expression itself.

And we also need a value, because regular_expression operator fails for empty values, so we either change the operator or decide what a default value would be.

Thoughts?

jibran’s picture

@gambry thanks for the context.
We are changing the data type of a filter so I think not mapping all the values makes sense because contains, word, allwords, not_starts, not_ends, shorterthan, longerthan doesn't make sense for a numeric filter.
Here is my suggestion:

  • If the value is empty then the operator should be changed to empty
  • If the value is not empty then the operator should be 'regular_expression' and value should be wrapped by preg_quote.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Dave Kopecek’s picture

Successfully applied Rerolled Patch for 8.4.4 from #273. Thanks @jcontreras !

gambry’s picture

Status: Needs work » Needs review

If the value is empty then the operator should be changed to empty

I'm just worried site-builders who have previously selected "contains" operator (*) with an empty value - i.e. because the filter will be exposed - will see their views not working anymore because we changed the operator.

I know these may not be common scenarios, but I can picture someone using operators like "contains", "not_starts" or "not_ends" to come up with custom solutions for filtering dates. And the fact "value" input is not required enforce the possibility of having an empty value.

Going back to the original problem:

+++ b/core/modules/datetime_range/datetime_range.module
@@ -26,3 +28,133 @@ function datetime_range_help($route_name, RouteMatchInterface $route_match) {
+                $operator = 'regular_expression';
+                // Add .* to prevent blank regexes.
+                if (empty($datetime_value['value'])) {
+                  $datetime_value['value'] = '.*';
+                }

[...]are we sure this is what we want as a default?

It's harmless, if this is the concern. It produces the same results as if the filter had an empty value (show all results), but for regular_expression operator an empty value throw an SQL error (which opens to a bigger conversation if this is expected or a bug).
The only downside of having ".*" as default value is if the filter is exposed won't look as beautiful as an empty textfield. But all will still work.

I still think this is the best option, but if we want to go to the path suggested by @jibran I do suggest to reintroduce the idea of a stronger message to the user about this change needed his/her review (see #189 #190).

(*)UPDATE: Forgot to mention "contains" is the only solution I can image allowing filtering by year, month or day with "string" filter plugin. IE Contains "2018' select all the 2018 nodes , Contains '2018-01' January 2018, Contains '-01-' January every year. I know, hacky... but probable.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Let's get committers opinion,

mpdonadio’s picture

I have been pondering this a lot, but have not had time to set up a manual both patch scenarios. I think I agree with the patch as-is, and we make sure this gets into the release notes and maybe review the CR and potentially make it more explicit.

larowlan’s picture

Issue tags: +8.6.0 release notes

The only downside of having ".*" as default value is if the filter is exposed won't look as beautiful as an empty textfield.

that seems like a red flag

I think we're going to need release notes here.

And I think the sooner this goes into the next minor, the more likely we are to find issues.

The last submitted patch, 270: 2786577-270.patch, failed testing. View results

jibran’s picture

Re-test of #270 came back green so this is still RTBC.

gambry’s picture

The only downside of having ".*" as default value is if the filter is exposed won't look as beautiful as an empty textfield.

that seems like a red flag

I know. I keep thinking about finding another solution, but we need to either decide if breaking those views or at least leaving them working while the sitebuilder can switch to the proper datetime operator.

In the meanwhile I've updated the CR, making its content more meaningful for non-technical users, i.e. using the operators labels ("Is equal to") instead of their machine name (=).

gambry’s picture

Issue tags: +SprintWeekend2018
mpdonadio’s picture

I have though about this several times and haven't posted since I don't know which option is more correct. I am leaning towards having the least impact on end users, despite how ugly it is.

I pinged @Lendude on Slack to see what he thinks.

larowlan’s picture

Can we add the new plugins and deprecate instead of removing the existing ones?
Then the upgrade path issue is gone

mpdonadio’s picture

#292, the thing is we aren't adding any new plugins. The patch is essentially just a plumbing change: daterange fields currently use the default string plugins from views, this patch sets them up to use the existing datetime ones. The actual patch is small; most of it is the test coverage and the upgrade path to map settings.

I am not sure how we could really deprecate anything here.

larowlan’s picture

Right, but we can add new entries to the views data, something like some_date_field__date which uses the new plugin, retain the old ones at some_date_field and indicate that the field is deprecated.

Perhaps we need a new issue 'work out how to deprecate views data entries' first, where I'd imagine the fix would be a warning in the views UI and a hook_requirements showing views using deprecated handlers with advice on how to fix.

gambry’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

@larowlan I see what you mean, however that won't work as explained on #16:

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

Applying this patch without converting the string plugins to datetime raises a fatal error. That's why we are forced to do the update path.
We can find a workaround applying your suggestions to 'start date' values only, but than this issue's code will start looking like spaghetti code. :(
Besides if we do that still doesn't help answering the question: '.*' YES or NO.

Striking-through Remaining tasks about IS update for section "API changes" and "Data model", already done in #203.
Also striking-through the task about adding those IS sections to the Change Record. They all were already there BUT "Api changes". I've added it now so I'm setting the issue status to Needs Review so someone can validate the text makes sense.

Lendude’s picture

I agree with @gambry I'm not thrilled about #294, that would become messy both in the code and in the UI I feel. So if we can avoid that, that would be great.

I'm manually testing the 'empty regex' issues, and to be honest, I can't reproduce them. Leaving it empty just means the filter isn't used.

What's worse, setting the filter value to .* when the filter is not exposed, means it raises an error 'Invalid date format.' when saving it in the UI. So we are injecting an invalid value (The fact that the regex operator shouldn't expect a valid date format in the first place, is out of scope).

Are we sure the whole empty regex is an issue? Can we add a test that the filter works (or fails) after the update when using a blank regex?

gambry’s picture

Status: Needs review » Needs work

Are we sure the whole empty regex is an issue? Can we add a test that the filter works (or fails) after the update when using a blank regex?

I must admit I can't replicate it myself anymore, but I definitely saw the error before.
For reference this is the screenshot with the error, discovered by #128.

I will try to replicate it and if I'm successful will add steps to reproduce and test coverage.
And If not I'll remove the '.*' bit so I can sleep better tonight. :)

Thanks everyone.

larowlan’s picture

Status: Needs work » Reviewed & tested by the community

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

Can we elaborate on why?

Just to be clear, in my proposal, we're leaving the existing views and views_data as is - I'm not sure how that would cause a fatal.

And then adding new views data entries for date range fields that extend from the date ones.

Anybody’s picture

I can not confirm #295 and was not able to reproduce it. So lets keep RTBC for now and hope for an integration into core soon. Thank you all very much.

mpdonadio’s picture

#298, the steps to reproduce the exception are in #16, which is when I set this as a Major. I'll try this again tonight.

wooody’s picture

Hello!
Is this bug fixed now in Drupal 8 Core ? or we still need to use this patch to fix this problem ?

Thanks

mpdonadio’s picture

#301, the patch is not in core yet. The latest version, against 8.5.x, is in #270.

wooody’s picture

Thanks mpdonadio for your answer. strange its not in the core yet and the issue from 2016 😨

vaplas’s picture

Yes, this issue has a long story. In fact, it appeared even before the #2161337: Add a Date Range field type with support for end date was commited (and module was in experimental mode).

And using a string filter was planned only as a short-term stub. It is really very difficult to imagine the benefit of such a filter when dealing with dates.

I'll venture to assume that anyone who needs to set up a filter uses a patch (and a large number of folowers confirm this), or just refuses to use the Datetime Range, and converts everything into datetime fields (with a negative mood).


It's great that we have rules for converting from string to date filter (because for its implementation @gambry has helped in correcting the work of regular expressions in Views (#2821112: Views NumericFilter 'regular_expression' operator is broken), and in general regular expressions in Postgresql #2845543: PostgreSQL regular expression match operators works only for text 😃).

But, to be honest, this is superfluous. Guys could do this conversion, and they did it. But, again, I can not imagine that someone has a customized string filter for the date ☘️☘️☘️.

So, absolutely agree that there are no BC violations here. We just change the non-working filter to the working one.
Can this help add the patch to 8.5?)

Anybody’s picture

How can we ensure this essential patch will be part of 8.5? Is there a core maintainer who can provide feedback?

Darvanen’s picture

There are several core maintainers and contributers in this conversation already @Anybody (not me).

pookmish’s picture

Any re-roll of this patch for 8.4.5?
I had to reorder my patches and this worked.

Jon Pollard’s picture

I've had a problem getting this working again. Eventually I tried just replacing the core datetime and datetime_range modules with previous versions from the patched 8.4.2 version that was working - and hey presto my site came back to life.

Now, I know this isn't a very good way of fixing things but it gets my site back up and running. Does anybody have the 'correct' versions of these two files taken from the 8.4.5 version with patches applied? Is it really that simple? Am I being dim? I still can't figure out why the patches won't run... Anybody else out there having similar problems?

MrPeanut’s picture

@Jon Pollard (#308) — I'm using the patch from #85 successfully on 8.4.5. Hope that helps.