Improve grouped filter form and fix validation problems

1. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -660,20 +660,35 @@ protected function buildGroupValidate($form, FormStateInterface $form_state) {
   +            assert(is_array($group['value']));
   

   We can't really assume the value is always going to be an array.

2. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -660,20 +660,35 @@ protected function buildGroupValidate($form, FormStateInterface $form_state) {
   +          else {
   +            if (trim($group['value']) == '') {
   +              if ($group['title'] != '' ) {
   +                $form_state->setError($form['group_info']['group_items'][$id]['value'], $this->t('A value is required if the label for this item is defined.'));
   +              }
                }
   +            else {
   +              $has_value = TRUE;
   +            }
   +          }
   

   By doing this, operations that don't require a value (like isEmpty) also get an error.

Fixed those issues but that caused a lot of reverts from the initial patch, didn't add any additional tests yet. Let's see if this already fixes the broken tests.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -658,21 +658,26 @@ protected function buildGroupValidate($form, FormStateInterface $form_state) {
-                (is_array($group['value']) && count(array_filter($group['value'], 'static::arrayFilterZero')) == 0)) {
...
-              (is_array($group['value']) && count(array_filter($group['value'], 'static::arrayFilterZero')) > 0)) {

this removes the only uses of

protected static function arrayFilterZero($var) {
    return trim($var) != '';
  }

should that method not be removed as well? Or be marked deprecated?

The fact that that method exists though, makes me think that at some point there were value arrays with 0 being a valid value. Only thing I can thing of would be a boolean filter, but that currently uses a text filter #2469553: Views filtering on boolean fields doesn't use right formatter, so even that doesn't apply.
But thats the only thing I can think of that would fail with this fix, an array where 0 is a valid value.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -658,21 +658,26 @@ protected function buildGroupValidate($form, FormStateInterface $form_state) {
+            if ($group['title'] == '' && $has_valid_value) {

using !empty() and empty() instead of all the =='' would make this easier to read, but that might just be me (or mean that I should really go to bed).

Manually tested this for the node type filter and fixes the issue there. Looks great otherwise.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -1450,19 +1449,6 @@ public function canGroup() {
   /**
-   * Filter by no empty values, though allow to use "0".
-   *
-   * @param string $var
-   *   The variable to evaluate.
-   *
-   * @return bool
-   *   TRUE if the value is equal to an empty string, FALSE otherwise.
-   */
-  protected static function arrayFilterZero($var) {
-    return trim($var) != '';
-  }

I also think this method should be removed, it doesn't seem to be used anywhere else, the @return comment is also a bit confusing.

+++ b/core/modules/views_ui/src/Tests/ExposedFormUITest.php
@@ -81,6 +95,57 @@ function testExposedAdminUi() {
+
+    // Test adding a new exposed sort criteria.
...
+    // Change the order and expose the sort.

Hmm, why are we testing setting sort criteria? Am I missing something?

@mikeker ah yeah missed all the - at the end there! Is all the rearranging necessary in the test file? Makes the patch much harder to read (and could be seen as unrelated changes).

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -658,21 +658,26 @@ protected function buildGroupValidate($form, FormStateInterface $form_state) {
+              $form_state->setError($form['group_info']['group_items'][$id]['title'], $this->t('A label is required if the specified operator.'));

+++ b/core/modules/views_ui/src/Tests/ExposedFormUITest.php
@@ -33,6 +42,11 @@ protected function setUp() {
+    $this->groupFormUiErrors['missing_title_empty_operator'] = t('A label is required if the specified operator.');

the text seems to miss something at the end... is used/ is set, or change if to for?

Oh and just manually tested the empty/not empty handling of the grouped date filter and that doesn't work. So maybe that needs the special handling as well?

1. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -636,6 +636,39 @@ public function validateExposeForm($form, FormStateInterface $form_state) {
   +   * @param $group
   

   missing type hint

2. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -636,6 +636,39 @@ public function validateExposeForm($form, FormStateInterface $form_state) {
   +   * Determines if the given group filter has a valid value.
   

   it tests a grouped filter group I'd say.

3. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -655,26 +688,21 @@ protected function buildGroupValidate($form, FormStateInterface $form_state) {
   +            if ($operators[$group['operator']]['values'] == 0) {
   

   this could use some explaining

4. +++ b/core/modules/views_ui/src/Tests/ExposedFormUITest.php
   @@ -81,6 +98,57 @@ function testExposedAdminUi() {
   +    // Check the label of the grouped exposed button
   

   missing period at the end.

5. +++ b/core/modules/views_ui/src/Tests/ExposedFormUITest.php
   @@ -96,82 +164,96 @@ function testExposedAdminUi() {
   +  public function disabledtestGroupedFilterAdminUiErrors() {
   

   disabledtest?

Looking good! hasValidGroupedValue() is a good addition I feel. I'd say not make it more general for now, this is getting big enough as it is.

@mikeker but thanks for trying.

Manually tested it again with the node type filter, patch does its job. Not sure if the addition of hasValidGroupedValue() is considered a API change. Since it doesn't change anything just gives you an extra option to do something more efficient (and the old method still works as always), I don't think so, but not sure.

But this now looks good to me.

Do you think we can have this patch available for Drupal 7

1. +++ b/core/modules/views/src/Plugin/views/filter/Date.php
   @@ -103,32 +103,23 @@ public function validateValidTime(&$form, FormStateInterface $form_state, $opera
   -  protected function buildGroupValidate($form, FormStateInterface $form_state) {
   ...
   +  protected function hasValidGroupedValue(array $group) {
   

   I think given how this is fixed I think we need a CR and this can only really be fixed in 8.1.x - any contrib/custom filters might need to make similar changes - no? It might be good to get the "should not be an error" part displayed in the issue summary fixed in 8.0.x though...

2. +++ b/core/modules/views_ui/src/Tests/ExposedFormUITest.php
   @@ -96,82 +164,96 @@ function testExposedAdminUi() {
   +
   +  protected function assertNoGroupedFilterErrors($message = '', $group = 'Other') {
   

   Missing docblock and assert* functions should return a boolean.

Re-rolled. Fixed coding standards.

During the re-roll I placed arrayFilterZero() in the wrong file.

Back to RTBC since #28 has been addressed.

I also tested this on simplytest.me and can confirm is working as expected.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -618,6 +618,38 @@ public function validateExposeForm($form, FormStateInterface $form_state) {
+        $actual_values = count(array_filter($group['value'], 'static::arrayFilterZero'));

Oh, I wasn't aware that this is actually possible, nice!

I really like the new named function, it just makes it easier to ready.
+1 for the changes in general

@mikeker no really didn't apply anymore. Reroll.

@mikeker nope, not in NOLA unfortunately.

Fails were unrelated, so back to RTBC per #34 and #41

Testbot hiccup, back to RTBC

Unrelated fails, back to RTBC

Committed 3bf487f and pushed to 8.2.x. Thanks!

Only committed to 8.2.x because of the addition of a method to the plugin base class.

FILE: .../core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1550 | ERROR | [x] The closing brace for the class must have an
      |       |     empty line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Fixed that on commit.