Exposed date filters 'empty' and 'not empty' are broken

I can reproduce this with a Date field but not a Timestamp field, so moving this to the Datetime module for now.

Also different is that when using the Date filter the 'value' column for the grouped filter isn't cleared when you select 'is empty', that does happen for the timestamp field. No idea if this is related.

Downgrading this, not sure if it truly classifies as a major.

Think we need a test-only patch to demonstrate the problem (or is someone wants to attach an exported view).

Wondering if this is related to that Notice issue in views+datetime that I can't locate right now.

Here is a test for this. Also added a test to show that it works fine if you don't expose it.

Since just exposing it when using the 'empty' or 'not empty' operator and not using a group makes very little practical sense, I don't think we need a test for that (I wouldn't even know what you would expect to see).

No idea what the fix should be at the moment.

Confirming the problem for Drupal 8.3.7 and 8.4.0-rc1.

This patch fixes the problem.

+++ b/core/modules/views/src/Plugin/views/filter/Date.php
@@ -152,7 +152,7 @@ class Date extends NumericFilter {
-    else {
+    else if ($operators[$operator]['values'] == 2) {
       if ($this->value['min'] == '' || $this->value['max'] == '') {

Extreme nit, but `elseif` is the Drupal coding standard.

This should be merged with the test in #4, but this does looks correct per the similar logic in Date::validateValidTime(), and NumericFilter::acceptExposedInput().

NW for the test.

Merged with #4 and changed to elseif.

index 2a8d7cd..e9071b7 100644
--- a/core/modules/datetime/src/Tests/Views/FilterDateTest.php

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

+++ b/core/modules/datetime/src/Tests/Views/FilterDateTest.php
@@ -18,6 +18,11 @@ class FilterDateTest extends DateTimeHandlerTestBase {

#2865992: Convert Datetime module Views tests to Kerneltest changed FilterDateTest from a functional test to a kernel test, so the patch in #4 needs to live somewhere else. Probably needs to be a new functional (ie, BrowserTestBase) test in the Drupal\Tests\datetime\Functional\Views namespace. Or, the final bit of the test needs to be converted to a kernel test.

changed FilterDateTest from a functional test to a kernel test, so the patch in #4 needs to live somewhere else. Probably needs to be a new functional (ie, BrowserTestBase) test in the Drupal\Tests\datetime\Functional\Views namespace. Or, the final bit of the test needs to be converted to a kernel test.

Who can provide a patch for FilterDateTest? It seems to me that I don't fully understand the kernel test architecture. Specifically NodeCreationTrait does not work out of the box because it has filter.module dependencies. Also, I can't use $this->drupalPostForm() anymore.

@organicwire in a kernel test just use NodeType::create() and Node::create() and not the Trait. Using $this->drupalPostForm() would require a browser, which kernel test doesn't have.

So like @mpdonadio said in #15

Probably needs to be a new functional (ie, BrowserTestBase) test in the Drupal\Tests\datetime\Functional\Views namespace.

Create a new functional test for this and move the code I wrote in #4 to the new test is probably the best way to go. We want this to be a functional test, since we really want to test that this is rendered correctly.

So date filters still don't ok. Fine!
Let's wait another decade until 1000 lines of tests are magically written for a 1-line change.

Moved the kernel bits to the kernel test and cobbled together a rough functional test (needs cleanup). Not readily apparent what I botched.

Added the @group and stopped using the preview (which has a problem with exposed filters), and tested a real page. Green locally now.

Oops forgot to upload the interdiff, here we go.

Duh..

Going to fix a few nits tonight, and do a proper IS.

Picked some phpcs nits.

After we get an RTBC we will have to do a 8.4.x version b/c #2826404: Create DateTimeItemInterface and deprecate global constants in datetime.module and possibly #2627512: Datetime Views plugins don't support timezones (didn't look closely where the merge conflicts were).

Adding the 8.4.x version here.

Wrote a bit too much of this to feel 100% comfortable RTBC'ing this but I feel this is ready.

This should fix the 8.4 fail, and here is a re-upload of the 8.5.x version renamed.

#30 worked great thanks :) on 8.4.x

Just a nit here.

+++ b/core/modules/views/src/Plugin/views/filter/Date.php
@@ -152,8 +152,8 @@ public function acceptExposedInput($input) {
+    elseif ($operators[$operator]['values'] === 2) {
+      if ($this->value['min'] === '' || $this->value['max'] === '') {

Could we add a code comment here to explain what's going on? It isn't clear why if the operator has 2 values we care about min and max being empty strings.

Aside from that, this looks great and RTBC.

Setting this to needs work based on #33.

How about this?

I've re-queued the test, but assuming it comes back green, I think this is RTBC.

1. +++ b/core/modules/datetime/tests/src/Functional/Views/FilterDateTest.php
   @@ -0,0 +1,148 @@
   +  protected static $field_name = 'field_date';
   ...
   +  protected static $date;
   ...
   +    static::$date = \Drupal::time()->getRequestTime();
   

   any reason why these are static?

2. +++ b/core/modules/datetime/tests/src/Functional/Views/FilterDateTest.php
   @@ -0,0 +1,148 @@
   +  public static $modules = ['datetime', 'datetime_test', 'node', 'views', 'views_ui'];
   

   nit, we should wrap these, > 80

I tested #35, it seems to be working fine for us. 👍

Addressed points from #38.

+++ b/core/modules/datetime/tests/src/Functional/Views/FilterDateTest.php
@@ -0,0 +1,154 @@
+
+    // Set to 'today'.
+    $this->requestTime = \Drupal::time()->getRequestTime();
+
...
+    // Create some nodes.
+    $dates = [
+      // Tomorrow.
...
+      // Today.
+      \Drupal::service('date.formatter')->format($this->requestTime, 'custom', DateTimeItemInterface::DATE_STORAGE_FORMAT, DateTimeItemInterface::STORAGE_TIMEZONE),
+      // Yesterday.
+      \Drupal::service('date.formatter')->format($this->requestTime - 86400, 'custom', DateTimeItemInterface::DATE_STORAGE_FORMAT, DateTimeItemInterface::STORAGE_TIMEZONE),
+    ];

I still find it strange to have this as a property at all, we only use it during setup, also strange comment, it's not today, it's now.

What I would do is something like this:

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

And then 3x a version of:

DrupalDateTime::createFromTimestamp($now + 86400, DateTimeItemInterface::STORAGE_TIMEZONE)->format(DateTimeItemInterface::DATE_STORAGE_FORMAT);

Then the variable name and so on is pretty self-explanatory, no property needed and it's shorter.

(the custom argument for format() should IMHO be deprecated/discouraged, just use ->format() directly, what happens internally is that we create a DateTime object again and call format() on it.

Addressed #40. Thanks for the review @Berdir!

We are using this patch with #2648950: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter and it looks like it broke our tests with a date range field.

Steps to reproduce:

• Apply #2648950.181 patch
• Add a date range field to a content type
• Add two exposed filters based on date range field properties: start date + end date
• Accessing the view with no filters applied will use "today" as start date and end date values which limits your default screen to today's results instead of not applying date filters (since those are empty)

In the mentioned use-case, $this->value['value'] has NULL value which does not work with strict comparison from before $this->value['value'] === ''.

The attached patch should fix described use-case as well as the original problem from the issue summary.

I'm not sure how to write a test since it only happens in combination with #2648950: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter...

I don't think there is a need to have those checks type safe. The feedback about the test has been addressed.

Let's not introduce a new pattern into views filter plugins by using a switch statement it poses more questions than it answers for me. Like what should the default action be and what are the other values possible - this thing can equal 0.

Also fixed a code style issue in one of the tests.

Credited @jhedstrom, @Berdir, @larowlan for review comments
Credited @bobemoe for creating the issue.

Committed and pushed 8e3bcf48ee to 8.7.x and 442ddf11f0 to 8.6.x. Thanks!

I think this change is causing an issue for me. I am using date field in views for my events. When the date field is empty it outputs today's date. I don't want it to output anything.