Date/time Views filter tries strotime() relative to Unix epoch

Thanks for the bug report and the patch.

This needs an IS update to use the template, and it also needs a test to demonstrate the bug and that the patch fixes it (this is the primary reason for the NW) . The fact that we remove a conditional check and the patch comes up green troubles me (it suggests we lack test coverage on something).

This code was introduced in #1838242: Provide Views integration for datetime field, and I am wondering what is really going on here, as we had some explicit discussions and tests (both automated and manual) regarding using "today" and "now". FilterDateTest should cover this scenario.

Regarding #3, that looks unrelated to the bug at hand, so let's table that for now (we can do a followup against 8.1.x to optimize that, if needed).

Spent a while with this. This is a real bug that wasn't exposed b/c of a lack of test coverage. This patch fixes the issue and the new test should prevent regressions.

+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -95,7 +95,7 @@ public static function create(ContainerInterface $container, array $configuratio
   protected function opBetween($field) {
-    $origin = ($this->value['type'] == 'offset') ? $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') : 0;
+    $origin = $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME');

I think we're missing test coverage of this for relative dates... _testRelative() only adds tests for ::opSimple().

More tests as per #14.

Changed my mind. If this is a bug, then we can get this into 8.1.x. New test coverage looks good, just waiting for a full bot run.

The fails for PostgreSQL is consistent with what HEAD is doing now and is unrelated to anything there, so I think this means we can RTBC this? The new test coverage looks good to me.

Needs reroll b/b #2572793: Fix 'Drupal.WhiteSpace.OperatorSpacing' coding standard. Easy conflict.

$ git checkout 6c73dc1508efb55c4e669f07f52ef7a0e326216c
$ git apply --index 2647292-16_0.patch
$ git rebase origin/8.1.x

First, rewinding head to replay your work on top of it...
Applying: 2647292-16_0.patch
Using index info to reconstruct a base tree...
M	core/modules/datetime/src/Plugin/views/filter/Date.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/datetime/src/Plugin/views/filter/Date.php
CONFLICT (content): Merge conflict in core/modules/datetime/src/Plugin/views/filter/Date.php
Failed to merge in the changes.

$ diff 2647292-16_0.patch 2647292-22.patch 
2c2
< index e53cb1c..6b6b881 100644
---
> index 9fd47fd..6b6b881 100644
18c18
< -    $origin =  (!empty($this->value['type']) && $this->value['type'] == 'offset') ? $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') : 0;
---
> -    $origin = (!empty($this->value['type']) && $this->value['type'] == 'offset') ? $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') : 0;

I just queued this for retesting. The patch is looking good to me.

I queued for re-test again since it's been so long. I think this is RTBC assuming those still go green.

+++ b/core/modules/datetime/src/Plugin/views/filter/Date.php
@@ -90,7 +90,7 @@ public static function create(ContainerInterface $container, array $configuratio
-    $origin = ($this->value['type'] == 'offset') ? $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') : 0;
+    $origin = $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME');

But if the date is 'today' then shouldn't the type be offset? Seems so whilst reading:

        '#options' => array(
          'date' => $this->t('A date in any machine readable format. CCYY-MM-DD HH:MM:SS is preferred.'),
          'offset' => $this->t('An offset from the current time such as "@example1" or "@example2"', array('@example1' => '+1 day', '@example2' => '-2 hours -30 minutes')),
        ),

Yes, 'today' is an 'offset' type (or relative date). `_testRelative()` should be covering all of the possibilities here. Assigning to myself to review this issue, since it is rather old.

I could be wrong, but I think this operation is also meant to cover absolute dates as well, which wouldn't work with this patch (unless I'm missing something). I think we should be using the same logic from Drupal\views\Plugin\views\filter\Date::opSimple(), which is:

$value = intval(strtotime($this->value['value'], 0));
    if (!empty($this->options['value']['type']) && $this->options['value']['type'] == 'offset') {
      $value = '***CURRENT_TIME***' . sprintf('%+d', $value); // keep sign
    }

**CURRENT_TIME** probably won't work here, but the important part is that the type is being accessed through options['value']['type'] rather than relying on $this->value['type'].

I have a spent a long time staring at this, and am truly confused, especially looking at this several months later.

#28, test coverage has explicit dates in it, so I think this is OK. Also, the other filter uses timestamps, where datetime use ISO strings; the ***CURRENT_TIME*** thing is a placeholder that ends up in the query. The datetime handles this in PHP before building the query.

#26, this is weird.

\Drupal\views\Plugin\views\filter\Date has

  protected function opSimple($field) {
    $value = intval(strtotime($this->value['value'], 0));
    if (!empty($this->value['type']) && $this->value['type'] == 'offset') {
      $value = '***CURRENT_TIME***' . sprintf('%+d', $value); // keep sign
    }
    // This is safe because we are manually scrubbing the value.
    // It is necessary to do it this way because $value is a formula when using an offset.
    $this->query->addWhereExpression($this->options['group'], "$field $this->operator $value");
  }

OK, so the date input is parsed with strtotime(), which will attempt to turn anything into a date. The input isn't validated, so you can enter an absolute or relative date in either.

Case 1: Enter an absolute date w/ type=='date', the date gets turned into a timestamp and used directly.
Case 2: Enter a relative/offset date w/ type=='date', the date gets turned into a timestamp and used directly. If you did something w/o a reference, then the offset gets added to 1970/01/01, and used.
Case 3: Enter an absolute date w/ type=='offset', the date gets turned into a timestamp and user directly; with strtotime() when you give it an absolute date, the second parameter gets igored.
Case 4: Enter a relative/offset date w/ type=='offset', and the input gets converted into a second delta from 1970/01/01 and then added the the current time as part of the query. Crude, but OK.

Cases 1 and 4 match the text on the UI, and work as expected. Cases 2 and 3 are really undefined conditions per the text on the UI.

In HEAD in \Drupal\datetime\Plugin\views\filter\Date we have

  protected function opSimple($field) {
    $origin = (!empty($this->value['type']) && $this->value['type'] == 'offset') ? $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME') : 0;
    $value = intval(strtotime($this->value['value'], $origin));

    // Convert to ISO. UTC is used since dates are stored in UTC.
    $value = $this->query->getDateFormat("'" . $this->dateFormatter->format($value, 'custom', DATETIME_DATETIME_STORAGE_FORMAT, DATETIME_STORAGE_TIMEZONE) . "'", $this->dateFormat, TRUE);

    // 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");
  }

Case 1: Enter an absolute date w/ type=='date', the date gets turned into a timestamp and used directly.
Case 2: Enter a relative/offset date w/ type=='date', the date gets turned into a timestamp and used directly. If you did something w/o a reference, then the offset gets based from 1970/01/01, and used.
Case 3: Enter an absolute date w/ type=='offset', the date gets turned into a timestamp and used directly; with strtotime() when you give it an absolute date, the second parameter gets igored.
Case 4: Enter a relative/offset date w/ type=='offset', and the input gets converted into a timestamp using the current time as the reference base.

In all cases, the timestamp gets formatted per storage.

Cases 1 and 4 match the text on the UI, and work as expected. Cases 2 and 3 are really undefined conditions per the text on the UI.

With the patch we have

  protected function opSimple($field) {
    $origin = $this->requestStack->getCurrentRequest()->server->get('REQUEST_TIME');
    $value = intval(strtotime($this->value['value'], $origin));

    // Convert to ISO. UTC is used since dates are stored in UTC.
    $value = $this->query->getDateFormat("'" . $this->dateFormatter->format($value, 'custom', DATETIME_DATETIME_STORAGE_FORMAT, DATETIME_STORAGE_TIMEZONE) . "'", $this->dateFormat, TRUE);

    // 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");
  }

Inadvertently, we removed the type from use in the filter plugin. But.

Case 1: Enter an absolute date w/ type=='date', the date gets turned into a timestamp and used directly.
Case 2: Enter a relative/offset date w/ type=='date', the date gets turned into a timestamp and used directly. If you did something w/o a reference, then the offset gets based from the current date, and used.
Case 3: Enter an absolute date w/ type=='offset', the date gets turned into a timestamp and user directly; with strtotime() when you give it an absolute date, the second parameter gets igored.
Case 4: Enter a relative/offset date w/ type=='offset', and the input gets converted into a timestamp using the current time as the reference base.

In all cases, the timestamp gets formatted per storage.

Cases 1 and 4 match the text on the UI, and work as expected. Cases 2 and 3 are really undefined conditions per the text on the UI.

In other words, it looks like with the unvalidated input matching the help text format, we are OK. See #10.

So, I am back to thinking this is not a code problem, but a UI help text problem on both filters.

?

So, I am back to thinking this is not a code problem, but a UI help text problem on both filters.

Marking as postponed until the IS is updated to reflect the findings in #29.

The current implementation makes it impossible to filter on dates not relative either to the epoch or "now".
If the type is not 'offset' the second argument to strtotime() must not be 0.

For example, I want to show nodes created from yesterday I would assume I could use the absolute mode and put in "Larger than yesterday", but now that actually results in "Larger than -86400" as opposed to "Larger than 1526256000".

The closest I can get while the second argument to strtotime is fixed to 0 is the equivalent of CURRENT_TIME - 86400, which is not what I want as it changes during the day.

Me and @TwoD were trying to solve the issue as stated above and the attached patch is what we landed on to fix this use case. Note that while we changed opBetween to stay consistent, it doesn't fully work as there is no granularity option in the datetime views widget and therefore the current time is always appended. #2868014: Views Date Filter Datetime Granularity Option is the issue for this.

It seems to me that the second argument to strtotime() should never be 0 actually. In D7 we used the views query substitution token ***CURRENT_TIME*** as the second argument of strtotime() which resulted in REQUEST_TIME. In D8 ***CURRENT_TIME*** is not used anymore in strtotime() but as the first number from which we subtract the result of intval(strtotime($this->value['value'], 0)) which doesn't produce the same results of course. As a side effect, even if we try to query for a specific time, it seems not possible. If it's 2019-01-10 14:00pm (UTC), then querying for "-1 day" or "-1 day midnight" will produce the same result: 2019-01-09 14:00pm and not 2019-01-09 12:00am as expected.

Since \Drupal::moduleHandler()->invokeAll('views_query_substitutions', [$this->view]) is executed when the whole query is executed, I would prefer not to copy exactly the code from D7 (invoking views_query_substitutions) but to set directly REQUEST_TIME as a second argument to ensure a consistent timestamp instead.

Attaching one patch with substitutions and with direct use of REQUEST_TIME.

PS: Setting this to Major to get some more attention since it seems to me that it fits the Cause a significant admin- or developer-facing bug with no workaround case - especially for websites showing reports per date. It's also not a datetime.module bug but a views bug affecting the datetime fields.

Based on my previous suggestion I also upload one more patch since I now found out about the deprecation notice of REQUEST_TIME. Take into account though that this patch will fail in Drupal versions < 8.3 where a time service does not exist.

Rereading this issue, especially the comments I made in #29.

We really needs tests here to demonstrate a problem, and I think basing them on the A,B,C,D variants outlined above would be best. We also need to ensure that we do the tests both in views (for timestamps) and datetime is best so that we can ensure consistency between them.

(time goes by while I do some `git blame`)

And, apparently we may have an issue for this already, #1182256: Date filter does not work with dynamic dates, with tests. Looks like this was fixed in 7.x and the port to 8.x wasn't finished.

Ok, merged in tests from #1182256: Date filter does not work with dynamic dates.

Prob want to check the datetime stuff, too.

And apparently, we added a datetime field onto the nodes in FilterDateTest, but don't really use them for anything...

Pass at adding tests for datetime. These are going to fail, but not sure why yet.

Million thx for the patch @mpdonadio. Tested against PHP 7.2 & Drupal 8.7.7

It fixed the problem I was seeing in views trying to filter all content authored between "yesterday" and "today -1 second" but instead with the bug, filtering against "now -24 hours".

What can we do to get this going with a passing patch and included in a future release of Drupal?

I may have spoke too soon...

Looking at the most recent dates, (because things weren't working properly) I noticed the time for:

between
min: today
max: tomorrow -1 second

showed up as: BETWEEN DATE_FORMAT('2019-09-23T19:00:00', '%Y-%m-%d') AND DATE_FORMAT('2019-09-24T18:59:59', '%Y-%m-%d')))

At least now it isn't going off "now", but the time doesn't seem to respect the sites timezone in respect to utc.

I ended up having to set the filter as:

min: yesterday +5 hours
max: today +5 hours -1 second

This correctly gives me:

BETWEEN DATE_FORMAT('2019-09-24T00:00:00', '%Y-%m-%d') AND DATE_FORMAT('2019-09-24T23:59:59', '%Y-%m-%d')))

Update: Ok, I think core fields that are based on "authored on", "updated", etc are probably correctly pulling the right dates, however if you add a date field and capture a specific date, you end up with the wrong UTC spread as shown in my example above.

Core fields appear to use UNIX timestamp where-as the date field uses a human readable timestamp. That may have something to do with it?

Keep this in mind when testing and creating patches that it needs to be applied against the core fields as well as entity date fields.

Ran into #48 yesterday which led me to this issue. I was real confused why "today" was giving me UTC instead of America/New_York like the site is configured for.

Where does this issue stand?

Adding a patch to fix the unequal Mapping of results and expected array.

Adding a patch to fix the unequal Mapping of results and expected array.

I hit this issue when porting a site from Drupal 7 to Drupal 9. It had a view where relative dates like "first day of this month 00:00:00" were used. The view was returning unexpected results and didn't match the D7 site results unless I explicitly gave it a date. Investigating it, I found that the 0 parameter to strtotime() was the source of the issue and this is fixed for me by the above patch. With the patch applied it is now returning the correct results. Thanks!

Adding patch for failing test in #53.

Thanks.

+1 for this change, I had the same use case (and a similar experience) as Stella in #56.

As mentioned before, this was already fixed in the Views project in 2015. At that point in time the 7.x-3.x branch had already been merged into Drupal core, so the issue was marked as "Patch (to be ported)" but unfortunately that never happened. Let's get this reviewed and committed.

I tested manually; The patch still applies to 9.4.x and works as expected. The tests look good to me.

Just one thing:

+    $request_time = \Drupal::time()->getRequestTime();

Shouldn't we use proper dependency injection instead of calling \Drupal::time()?

I have closed #1182256: Date filter does not work with dynamic dates, the original D7 issue about the same problem, as duplicate.

Maintainers, please credit patch contributors in that issue: https://www.drupal.org/u/yasser-samman, https://www.drupal.org/u/shubhamprakash, https://www.drupal.org/u/lendude and https://www.drupal.org/u/jibran.

Moving credit from duplicate

This issue also came up for us on one of our developments using views.

Here's a patch that addresses #60 and adds dependency injection for the time service.

It's ready for review again. Let's see if we can get it committed 😄

Some missing spaces on my docblocks on the latest patch. This one only fixes those.

thanks for the work here we ran into this issue today nice to have a fix.
I read over the patch and it is working in a patched site.

so +1 for an RTBC (but not confident enough to set it myself)

I took a stab at updating the issue summary with some of the comments in #29, which were slightly out-of-date based on further work.

I did not remove the "Needs tests" as it is not clear whether there is test coverage for the 4 cases outlined in #29. If there needs to be more tests, then this should be back to "Needs work" status. If the issue summary and tests are sufficient, then change to RTBC.

This also seems to resolve date filtering with timestamp fields such as "created" and "changed", which without the patch become something like (node_field_data.created BETWEEN 1661358922+1657944000 AND 1661358922+1661313600) in SQL.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Removing tests tag as they appear to be added.

But running them locally without the fix
core/modules/datetime/tests/src/Kernel/Views/FilterDateTest.php actually passes. So this test needs to be updated to cover the issue.

Closed #2628322: Clarify options for 'offset' on date field filters as something we should handle here.

We should update the examples provided by the date field to include an example (or two) of the strings that this change now allows.

Re-rolled for 11.x

I think the issue didn't happen on 11.x, but I also converted patch to git branch for 11.x.
\Drupal\Tests\datetime\Kernel\Views\FilterDateTest::testDateOffsets run without the fix actually passes, the test didn't cover the issue.

I'm seeing this on my server with Drupal 10.3. Time on server is correct. Time in Drupal is an hour out. Set to United Kingdom and UTC.

Please ignore.

Turns out I had selected "Users may set their own time zone" so needed to update timezone from UTC to London there.

Re-based the MR for the latest 11.x-dev