Date filter does not work with dynamic dates

Can you please have a look at see whether it works for you in the dev versions?

patch against 6.x-3.x-dev

patch against 7.x-3.x-dev

Nice catch. I tested your patch and works as expected. However reading the code of views_handler_filter_date.inc I found this:

  function op_simple($field) {
    $value = intval(strtotime($this->value['value'], 0));

And after test the same "first day of last month" I noticed that it doesn't work as expected.

Please provide a patch containing the fix for op_simple too. Thanks.

Changing the base of strtotime is also not right because this would kill a lot of other things.

The filter is not designed for expecting this kind of input if you use "A date in any machine readable format."

Maybe a third value type could be introduced to make it a bit easier to understand.

If this is fixed it should be simpletested as well. Updating the status, so the testbot is runned here.

Atm. I can't imagine a scenario in which I need the base 0 for the relative date calculation. For me it seems 0 is used to be able to work with ***CURRENT_TIME*** for offset values.
But what if we change to code to something like this:

function op_between($field) {
    $query_substitutions = views_views_query_substitutions($this->view);
    $a = intval(strtotime($this->value['min'], $query_substitutions['***CURRENT_TIME***']));
    $b = intval(strtotime($this->value['max'], $query_substitutions['***CURRENT_TIME***']));

That way offset values rely on $query_substitutions['***CURRENT_TIME***'] and in the normal case $query_substitutions['***CURRENT_TIME***'] is REQUEST_TIME - which means a value like now gets the current timestamp and not 0.

After a short chat with Daniel I decided to create a patch and see if all current tests pass ;)
Values like first day of last month / last day of last month should now return a usable timestamp.
Feedback very welcome!

So this doesn't break any existing tests, but it definitely needs new ones before being committed.

I tried to remember why I had to change this but I can't reproduce it anymore :|
The current code will produce {field} > {CURRENT_TIME}+0 for now as long as you define the value as an offset.

   $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->query->add_where_expression($this->options['group'], "$field $this->operator $value");

Only "advantage" of the patch would be a potentially easier to understand construct because it removes to need to handle "offset" values in a special way.
This also means that the UI could be adjusted because there's no need to declare the type of the date.

... but it definitely needs new ones before being committed.

What kind of new tests? As far as I can see the coverage is quite good for this.

I had the same issue, seems to me the bug is because

  strtotime('last mon', 0) 

doesn't return the correct interval

#8 works for me !!

Same problem here trying to filter node created between today and tomorrow. #8 worked for me

Updated patch, no changes other than the addition of some new tests

Patch reroll against latest dev

I tested the patch in #15, and it fixes the issue on both between and simple conditions.

Patch in #15 works here so that "midnight" results in the previous midnight, whatever time of day it is currently, as you'd expect from a php relative date.

I haven't done exhaustive testing, but this seems to be quite an important fix. Without it common relative date strings like "yesterday" and "today" just don't work the way they work in PHP's strtotime().

I've tried to add views filter between two dates: 1) 'Last Monday -1 week', 2) 'Last Monday' to get entities for previous week.

Generic views behaviour calculates for me wrong timestamps. It became shifted and instead of get two mondays return me just thursdays.

I think it is because strtotime uses calendar for 01.01.1970 day and it produces an logical error.
Patch #15 works for me. Thank you a lot!

+++ b/handlers/views_handler_filter_date.inc
@@ -172,10 +170,10 @@ class views_handler_filter_date extends views_handler_filter_numeric {
-      $value = '***CURRENT_TIME***' . sprintf('%+d', $value); // keep sign
-    }
+    // Use the substitutions to ensure a consistent timestamp.
+    $query_substitutions = views_views_query_substitutions($this->view);
+    $value = intval(strtotime($this->value['value'], $query_substitutions['***CURRENT_TIME***']));

I think the ***CURRENT_TIME*** is needed for caching so we can't replace it here, right?

Would be really nice to get this patch into a supported release. The latest Views security release doesn't include this important fix, so the patch has to be re-applied.

This fixes a nasty bug that's difficult to spot and diagnose if you're using relative date filters in Views!

@olli
I think the query cache doesn't change. The following code generates parts of the result key:

        // the query objects.
        if ($build_info[$index] instanceof Select) {
          $query = clone $build_info[$index];
          $query->preExecute();
          $build_info[$index] = (string)$query;
        }

This piece of code triggers the query_alter() hook and based upon that always had the ***CURRENT_TIME*** replace.
This means that those queries are already not cacheable and you better write your own cache plugin, if you need them to be. (I hope all this is true)

Given that this issue solves a bug, let's get it in. It would be great if someone could evaluate whether we need this in 8.0.x as well.

Committed to 7.x-3.x and pushed

Here is the patch without tests.

That part seriously needs tests indeed but the port is looking fine so far!

Added some tests. Interdiff is the test-only patch.

Ran into this issue today. I think the patch needs a re-roll.

Re-roll.
This is kind of an important issue I don't know why it's being ignored.

Missed including views.views_execution.inc file to be able to use views_views_form_substitutions

Thank you @Yasser Samman for the patch re-roll. I applied it and it fixed the issue.

Thanks for working on this! One thing I see though:

+++ b/core/modules/views/src/Plugin/views/filter/Date.php
@@ -166,15 +166,12 @@ public function acceptExposedInput($input) {
+    \Drupal::moduleHandler()->loadInclude('views', 'inc', 'views.views_execution');
+    $query_substitutions = views_views_query_substitutions($this->view);

@@ -182,11 +179,11 @@ protected function opBetween($field) {
+    // Use the substitutions to ensure a consistent timestamp.
+    \Drupal::moduleHandler()->loadInclude('views', 'inc', 'views.views_execution');
+    $query_substitutions = views_views_query_substitutions($this->view);

views_views_query_substitutions() is a hook implementation so we shouldn't be calling that directly.
We should probably \Drupal::moduleHandler()->invokeAll('views_query_substitutions', [$this->view]); (or better yet inject the moduleHandler in the plugin if possible)

Did the changes suggested in #41

@shubham.prakash thanks for the quick pick up, couple of things:

1. +++ b/core/modules/views/src/Plugin/views/filter/Date.php
   @@ -166,15 +166,12 @@ public function acceptExposedInput($input) {
   +    \Drupal::moduleHandler()->loadInclude('views', 'inc', 'views.views_execution');
   
   @@ -182,11 +179,11 @@ protected function opBetween($field) {
   +    \Drupal::moduleHandler()->loadInclude('views', 'inc', 'views.views_execution');
   

   We can lose the includes when using the module handler to invokeAll

2. +++ b/core/modules/views/src/Plugin/views/filter/Date.php
   @@ -166,15 +166,12 @@ public function acceptExposedInput($input) {
   +    $query_substitutions = \Drupal::moduleHandler()->invokeAll('views_query_substitutions', [$this->view]);
   
   @@ -182,11 +179,11 @@ protected function opBetween($field) {
   +    $query_substitutions = \Drupal::moduleHandler()->invokeAll('views_query_substitutions', [$this->view]);
   

   Did you try using Dependency Injection (like I hinted at in #40) by implementing ContainerFactoryPluginInterface on the plugin instead of using \Drupal? If not, that would be the preferred way of doing this, unless there is some reason not to do this.

Thanks @Lendude.
Using dependency injection to be able to use module_handler service.
I just didn't have enough time the last time I did it :p

Ok..
So the test failed because the datetime module is extending our Drupal\views\Plugin\views\filter\Date that we modified. Possibly other modules are extending the same plugin too.
What do you think we should do? Should we keep using \Drupal::moduleHandler() insead of dependency injection for the time being?

Closing this in favor of #2647292: Date/time Views filter tries strotime() relative to Unix epoch. The solution in that issue is almost identical to the patch here. Reviews are appreciated.