Add extra test coverage for operators of views date filters

+++ b/core/modules/views/src/Plugin/views/filter/Date.php
@@ -111,7 +111,7 @@ protected function buildGroupValidate($form, FormStateInterface $form_state) {
-      if (empty($group['remove'])) {
+      if (empty($group['remove']) && !in_array($group['operator'], array('empty', 'not empty'))) {

Can't we use \Drupal\views\Plugin\views\filter\NumericFilter::operators in order to figure out how many parameters this specific operator needs? This would feel just a little bit nicer :)

I couldn't reproduce the described bug.
There is no "The value is required if title for this item is defined." error message when I choice the "Is empty" or "Is not empty" filters as grouped options with custom label.

I think this problem was fixed as a part of another issue.
So I recommend to close this issue with "Closed (outdated)" or "Closed (cannot reproduce)"

However find 2 other strange things that could be a separated issue (if not reported already)

Unnecessary descriptions in the "Value" column

Does not make any sens this description for a filter which does not use any user input (is_empty, is_not_empty)

Value type

• A date in any machine readable format. CCYY-MM-DD HH:MM:SS is preferred.
• An offset from the current time such as "+1 day" or "-2 hours -30 minutes"

The grouped "Is empty" and "Is not empty" are ignored

The WHERE clause of the generated SQL statement does not contains any filter for a date field.

I get this same error when switching from an "is equal to" to an "in between" operator. When switching i now get 2 fields to fill in and even though both are filled in, i get both fields reported as in error.

seems like this patch may be focused on the "Is empty" and "Is not empty" operators; so probably not targeting the core issue.

I'm not able to reproduce the issue on latest 9.4.x version following the steps mentioned in the issue summary.
I tried different operators that require different set of values - it works as expected.

I didn't dig too much, but those 2 issues seemed very similar, so probably it was fixed there:

• #2633678: Improve grouped filter form and fix validation problems
• #2865344: Exposed date filters 'empty' and 'not empty' are broken

To prove this, I'm attaching a merge request with the test coverage for Date filter with all possible variations. Each of the tested operators accepts different values and it'll prove that the issue no longer exists.

By the way, when I was there, I noticed that we have multiple functional tests for views date filter:

1. \Drupal\Tests\datetime\Functional\DateFilterTest
2. \Drupal\Tests\datetime\Functional\Views\FilterDateTest
3. \Drupal\Tests\views\Functional\Handler\FilterDateTest

I think we should merge 3 into 1, and I tend to keep #2, which I used in the merge request. I think it should be done in a separate issue, but I need to confirm with Len.

Updating the issue summary and going to dig into existing tests.

Updating again and moving to "Needs review".

Adding possible operator variations to the IS.

Thanks, this looks great.

Really good to get this extra coverage because grouped filters tend to be fragile and under-tested.

Since this is Views + dates I've kicked of postgres and sqlite tests too, RTBC'ing assuming those come back green too.

It still applies to 9.4.x and 10.0.x.
I'm changing the branch and uploading a patch (there is no diff) to show that it applies to both versions.

Restoring the status because of random test failure.

This looks good to me, we're in commit freeze this week, but I'll revisit it later in the week

Committed and pushed to 10.0.x

As this is adding extra test coverage and there is little risk of disruption, backported to 9.4.x and 9.3.x as well to keep the branches consistent.

This broke HEAD, so I reverted it.

See https://www.drupal.org/pift-ci-job/2342417 for details

Running PHPStan on changed files.
PHP Fatal error:  Declaration of Drupal\Tests\datetime\Functional\Views\FilterDateTest::setUp($import_test_views = true): void must be compatible with Drupal\Tests\views\Functional\ViewTestBase::setUp($import_test_views = true, $modules = [...]): void in /home/rowlands/git/core/drupal/core/modules/datetime/tests/src/Functional/Views/FilterDateTest.php on line 68
Fatal error: Declaration of Drupal\Tests\datetime\Functional\Views\FilterDateTest::setUp($import_test_views = true): void must be compatible with Drupal\Tests\views\Functional\ViewTestBase::setUp($import_test_views = true, $modules = [...]): void in /home/rowlands/git/core/drupal/core/modules/datetime/tests/src/Functional/Views/FilterDateTest.php on line 68

PHPStan: failed

Looks like this collided with #3264122: Move all non migration aggregator tests to the module in preparation of removal in d10 which changed the signature of Drupal\Tests\datetime\Functional\Views\FilterDateTest::setUp().

Attached patch should apply to 10.0.x-dev and 9.4.x-dev and make PHPStan happy.

I've added a raw diff, because patch #21 is in a bit of an awkward format. Basically the only change between #21 and #31 is this bit:

 --- a/core/modules/datetime/tests/src/Functional/Views/FilterDateTest.php
 +++ b/core/modules/datetime/tests/src/Functional/Views/FilterDateTest.php
 @@ -7,7 +7,7 @@
@@ -57,8 +48,8 @@
     */
 -  protected function setUp(): void {
 -    parent::setUp();
-+  protected function setUp($import_test_views = TRUE): void {
-+    parent::setUp($import_test_views);
++  protected function setUp($import_test_views = TRUE, $modules = ['views_test_config']): void {
++    parent::setUp($import_test_views, $modules);

Let's take this back from the top -> 10.0x.-dev.

@Spokje thanks for pointing at #3264122: Move all non migration aggregator tests to the module in preparation of removal in d10 - it explains why it wasn't caught by the test bot.

because patch #21 is in a bit of an awkward format

If you're curious, I was using GitLab generator: https://git.drupalcode.org/project/drupal/-/merge_requests/1448.patch

The re-roll is pretty straightforward. I would RTBC if I could :)

Was going to say the same thing I did in #20, awesome stuff, thanks all!

This doesn't apply clean to 10.x anymore

error: patch failed: core/modules/datetime/tests/src/Functional/Views/FilterDateTest.php:142
error: core/modules/datetime/tests/src/Functional/Views/FilterDateTest.php: patch does not apply

I tried a 3-way apply but that failed too.

This time we were derailed by #3269517: Datetime and Datetime Range tests should not rely on Classy.

The attached patch should pass for 10.0.x-dev.

Added reroll of patch #38 on Drupal 9.3.x.

So for 10 and 9.4 -> #38

Unsure if #3269517: Datetime and Datetime Range tests should not rely on Classy was backported to 9.3, if not #31 might still apply. Otherwise #39 might be the one.

The fail in #39 is unrelated.

Back to green, back to RTBC

Committed 248d012 and pushed to 10.0.x. Thanks!

Backported to 9.4.x

It would be good to keep 9.3.x consistent here too, can we get a version that applies to 9.3.x

So...

both patches #31 and #39 apply and pass tests on 9.3.x-dev (The 1 fail on #39 is a random one).

Since #3269517: Datetime and Datetime Range tests should not rely on Classy wasn't backported to 9.3.x-dev, I think #31 is the most clean way to go?

Moving back to RTBC for #31.

This is now testing the patch in #38 on 10.0.x instead of a patch for 9.3.x. In #44 @Spokje, thinks the patch to use is the one from #31 so I am re-uploading that so tests run on that. The patch in #39 also applies to 9.3.x, so I am uploading a diff between #31 and #39 to help those who know this work.

Keeping this at RTBC because no changes were made to the patch from #31. Hope this helps.

Backported, thanks!