Problem/Motivation

Views integration for the datetime field type is non-existent.

Proposed resolution

Create filter, sort, and argument plugins for the datetime module.

Remaining tasks

User interface changes

Better options for building views with date fields.

API changes

3 datetime-specific plugins to be added.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug
Issue priority Major because the date integration with Views is not complete
Unfrozen changes Unfrozen because it only adds crucial functionality to the date field views integration
Prioritized changes The main goal of this issue is usability

Original report by @KarenS

This is a follow up to #501428: Date and time field type in core. Once the date field is in it needs better Views integration. It needs a filter and argument that have logical settings for date fields rather than the plain string filter and argument provided as the default.

This is currently blocked until the field gets in, but even fixing it in contrib is blocked by #1833296: Unable to alter views data.

Files: 
CommentFileSizeAuthor
#150 interdiff.txt18.4 KBbojanz
#149 date-views-integration-1838242-148.patch39.86 KBbojanz
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,328 pass(es).
[ View ]
#144 date-views-integration-1838242-144.patch40.32 KBjhedstrom
PASSED: [[]]: [] 98,145 pass(es).
[ View ]

Comments

KarenS’s picture

Title:Add better Views integration for date field» Improved Views integration for date field

Fixing title

sun’s picture

Title:Improved Views integration for date field» Improve Views integration for datetime field
Component:views.module» datetime.module
Status:Postponed» Active
Issue tags:+VDC
tim.plunkett’s picture

I'm not going to clobber the major queue by bumping this, but this is pretty darn important.

dawehner’s picture

@karens

It feels like we should bring in the features of the date filter/sort to support datetime values as well,
so something similar to what you started on #241759: Better date filter and continued in the date module?

tim.plunkett’s picture

Assigned:Unassigned» tim.plunkett

I'm going to claim this one for now. We'll see.

tim.plunkett’s picture

Assigned:tim.plunkett» Unassigned

Haven't had the time :(

tim.plunkett’s picture

Issue summary:View changes

Fix the wrong related issue id

klonos’s picture

dawehner’s picture

Assigned:Unassigned» dawehner

Let me try that.

tim.plunkett’s picture

blueminds’s picture

Priority:Normal» Major

I think this is major as the views/datefield combination is not really useful currently. Pls, correct me if I am wrong.

judahtanthony’s picture

Is it just me, or are your dates stored as strings? This has to be the worst data type to store a date in. A string has no sequence and is (most likely) bound to a timezone. Why don't we store it as a DATETIME or at least an INT? It seems pretty significant that you can't create a simple calendar in D8.

field.views.inc: field_views_field_default_views_data()

<?php
   
// Identify likely filters and arguments for each column based on field type.
   
switch ($attributes['type']) {
      case
'int':
      case
'mediumint':
      case
'tinyint':
      case
'bigint':
      case
'serial':
      case
'numeric':
      case
'float':
       
$filter = 'numeric';
       
$argument = 'numeric';
       
$sort = 'standard';
        break;
      case
'text':
      case
'blob':
       
// It does not make sense to sort by blob or text.
       
$allow_sort = FALSE;
      default:
       
$filter = 'string';
       
$argument = 'string';
       
$sort = 'standard';
        break;
    }
?>
jbrown’s picture

dawehner’s picture

Assigned:dawehner» Unassigned

HAHA

jhedstrom’s picture

I wonder if this makes sense to do in core at this time. It seems to me there will still be need for a contrib 'Date' module, to provide things that have not made it into core (end dates immediately come to mind). As such, contrib views integration would most likely need to rework the core views integration.

tim.plunkett’s picture

I think that making date fields at all usable in Views (which they are really not right now) is still a major task that could be committed at any time.
Yes, date.module will need to exist in contrib to provide end dates and the views handlers for that, but we shouldn't give up on handlers for datetime.

webchick’s picture

Agreed on both.

jhedstrom’s picture

Assigned:Unassigned» jhedstrom
Issue summary:View changes

I'll give this a shot.

jhedstrom’s picture

Issue summary:View changes
jhedstrom’s picture

Issue summary:View changes
Status:Active» Needs review
StatusFileSize
new2.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,077 pass(es).
[ View ]

This is a very rough start. It has a 'working' filter plugin. However, I don't think this approach will be cross-database compatible, and to truly get working date filters, we'll need to bring in more of the SQL logic from the Date module (ADDTIME, STR_TO_TIME for MySQL, and the PostgreSQL equivalents).

jhedstrom’s picture

Status:Needs review» Needs work
jhedstrom’s picture

To add an argument plugin, it may be necessary to bring over some of the Date module SQL logic I mentioned above. From D7, adding an argument allowed one to pass in date parts (eg, 2015 for all dates in 2015, or 2015-10 for just October 2015 dates). The resulting SQL (when using MySQL):

WHERE (( (DATE_FORMAT(ADDTIME(STR_TO_DATE(field_data_field_date.field_date_value, '%Y-%m-%dT%T'), SEC_TO_TIME(-25200)), '%Y-%m') >= '2015-10' AND DATE_FORMAT(ADDTIME(STR_TO_DATE(field_data_field_date.field_date_value, '%Y-%m-%dT%T'), SEC_TO_TIME(-25200)), '%Y-%m') <= '2015-10') )

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new6.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,093 pass(es).
[ View ]
new4.24 KB

Actually, looking into how numeric dates are handled, each part is a separate argument type (and core already has the date SQL methods mentioned above, just by different names).

I've started on this approach, and am posting for review now to see what folks think of this direction.

jhedstrom’s picture

StatusFileSize
new8.01 KB
new14.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,049 pass(es).
[ View ]

This adds tests for the new filter plugin.

jhedstrom’s picture

StatusFileSize
new13.48 KB
new22.24 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,131 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This adds tests for the 3 argument types (year, month, and day). It also uses format_date() instead of just date() for timezone compatibility.

jhedstrom’s picture

Issue summary:View changes
StatusFileSize
new5.13 KB
new22.76 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,169 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This replaces the simple string-comparison in the filter with db-native date functions. I've run the tests on MySQL and PostgreSQL.

MySQL is all green, PostgreSQL fails with:

function to_char(character varying, unknown) does not exist LINE 6: WHERE (( (TO_CHAR(node__field_date.field_date_value, &#039;YYYY&#039;)... ^ HINT: No function matches the given name and argument types. You might need to add explicit type casts.

since the current logic assumes the field being passed in is numeric. I think this can be addressed by overriding the method in the datetime filter handler.

I also added an initial beta-phase evaluation to the summary.

jhedstrom’s picture

Issue summary:View changes
jhedstrom’s picture

Issue summary:View changes
StatusFileSize
new5.55 KB
new25.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,135 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

This gets things working for PostgreSQL, but isn't pretty. The Date module uses the 'EXTRACT' function to grab date partials, but that is a lot of logic to bring in at first glance.

The last submitted patch, 24: date-views-integration-1838242-24.patch, failed testing.

The last submitted patch, 25: date-views-integration-1838242-25.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 27: date-views-integration-1838242-27.patch, failed testing.

jhedstrom’s picture

Status:Needs work» Needs review

I'm unsure as to why that one test is failing. It passes locally on both pgsql and mysql.

Setting this back to needs review since feedback on the technical approach is more important at this stage.

jhedstrom’s picture

StatusFileSize
new968 bytes
new25.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,187 pass(es).
[ View ]

This fixes the fail, which was caused by an additional record with a January date being created due to the current date (this was copied from the filter test which needs relative test data).

Sam152’s picture

Came up against this issue. Patch adds good support for filtering with custom date fields. One thing I noticed is that using the table style, dates were not able to be made sortable.

If that is out of scope for this issue +1 to RTBC.

jhedstrom’s picture

Issue summary:View changes

One thing I noticed is that using the table style, dates were not able to be made sortable

This seems to be a larger issue than just dates. I tried adding a numeric field, and that wasn't click sortable either. I'm not able to find an open issue for this though.

This did get me thinking about adding a datetime sort handler though, which would add granularity for sorting.

jhedstrom’s picture

Issue summary:View changes
StatusFileSize
new8.57 KB
new31.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,228 pass(es), 18 fail(s), and 5 exception(s).
[ View ]

This patch adds a sort handler and corresponding tests.

jhedstrom’s picture

I created #2395763: Fields are not 'click sortable' in views for the click sort issue mentioned in #33.

Status:Needs review» Needs work

The last submitted patch, 35: date-views-integration-1838242-35.patch, failed testing.

Status:Needs work» Needs review
Sam152’s picture

In terms of functionality I vote +1 to RTBC, works as advertised with no issues.

Lendude’s picture

Status:Needs review» Needs work
+++ b/core/modules/datetime/datetime.views.inc
@@ -0,0 +1,47 @@
+  $data = field_views_field_default_views_data($field_storage);

This function has been renamed to views_field_default_views_data, this is what causes the tests to fail. Changing the function name turns tests back to green locally.

some nitpicking....

  1. +++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
    @@ -0,0 +1,71 @@
    + * Definition of Drupal\views\Plugin\views\filter\String.

    Change to Contains \ and set to the right class name

  2. +++ b/core/modules/datetime/src/Tests/Views/ArgumentDateTimeTest.php
    @@ -0,0 +1,141 @@
    + * Contains of Drupal\datetime\Tests\Views\ArgumentDateTimeTest.

    'of' too many, lacks leading \

  3. +++ b/core/modules/datetime/src/Tests/Views/FilterDateTimeTest.php
    @@ -0,0 +1,151 @@
    + * Contains of Drupal\datetime\Tests\Views\FilterDateTimeTest.

    of too many, missing leading \

The last submitted patch, 35: date-views-integration-1838242-35.patch, failed testing.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new3.07 KB
new31.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,932 pass(es).
[ View ]

Thanks for the feedback @Lendude!

This should fix the fatal error, and code comments as mentioned in #42.

It'd be great to get this in before Beta 5 if it isn't too late.

pivica’s picture

StatusFileSize
new4.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch interdiff-1838242-43-44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new33.21 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,921 pass(es).
[ View ]

Patch for #43 works nicely, but what I miss is a fact that end user will see plain text fields for datetime values in views filter config forms and exposed filter form.

Patch #44 is trying to fix this, introducing datetime controls for views datetime filter config popup and exposed datetime filters.
Also one additional settings is addition of 'compare by date' options which alows user to specify does it wants to filter dates by date or also by time part. This is usefull when end user wants to filter all post in one specific day or couple of days and does not wants to select time part.

The last submitted patch, 44: interdiff-1838242-43-44.patch, failed testing.

pivica’s picture

StatusFileSize
new4.34 KB
new33.39 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 84,933 pass(es).
[ View ]

OK this form values elements works little strange when they are in config popup and later in views exposed form. New patch against #43, should work better now.

jhedstrom’s picture

I think this direction makes sense to potentially simplify input (not requiring full date+time being entered).

+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -0,0 +1,127 @@
+        '#description' => $this->t('Should we compare just date part or also time part?'),

I think this UI text needs to be reworked, taking into account https://www.drupal.org/node/604342.

Berdir’s picture

Yeah :)

I think the way it used to work in 7.x is that you can tick of the granularity for every part, from year to seconds. We should probably look at how that works and steal from there, instead of those two hardcoded "part" thingies ;)

benjy’s picture

Tested this patch, the "An offset from the current time such as "+1 day" or "-2 hours -30 minutes" feature doesn't seem to do anything right now?

jhedstrom’s picture

@Benjy Using an offset is working for me. There's also a test that uses '- 1 day'.

jhedstrom’s picture

@Benjy nevermind, I was still testing with the patch from #43. This is indeed not working in the patch from #46.

jhedstrom’s picture

Thinking about this more, the UI changes from #46 should probably be split into a separate issue which would also address the numeric date plugin UIs provided by Views. The UI from the patch in #43 is identical to that provided by Views.

jhedstrom’s picture

StatusFileSize
new31.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

This is a reroll of #43. As I mentioned in #52, let's improve the UI of both the numeric date plugins as well as these new plugins in a follow-up issue so they can have a unified look and feel.

Status:Needs review» Needs work

The last submitted patch, 53: date-views-integration-1838242-53.patch, failed testing.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new489 bytes
new31.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,690 pass(es), 15 fail(s), and 3 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 55: date-views-integration-1838242-55.patch, failed testing.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new2.65 KB
new31.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,711 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 57: date-views-integration-1838242-57.patch, failed testing.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new1.26 KB
new31.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,469 pass(es), 0 fail(s), and 9 exception(s).
[ View ]

More schema fixes.

Lendude’s picture

+++ b/core/modules/datetime/src/Plugin/views/argument/Date.php
@@ -0,0 +1,46 @@
+class Date extends NumericDate {

Since this extends the numeric Date argument handler it suffers from the same problems as that handler when trying to add default date arguments, see #2325899: Views argument handlers no longer can provide their own default argument handling. The fix proposed there would probably work for this implementation too, but haven't tested with both patches applied. But if either issue gets in, it would influence the other, so added it as related.

I'd love to see #2325899: Views argument handlers no longer can provide their own default argument handling get in first, otherwise we'd just be introducing more known issues.

Other then that, this looks good to me.

Status:Needs review» Needs work

The last submitted patch, 59: date-views-integration-1838242-59.patch, failed testing.

anavarre’s picture

Issue tags:+Needs reroll
jhedstrom’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.75 KB
new31.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,513 pass(es).
[ View ]
GaëlG’s picture

StatusFileSize
new31.11 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,460 pass(es).
[ View ]
new1.75 KB

Sorry if it's not the right place to tell about it. I can create a separate issue if it's better.

I just needed views datetime integration for an internal D8 project, and wanted to be able to set something like "last day of this month" in the date filter. Here's a patch. Drupal\views\Plugin\views\filter\Date might need the same kind of update too?

jhedstrom queued 65: 1838242-65.patch for re-testing.

tim.plunkett’s picture

Title:Improve Views integration for datetime field» Provide Views integration for datetime field
Category:Task» Bug report
Issue summary:View changes
Status:Needs review» Reviewed & tested by the community

Saying we have existing Views integration for datetime is just patently false. I've manually tested this patch, and @jhedstrom has apparently been using it for months. There is extensive test coverage added, and it matches very closely with the D7 date.module integration.

Therefore, I consider this RTBC.

dawehner’s picture

Status:Reviewed & tested by the community» Needs work

I'm sorry

  1. +++ b/core/modules/datetime/datetime.views.inc
    @@ -0,0 +1,47 @@
    +function datetime_field_views_data(FieldStorageConfigInterface $field_storage) {
    +  $data = views_field_default_views_data($field_storage);
    +  foreach ($data as $table_name => $table_data) {

    Its a wrong assumption that we don't store that field on base tables.

  2. +++ b/core/modules/datetime/src/Plugin/views/argument/Date.php
    @@ -0,0 +1,46 @@
    + */
    +namespace Drupal\datetime\Plugin\views\Argument;

    here should be a new line in between.

  3. +++ b/core/modules/datetime/src/Plugin/views/argument/Date.php
    @@ -0,0 +1,46 @@
    +  /**
    +   * Override to account for dates stored as strings.
    +   */
    +  public function getDateField() {
    ...
    +  /**
    +   * Override to account for dates stored as strings.
    +   *
    +   * {@inheritdoc}
    +   */
    +  public function getDateFormat($format) {

    Should we synchronize those comments a bit?

  4. +++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
    @@ -0,0 +1,67 @@
    +    $a = $this->query->getDateFormat("'" . format_date($a, 'custom', 'c') . "'", static::$dateFormat, TRUE);
    +    $b = $this->query->getDateFormat("'" . format_date($b, 'custom', 'c') . "'", static::$dateFormat, TRUE);
    ...
    +    $value = $this->query->getDateFormat("'" . format_date($value, 'custom', 'c') . "'", static::$dateFormat, TRUE);

    Can we use \Drupal::service('date.formatter')->format()

dawehner’s picture

Let's skip the first part of the complain and open a follow up.

tim.plunkett’s picture

Status:Needs work» Needs review
StatusFileSize
new32.51 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,532 pass(es).
[ View ]
new5.38 KB

Opened #2489476: Provide Views integration for datetime fields used as base fields per discussion at drupalcon.
Fixed the other points.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Thank you tim!

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/datetime/src/Plugin/views/argument/DayDate.php
    @@ -0,0 +1,22 @@
    + * Argument handler for a day (DD)
    ...
    +  protected $argFormat = 'd';

    Missing fullstop and I'm confused as to why (DD) when the argFormat is 'd'. Perhaps the best thing to do would be to remove (DD).

  2. +++ b/core/modules/datetime/src/Plugin/views/argument/MonthDate.php
    @@ -0,0 +1,22 @@
    + * Argument handler for a month (MM)
    ...
    +  protected $argFormat = 'm';

    +++ b/core/modules/datetime/src/Plugin/views/argument/YearDate.php
    @@ -0,0 +1,22 @@
    + * Argument handler for a year (CCYY)
    ...
    +  protected $argFormat = 'Y';

    As above

  3. +++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
    @@ -0,0 +1,106 @@
    +use Drupal\Core\Form\FormStateInterface;

    Not used.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
new31.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,552 pass(es).
[ View ]

This should address #72.

jhedstrom’s picture

Status:Needs review» Needs work

#73 is the wrong patch. Disregard.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
new32.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,552 pass(es).
[ View ]

This is the proper patch with fixes from #72.

jhedstrom’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC (assuming the above goes green) since the changes were just cosmetic.

jhedstrom’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new605 bytes
new32.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,219 pass(es).
[ View ]

Oops, missed one item from #72.

jhedstrom’s picture

Status:Needs review» Reviewed & tested by the community

Tentatively back to RTBC assuming green.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 77: date-views-integration-1838242-77.patch, failed testing.

Status:Needs work» Needs review
jhedstrom’s picture

Status:Needs review» Reviewed & tested by the community

Random testbot fail--back to RTBC.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 26b7df5 and pushed to 8.0.x. Thanks!

  • alexpott committed 26b7df5 on 8.0.x
    Issue #1838242 by jhedstrom, pivica, tim.plunkett, GaëlG, dawehner,...
webchick’s picture

WOOHOO! Thanks for sticking with this one, jhedstrom!!

alexpott’s picture

Status:Fixed» Needs work

I've reverted due to multiple failures seen in Drupal\datetime\Tests\Views\FilterDateTimeTest.

Specifically:

* Drupal\datetime\Tests\Views\FilterDateTimeTest (10 pass(es), 4 fail(s), and 0 exception(s))
   - [fail] [Other] "Actual result
array (
)
is  identical to expected
array (
  0 =>
  array (
    'nid' => '4',
  ),
)
" in FilterDateTimeTest.php on line 72 of Drupal\datetime\Tests\Views\FilterDateTimeTest->_testOffset().
   - [fail] [Other] "Actual result
array (
)
is  identical to expected
array (
  0 =>
  array (
    'nid' => '4',
  ),
)
" in FilterDateTimeTest.php on line 86 of Drupal\datetime\Tests\Views\FilterDateTimeTest->_testOffset().
   - [fail] [Other] "Actual result
array (
)
is  identical to expected
array (
  0 =>
  array (
    'nid' => '4',
  ),
)
" in FilterDateTimeTest.php on line 72 of Drupal\datetime\Tests\Views\FilterDateTimeTest->_testOffset().
   - [fail] [Other] "Actual result
array (
)
is  identical to expected
array (
  0 =>
  array (
    'nid' => '4',
  ),
)
" in FilterDateTimeTest.php on line 86 of Drupal\datetime\Tests\Views\FilterDateTimeTest->_testOffset().

Darn.

  • alexpott committed 3fd94bd on 8.0.x
    Revert "Issue #1838242 by jhedstrom, pivica, tim.plunkett, GaëlG,...
jhedstrom’s picture

These fails are in the offset test, and while I can't reproduce them locally, I think I can fix the test so it isn't so fragile (I think on really slow test runs, the filter fails because the nodes are too old for the filter to work).

jhedstrom’s picture

Actually, the test is already setting a date 1 day in the future, so unless 24 hours are passing between when the setUp method is called, and when the test runs, I don't think that's the issue.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new843 bytes
new32.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,301 pass(es).
[ View ]

I checked how the offset test in the views numeric date filter works, and it is identical except it uses time() instead of REQUEST_TIME, so I've made that change here.

jhedstrom’s picture

StatusFileSize
new1.69 KB
new32.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,308 pass(es).
[ View ]

After thinking about this more, I've realized the intermittent failures were due to timezones! The test data was only storing date, not date and time, so under certain circumstances, the offset query would fail to detect a future date. This patch adds time storage to the test date, which should resolve the seemingly-random failures.

jhedstrom’s picture

Timezones aren't the actual issue--rather as the previous versions of the patch were run near midnight on the testbot, since the time part wasn't stored, the queries would fail to find any data.

For example, imagine the node is created with field_date = 2015-05-26 when the actual time is 2015-05-25T23:59:59--the query is then run at 2015-05-26T00:00:01, where a +1 day offset would look for values of 2015-05-27, so our test node was not found.

Status:Needs review» Needs work

The last submitted patch, 90: date-views-integration-1838242-90.patch, failed testing.

Status:Needs work» Needs review
dawehner’s picture

Oh interesting research on the failures.
Ideally I would like to see @mpdonadio to have a look at them, he did some really good work on similar stuff recently.

olli’s picture

Confirmed that #89 failed at 23:32, #90 passed at 23:33. Difference is that #89 set only date, #90 sets date and time.

olli’s picture

I tried to test the filter manually and I think there's a problem with timezones.

1. Add field_date to article
2. Created an article and set the date to 22:30
3. Added field_date to admin/content as a field and exposed filter with "between" operator
4. Went to admin/content and date was displayed correctly
5. Typed 22:00 and 23:00 for values in the filter, and got no results
6. Typed 19:00 and 20:00, and now I see my article

In table node__field_date the value is set as 2015-05-30T19:30:00.

Is this only me or something with the patch?

mpdonadio’s picture

Status:Needs review» Needs work
+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -0,0 +1,105 @@
+  /**
+   * Override parent method, which deals with dates as integers.
+   */
+  protected function opBetween($field) {
+    $origin = ($this->value['type'] == 'offset') ? REQUEST_TIME : 0;
+    $a = intval(strtotime($this->value['min'], $origin));
+    $b = intval(strtotime($this->value['max'], $origin));
+

The request stack should be injected, and the request time should be retrieved from the current request.

Regarding spurious fails, I pulled my hair out over them in #2399211: Support all options from views fields in DateTime formatters. Take a look at that. The tests should be setting up a timezone in the setUp, so TestBot and local tests behave the same. Also, I am not seeing any calls to datetime_date_default_time() anywhere. Date-only objects should be created in UTC have this called on them, so the time portion gets set to noon. This eliminates timezone problems when you use them (noon UTC is the same day everywhere). That issue also has links out to a bug I found last night and to a discussion to add timezone setup to WebTestBase.

mpdonadio’s picture

+++ b/core/modules/datetime/src/Tests/Views/FilterDateTimeTest.php
@@ -0,0 +1,153 @@
+      // For the offset test, put a date 24 hours in the future. The `time()`
...
+      \Drupal::service('date.formatter')->format(time() + 86400, 'custom', DATETIME_DATETIME_STORAGE_FORMAT),
+    );

I think if you do the datetime_date_default_time() thing on the dates, that you can go back to REQUEST_TIME or the injected request object. This just seems like a ticking time bomb.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new851 bytes
new32.72 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,302 pass(es).
[ View ]

The request stack should be injected, and the request time should be retrieved from the current request.

I don't think we need to do that here--the REQUEST_TIME is also set using $_SERVER['REQUEST_TIME'], which is the same as that in the request object.

Regarding the timezone issues, that was a red herring as to the failures here, so let's follow that up in a separate issue (it would be more broadly applicable to the date field, etc).

This removes the un-needed switch to time() as mentioned in #98.

jhedstrom’s picture

Assigned:jhedstrom» Unassigned
+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -0,0 +1,105 @@
+    $a = $this->query->getDateFormat("'" . $this->dateFormatter->format($a, 'custom', 'c') . "'", static::$dateFormat, TRUE);
+    $b = $this->query->getDateFormat("'" . $this->dateFormatter->format($b, 'custom', 'c') . "'", static::$dateFormat, TRUE);
...
+    $value = $this->query->getDateFormat("'" . $this->dateFormatter->format($value, 'custom', 'c') . "'", static::$dateFormat, TRUE);

Thinking more about the TZ issue in #95, I think actually in this code we want to pass in the timezone not of the site, but of the server, since that's how the dates are stored in the db, and the formatted date used here is part of the db query.

jhedstrom’s picture

StatusFileSize
new5.31 KB
new33.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,579 pass(es).
[ View ]
new34.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,560 pass(es).
[ View ]

This should resolve the issue Olli noticed in #96. (Essentially the changes I outlined in #100, plus a test). The test only patch is against the previous patch, but without the fix included.

olli’s picture

Status:Needs review» Reviewed & tested by the community

Manually tested that the latest patch fixes #96. Great work @jhedstrom!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 101: date-views-integration-1838242-101.patch, failed testing.

Status:Needs work» Needs review
jhedstrom’s picture

StatusFileSize
new1.16 KB
new33.97 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,582 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new34.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,584 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

The test that was supposed to fail didn't because I think the testrunners are on UTC. This change hard-codes a non-UTC timezone for the test.

The last submitted patch, 105: date-views-integration-1838242-105-TEST-ONLY.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 105: date-views-integration-1838242-105.patch, failed testing.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new548 bytes
new34.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,610 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new34.31 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,717 pass(es).
[ View ]

Ok, previous test didn't actually set the timezone. This time!

Test-only patch is still against #99.

The last submitted patch, 108: date-views-integration-1838242-108-TEST-ONLY.patch, failed testing.

jhedstrom’s picture

Status:Needs review» Reviewed & tested by the community

Only minor changes to the test since #102, tentatively bouncing back to RTBC.

To summarize the changes since this was previously committed:

  • Random failures were due to storing date only, rather than date and time. This caused tests that spanned midnight on the testbot to fail. See example in #91
  • Olli discovered timezone issues in #96. These are now resolved (and have tests to boot)
alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Thanks for working out what caused the random fails - and we;ve another bug whilst we're at it - awesome!

Let's try again... Committed 4a4ee05 and pushed to 8.0.x. Thanks!

  • alexpott committed 4a4ee05 on 8.0.x
    Issue #1838242 by jhedstrom, pivica, tim.plunkett, GaëlG, dawehner, olli...
bzrudi71’s picture

Just back from my holiday I noticed that this causes continuous test fails in the datetime test group for SQLite (see SQLite bot).

Drupal\datetime\Tests\Views\FilterDateTimeTest                 1 passes   7 fails                           
Drupal\datetime\Tests\Views\SortDateTimeTest                   2 passes   2 fails                           
Drupal\datetime\Tests\Views\ArgumentDateTimeTest          4 passes   8 fails
alexpott’s picture

Thanks for finding this @bzrudi71. Let's open a new issue to fix this.

alexpott’s picture

Status:Fixed» Needs work

This patch introduced the $string_date to getDateFormat.

+++ b/core/modules/views/src/Plugin/views/query/QueryPluginBase.php
@@ -237,12 +237,15 @@ public function setupTimezone() {
    *   An appropriate query expression pointing to the date field.
    * @param string $format
    *   A format string for the result, like 'Y-m-d H:i:s'.
+   * @param boolean $string_date
+   *   For certain databases, date format functions vary depending on string or
+   *   numeric storage.
    *
    * @return string
    *   A string representing the field formatted as a date in the format
    *   specified by $format.
    */
-  public function getDateFormat($field, $format) {
+  public function getDateFormat($field, $format, $string_date = FALSE) {
     return $field;
   }

+++ b/core/modules/views/src/Plugin/views/query/Sql.php
@@ -1748,9 +1748,9 @@ public function setupTimezone() {
   /**
-   * Overrides \Drupal\views\Plugin\views\query\QueryPluginBase::getDateFormat().
+   * {@inheritdoc}
    */
-  public function getDateFormat($field, $format) {
+  public function getDateFormat($field, $format, $string_date = FALSE) {

Unfortunately this is not reliable as there are several calls to getDateFormat where it is not provided. For example in Drupal\views\Plugin\views\HandlerBase::getDateFormat(). In order for Postgres and SQLite to work as expected we need this information :(. Prior to this patch SQLite was at zero fails and the only reason it is not a supported test environment is because new DrupalCI has not yet been delivered.

Regardless of whether SQLite or Postgres is supported $string_date not being correct is a critical bug introduced by this patch therefore I've decided to revert this patch after doing investigative work in #2502343: New SQLite failures in vewis due to data handling.

  • alexpott committed 9ae6474 on 8.0.x
    Revert "Issue #1838242 by jhedstrom, pivica, tim.plunkett, GaëlG,...
mpdonadio’s picture

Can/should $string_format be added to \Drupal\Core\Database\Connection as a static $dateStringFormat (I can't remember if PHP support abstract constants), and then each implementation can set it, which then can be used where needed?

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new1.98 KB
new35.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,267 pass(es).
[ View ]

Interestingly enough, the $string_date parameter was introduced for postgres rather than MySQL (but not retested once the sort handler was added). This patch gets things working again on postgres and SQLite. I've tested MySQL. SQLite, and Postgres locally and all are passing.

jhedstrom’s picture

Re #117 Views already has special handling for dates in Drupal\views\Plugin\views\query\Sql, so probably no need to propagate that up the the connection level at this time.

alexpott’s picture

Unfortunately I think this is more complex that that... since date plugins are being used on field which are a timestamp.

For example:

    $data['node_field_data']['created_fulldate'] = array(
      'title' => t('Created date'),
      'help' => t('Date in the form of CCYYMMDD.'),
      'argument' => array(
        'field' => 'created',
        'id' => 'date_fulldate',
      ),
    );

in \Drupal\node\NodeViewsData

And it's definition is defined as:

    $fields['created'] = BaseFieldDefinition::create('created')
      ->setLabel(t('Authored on'))
      ->setDescription(t('The time that the node was created.'))
      ->setRevisionable(TRUE)
      ->setTranslatable(TRUE)
      ->setDisplayOptions('view', array(
        'label' => 'hidden',
        'type' => 'timestamp',
        'weight' => 0,
      ))
      ->setDisplayOptions('form', array(
        'type' => 'datetime_timestamp',
        'weight' => 10,
      ))
      ->setDisplayConfigurable('form', TRUE);

In Drupal\node\Entity\Node - which uses Drupal\Core\Entity\Plugin\Field\FieldType\CreatedItem which is an integer. I think whether or not we're dealing with a string date or a numeric date needs to be pushed into the plugin - not sure when it could work this out. So that all date plugins have a chance of working on all date fields.

tim.plunkett’s picture

Reminds me of #1145976: Make date handlers easily identifiable (not actually going to help here, I just remember it being similar)

jhedstrom’s picture

re #120 this doesn't touch the display plugin though, and id = date_fulldate will use the existing views numeric plugin, rather than these?

alexpott’s picture

+++ b/core/modules/datetime/src/Plugin/views/argument/MonthDate.php
@@ -0,0 +1,22 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\datetime\Plugin\views\argument\MonthDate.
+ */
+
+namespace Drupal\datetime\Plugin\views\argument;
+
+/**
+ * Argument handler for a month.
+ *
+ * @ViewsArgument("datetime_month")
+ */
+class MonthDate extends Date {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected $argFormat = 'm';
+
+}

re #122 I think that FullDate plugin is a argument plugin, same as this. Will this work with SQLite?

jhedstrom’s picture

+++ b/core/modules/datetime/src/Plugin/views/argument/Date.php
@@ -0,0 +1,45 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function getDateFormat($format) {
+    // Pass in the string-field option.
+    return $this->query->getDateFormat($this->getDateField(), $format, TRUE);
+  }

This is what sets the string date parameter in the argument handler (the day/month/year handlers for datetime extend this one), so with the changes to views' Sql class, this does now work with SQLite (there's tests that cover the new argument handler too).

jhedstrom’s picture

Perhaps if the string date argument were converted to a class property it would better-document the behavior?

jhedstrom’s picture

Er, scratch that... The query object is used across all fields, which could involve both string dates and numeric dates.

jhedstrom’s picture

I think that FullDate plugin is a argument plugin, same as this. Will this work with SQLite?

Yes, the $argFormat = 'm' maps directly to the SQLite format in \Drupal\views\Plugin\views\query\Sql::getDateFormat():

<?php
...
     case
'sqlite':
       
$replace = array(
         
'Y' => '%Y',
         
// No format for 2 digit year number.
         
'y' => '%Y',
         
// No format for 3 letter month name.
         
'M' => '%m',
         
'm' => '%m',
?>

and then with the new parameter <$string_date> which defaults to false, numeric dates continue to function as they do now, and string dates in sqlite skip the unixepoch bit which broke tests in sqlite above.

As mentioned above, since the new Drupal\datetime\Plugin\views\argument\Date is the only plugin that will call getDateFormat() with $string_date set to TRUE, the new parameter will only impact string dates.

kclarkson’s picture

Status:Needs review» Reviewed & tested by the community

Patch 118 applied cleanly to the drupal 8.0.0-beta11

After the patch I am able to filter my view by an end date field. With filter of greater than "now".

Perfect for creating Upcoming or Past Events. Can we get some more People to Review?

daften’s picture

Status:Reviewed & tested by the community» Needs work

Doesn't work completely.
For fields that are date only (no time), it doesn't seem to work properly, because it also takes a time (which can't be entered then) into account.
There is also a small formatting mistake it seems, because in the filter on view edit it shows as e.g. (> now).

jhedstrom’s picture

@daften can you clarify what isn't working with just date fields? There are tests that use dates, and tests that use datetime.

jhedstrom’s picture

Also, the UI is identical to numeric dates for Views, so if there are formatting issues, those most likely need to be in a follow-up issue that addresses both.

mpdonadio’s picture

arlinsandbulte’s picture

I've been out of the loop for a long time, but I keep an eye on much of the 'date' stuff.

Regarding #130, I'm pretty sure ALL date fields include time information, even if the data field granularity does not include time (it is date only).
Thus IIRC, date fields without time, actually have time set to 00:00 (12am midnight) in the stored date field value, which is not displayed, but may be used in some date manipulations.

daften’s picture

I've tried it out quickly again on simplytest (Simplytest direct url for patch #118).

  • First adapt the content type Page so it has a date field with date type "Date only", Default "Current Date".
  • Then create content of type Page one with date on yesterday, one with date today, one with date tomorrow
  • Create a view for content type page with an extra filter with date field greater than or equal to now
  • The view should show all content for today and tomorrow, it only shows content for today

So granularity is not taken into account it seems. Like @arlinsandbulte says, all date fields include time information, but that shouldn't be taken into account for the comparison in the view IMO.

mpdonadio’s picture

Date-only fields in the database are stored at 00:00:00+00:00 (midnight UTC).

I suspect (but can't be certain) that there may be a timezone problem here. TestBot runs as UTC, which can mask some problems. A worthwhile check would be to set an explicit timezone in the test and see if there are failures.

jhedstrom’s picture

Date-only fields are stored simply as date:

<?php
/**
 * Defines the format that date and time should be stored in.
 */
const DATETIME_DATETIME_STORAGE_FORMAT = 'Y-m-d\TH:i:s';

/**
 * Defines the format that dates should be stored in.
 */
const DATETIME_DATE_STORAGE_FORMAT = 'Y-m-d';
?>
jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new4.44 KB
new36.07 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,324 pass(es).
[ View ]

This should address the issue detailed in #135. Thanks for digging into this folks. Reviews much appreciated!

mpdonadio’s picture

Sorry, yes, @jhedstrom is correct about the storage; I got something confused in my head. When it comes out as a DrupalDateTime object, the time gets set to midnight UTC, until you call datetime_date_default_time() on it. This is one of the quirks in date handling...

mpdonadio’s picture

Status:Needs review» Needs work

I want to look at the tests more closely. We should probably have a case that would have caught #135, or am I missing something?

  1. +++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
    @@ -0,0 +1,119 @@
    +/**
    + * @file
    + * Contains \Drupal\datetime\Plugin\views\filter\String.
    + */
    +

    Should be \Drupal\datetime\Plugin\views\filter\Date

  2. +++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
    @@ -0,0 +1,119 @@
    +  protected $dateFormat = 'Y-m-d H:i:s';

    It's a shame we can't use the constant from the module here.

  3. +++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
    @@ -0,0 +1,119 @@
    +      $this->dateFormat = 'Y-m-d';

    Torn on this. We can't use the module constant above, so using it here would be weird.

  4. +++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
    @@ -0,0 +1,119 @@
    +  protected function opBetween($field) {
    +    $origin = ($this->value['type'] == 'offset') ? REQUEST_TIME : 0;
    +    $a = intval(strtotime($this->value['min'], $origin));

    Do we want to inject the RequestStack so we can get this from the current request instead? I would kinda prefer this, and it would also help with unit tests (if we decide to add them).

  5. +++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
    @@ -0,0 +1,119 @@
    +    // This is safe because we are manually scrubbing the value.
    +    $field = $this->query->getDateFormat($field, $this->dateFormat, TRUE);
    +    $this->query->addWhereExpression($this->options['group'], "$field $this->operator $value");

    Just want to point out that a similar thing is done with the timestamp filters in the views module itself, and I don't see a problem with it.

  6. +++ b/core/modules/datetime/src/Tests/Views/ArgumentDateTimeTest.php
    @@ -0,0 +1,141 @@
    +class ArgumentDateTimeTest extends DateTimeHandlerTestBase {

    +++ b/core/modules/views/src/Plugin/views/query/Sql.php
    @@ -1797,7 +1797,13 @@ public function getDateFormat($field, $format) {
    +        // In order to allow for partials (eg, only the year), transform to a
    +        // date, back to a string again.
    +        // @todo this is very messy, and EXTRACT should probably be used.
    +        return "TO_CHAR(TO_TIMESTAMP($field, 'YYYY-MM-DD HH24:MI:SS'), '$format')";

    I think there is a policy in place to not commit anything with a @todo that doesn't correspond to a followup. Not totally sure this is worth a followup? Can we fix it here?

Jaesin’s picture

@jhedstrom thanks for your persistence with this issue.

I tested the patch from #138 as instructed in #135. The filter works as expected if you use >= "TODAY" but not >= "NOW". One might expect a date of the current day to be equal to "NOW" but at least there is a workaround.

I want to add that I think that a numeric date argument plugin is essential. Is the plan to work that out after the filter seems solid enough to commit?

jhedstrom’s picture

@Jaesin the input for relative time offsets uses these allowed formats: https://secure.php.net/manual/en/datetime.formats.relative.php What's really odd is that 'now' is allowed, but according to those docs "is simply ignored"...

Since the numeric filters have the same input form, rather than change the help text here, we could do that in a follow-up that addressed both numeric and string date handlers.

jhedstrom’s picture

Er, actually, it makes sense that 'now' is ignored, because strtotime('now', $now) should always just return whatever $now is set to.

I'm writing tests now that use 'now' for offset views of date-only fields.

jhedstrom’s picture

Status:Needs work» Needs review
StatusFileSize
new8.87 KB
new40.32 KB
PASSED: [[]]: [] 98,145 pass(es).
[ View ]

This should address #140.

@Jaesin in both manual testing, and in the new automated tests added here, both 'today' and 'now' are working as expected for me.

jhedstrom’s picture

StatusFileSize
new171.4 KB
new175.18 KB

And for the sake of completeness, here's the test runs from sqlite and pgsql.

mpdonadio’s picture

The new test coverage looks good, but ...

With manual testing, <= today and <= now both work. >= today works, but >= now doesn't (only pulls up future date). Oddly enough, the timestamp plugins do the exact same thing.

And even odder, if I change FilterDateTest to use 'today' instead of 'now', then it fails on lines 85 and 98.

Could this be because `new DateTime('now')` uses the current time and `new DateTime('today')` uses midnight? I was going to suggest mapping 'now' to 'today' for date-only fields, but that doesn't describe the fails when I hacked the test.

Not changing the status since I am confused on what is really happening here.

jhedstrom’s picture

I can not reproduce the manual test results @mpdonadio reports above. For date-only fields, >= now and >= today both work the same. The test performs this same query too. For date+time fields, today always sets the time to midnight of the current date (00:00:00), while now uses the current date and time, thus they will behave differently.

mpdonadio’s picture

Status:Needs review» Reviewed & tested by the community

I re-ran my tests and think this is good to go. I am pretty sure my observations in #146 were because I misconfigured the field to be date+time.

bojanz’s picture

StatusFileSize
new39.86 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,328 pass(es).
[ View ]

array() is dead, usage of the short array syntax is highly recommended (not sure if we mandate it yet) for all new code.

Here's a reroll to address that.

bojanz’s picture

StatusFileSize
new18.4 KB

My interdiff got swallowed.

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

@bojanz actually atm we have no coding standard around which array syntax to use - only that we should try to be consistent within a method / function.

Third time lucky?

Committed 24219fe and pushed to 8.0.x. Thanks!

  • alexpott committed 24219fe on 8.0.x
    Issue #1838242 by jhedstrom, pivica, tim.plunkett, bojanz, GaëlG,...

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.