#states not supported for elements in formatter settings being displayed on Views field handler form

That's a great idea!

I guess we would also need some test for this new function.

#3: 1985406-views-field-formatter-settings-states.patch queued for re-testing.

Re-rolled the above solution, but would this make more sense in a component somewhere?

I think having this there, for now, is a totally fine place ... All of the states API is still in that place.

Let's readd the tags, @jhedstrom do you mind writing a test?

This adds a very simple test (taken from Dave Reid's example).

The property $field->id no longer exists. I've updated the patch to use $field->getLabel() instead, but am not sure that is the correct change to make.

The proper method to use is getName() (in my manual testing anyway). The id() method includes entity and bundle info, which is not present in the markup.

I've included an update to the manual testing patch.

Cool. Does someone know whether we should backport this to D7 (API addition or just directly in views)?

I think we might be able to leverage this code in FilterPluginBase::buildExposedFiltersGroupForm() there is a load of code after line 988 manipulating states.

@alexpott
So no commit of the current status which already solves a bug? your suggestion is just a task.

1. +++ b/core/includes/common.inc
   @@ -2021,6 +2021,33 @@ function drupal_process_states(&$elements) {
   +function drupal_rewrite_states_selector(array &$elements, $search, $replace) {
   

   I think this should be a static method on a new FormHelper class.

2. +++ b/core/includes/common.inc
   @@ -2021,6 +2021,33 @@ function drupal_process_states(&$elements) {
   +  foreach (element_children($elements) as $key) {
   

   Deprecated.

3. +++ b/core/modules/system/src/Tests/Form/StatesTest.php
   @@ -0,0 +1,37 @@
   +class StatesTest extends DrupalUnitTestBase {
   

   Once the above is done this can be a PHPUnit test.

Also this will fail in the states array is declared like this:

          '#states' => array(
            'visible' => array(
              array(
                ':input[name="menu[type]"]' => array('value' => 'normal'),
              ),
              array(
                ':input[name="menu[type]"]' => array('value' => 'tab'),
              ),
              array(
                ':input[name="menu[type]"]' => array('value' => 'default tab'),
              ),
            ),

From \Drupal\views\Plugin\views\display\Page

See #2356443: Grouped numeric filters can not have a value set for an instance of this bug - and somewhere this function should be used.

This patch moves the logic to a helper class as suggested, and has been reworked to apply to the example in #20.

I need more insight to the code in FilterPluginBase::buildExposedFiltersGroupForm() before I feel comfortable porting it over to use this new helper method.

Look at all the untested code we can remove! Closing #2356443: Grouped numeric filters can not have a value set in favour of this issue since this adds a tested method to do what we need. I've manually tested too and it works great.

Upgrading to major since grouped views filters can not be set up without it (if you have javascript turned on).

Steps to reproduce:

1. Add a numeric field to user
2. Edit the admin/people view
3. Add a filter for the numeric field
4. Expose it and group it.

The value, min and max fields are all hidden. What is supposed to happen is that either the value or the min max fields are supposed to appear depending on which operator you have selected.

1. +++ b/core/lib/Drupal/Core/Form/FormHelper.php
   @@ -0,0 +1,56 @@
   +      foreach ($elements['#states'] as $state => $ids) {
   +        foreach ($ids as $id => $values) {
   +          if (strpos($id, $search) !== FALSE) {
   +            $new_id = str_replace($search, $replace, $id);
   +            $elements['#states'][$state][$new_id] = $values;
   +            unset($elements['#states'][$state][$id]);
   +          }
   +          elseif (is_array($values)) {
   +            foreach ($values as $selector => $value) {
   +              if (strpos($selector, $search) !== FALSE) {
   +                $new_selector = str_replace($search, $replace, $selector);
   +                $values[$new_selector] = $value;
   +                unset($values[$selector]);
   +              }
   +            }
   +            $elements['#states'][$state][$id] = $values;
   +          }
   +        }
   

   The #states can be more than two levels deep.

2. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -979,39 +980,14 @@ protected function buildExposedFiltersGroupForm(&$form, FormStateInterface $form
   -          if (!empty($this->options['group_info']['group_items'][$item_id]['value'][$children])) {
   -            $row['value'][$children]['#default_value'] = $this->options['group_info']['group_items'][$item_id]['value'][$children];
   -          }
   

   Without this default values are not set. The value, min and max fields are all empty when you go back to edit the grouped filter.

re #23

1. @olli looking at all the examples in https://api.drupal.org/api/drupal/includes%21common.inc/function/drupal_... I can't see how this function does not cope with all of them. Can you provide an example?
2. Yep that needs fixing.

See the example in https://www.drupal.org/node/1464758

How is that different from the example I pointed out in #20 and was subsequently fixed?

Oh I see :) - patch incoming.

Added https://www.drupal.org/node/1464758 as part of the test and improved rewriteStatesSelector to handle this. Also fixed #23.2

Great!! Some notes:

1. +++ b/core/lib/Drupal/Core/Form/FormHelper.php
   @@ -0,0 +1,68 @@
   +/**
   + * Encapsulates the caching of a form and its form state.
   + *
   + * @ingroup form_api
   + */
   +class FormHelper {
   

   "Provides helpers to operate on forms"?

2. +++ b/core/lib/Drupal/Core/Form/FormHelper.php
   @@ -0,0 +1,68 @@
   +      foreach ($elements['#states'] as $state => $ids) {
   +        static::processStatesArray($elements['#states'][$state], $search, $replace);
   +      }
   

   Unused $ids.

3. +++ b/core/lib/Drupal/Core/Form/FormHelper.php
   @@ -0,0 +1,68 @@
   +   * @param array $ids
   +   *   States array element.
   +   * @param $search
   +   *   A partial or entire jQuery selector string to replace in #states.
   +   * @param $replace
   +   *   The string to replace all instances of $search with.
   +   */
   +  protected static function processStatesArray(array &$elements, $search, $replace) {
   

   $ids -> $elements

4. +++ b/core/lib/Drupal/Core/Form/FormHelper.php
   @@ -0,0 +1,68 @@
   +    foreach ($elements as $id => $values) {
   ...
   +          $keys = array_keys($elements);
   +          $index = array_search($id, $keys, TRUE);
   +          $keys[$index] = $new_id;
   +          $elements = array_combine($keys, array_values($elements));
   

   I wonder if it is possible to move these array_*() friends outside the loop.

5. +++ b/core/lib/Drupal/Core/Form/FormHelper.php
   @@ -0,0 +1,68 @@
   +      if (strpos($id, $search) !== FALSE) {
   +        $new_id = str_replace($search, $replace, $id);
   +        if ($new_id != $id) {
   

   Why the second "if"?

6. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -979,39 +980,22 @@ protected function buildExposedFiltersGroupForm(&$form, FormStateInterface $form
   +          if (!empty($this->options['group_info']['group_items'][$item_id]['value'][$child])) {
   +            $row['value'][$child]['#default_value'] = $this->options['group_info']['group_items'][$item_id]['value'][$child];
              }
   ...
   -          $row['value'][$children]['#title'] = '';
   -
   -          if (!empty($this->options['group_info']['group_items'][$item_id]['value'][$children])) {
   -            $row['value'][$children]['#default_value'] = $this->options['group_info']['group_items'][$item_id]['value'][$children];
   -          }
   

   Since we are inside a table, would #title_display = invisible work here?

7. +++ b/core/tests/Drupal/Tests/Core/Form/FormHelperTest.php
   @@ -0,0 +1,90 @@
   + * Contains \Drupal\system\Tests\Form\StatesTest.
   

   Contains core FormHelperTest.

1. Fixed
2. Well then we'd have to call array_keys - don't see the point
3. I've changed this to conditions to mirror the language in the docs for drupal_process_states()
4. Good point - it is for the array_keys but not the array_values since the values can change during a recursive call.
5. Overly defensive code - removed.
6. Well changing that is out of scope - but I had removed in the latest patch :( that code is so untested. Will open a followup to test. Hopefully now I have made the new code work exactly like the old code.
7. Fixed

I've just scanned the whole of core and I think that FilterPluginBase is the only place we're doing this sort of manipulation of the states array so it's definitely good to covert it to the new helper method.

#30.6 I agree, we need to improve that anyway. I've made a small fix in this code to restore your fix to #23.2.

re #32 @olli nice catch - again! We so need tests!

Great work on the expanded tests! RTBC IMO.

Re #33: There are tests for #23.2 in #2357145: Views can not be saved with a numeric (grouped) filter and #2369119: Fatal error when trying to save a View with grouped filters using other than string values.

I only made a small change in #32 (we didn't have tests for it yet) so I guess I can set this to RTBC.

Opened #2387353: Improve form element labels in views grouped filter for #29.6

1. +++ b/core/lib/Drupal/Core/Form/FormHelper.php
   @@ -0,0 +1,70 @@
   +        $conditions = array_combine($keys, array_values($conditions));
   

   Looks like the array_combine() could happen after the foreach has run?

2. +++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
   @@ -979,39 +980,30 @@ protected function buildExposedFiltersGroupForm(&$form, FormStateInterface $form
   +      // is needed because forms are inside a new form and their ids changes.
   

   IDs change?

1. Nice catch @catch! Yep you're correct - fixed
2. Fixed (this is in HEAD already but we're moving the comment so fixing).

Committed/pushed to 8.0.x, thanks!