Grouped exposed taxonomy filter fails validation for autocomplete widget

I thin the underlying problem is that the autocomplete widget is completely broken in this case. If you switch to the dropdown widget, the grouped filter works correctly. As such I don't think this qualifies as Major (isolated impact, has a workaround).

There is also a nomenclature issue in that "title" in the error refers to fields in the column "label." One or the other needs to change.

Agreed! And it's missing "the" before "title." Attached patch fixes that, but does nothing about the underlying problem. I'll look into that later...

grr...

@mikeker I'm looking into this, but can I suggest that we move your patch to a new issue? I think it's a good documentation fix and RTBC in itself. Fixing the TaxonomyIndexTid filter handler for grouping is going to take much more work to fix (it's a mess).

Anyway, this needs more work.

First stab at a fix partial. I didn't include the patch in #8 (see #14).

Manual testing lets me save the value and it rebuilds the form like it should. The filter looks good too, but it doesn't actually filter anything when you select a value. So that needs work. And tests.

#14: @Lendude, makes sense -- I've filed a followup in #2633678: Improve grouped filter form and fix validation problems.

Agreed, TaxonomyIndexTid is going to be a bit of work... Thanks for taking that on and good luck!

Now with working filter. Still needs tests.

Now with tests. When this is fixed though you run into #2369119: Fatal error when trying to save a View with grouped filters using other than string values. So you can't actually save the View until that is fixed.

Because of that I now only test the output in the preview because that works fine.

Interdiff is the test only patch.

Rerolled #18.

@mikeker thanks for the reroll! forgot to remove a couple of merge tags.

Needs a re-roll

array() => [] reroll, nothing else.

edit: not sure why that got uploaded twice, same patch, ignore one.

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -265,13 +265,30 @@ protected function valueValidate($form, FormStateInterface $form_state) {
   +      if (!empty($form_state->getValue(['options', 'value']))) {
   

   This !empty should not be neccesary

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -398,4 +419,21 @@ public function calculateDependencies() {
   +  public function buildExposedFiltersGroupForm(&$form, FormStateInterface $form_state) {
   ...
   +    return parent::buildExposedFiltersGroupForm($form, $form_state);
   

   I never saw this pattern in views before. I know this probably will work without side effects, but usually what we do is call the parent method at the beginning and then do the other modifications.

3. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -398,4 +419,21 @@ public function calculateDependencies() {
   +    if ($this->options['type'] == 'textfield') {
   

   Hm. Is this the only way we have to check that a field is using autocomplete?

4. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -398,4 +419,21 @@ public function calculateDependencies() {
   +          $terms = Term::loadMultiple(($item['value']));
   

   Double parenthesis here

Updated this for 8.4.x and 8.3.x.

• Added a check for the all filter in validateExposed method. This was throwing an undefined index notice.
• Changed location of test for 8.4.x so it applies cleanly to 8.4.x. 8.3.x patch retains the original location
• Looks like most of the comments from dagmar were already addressed in #29. I verified the parent:: return but if we move this to top of method then the grouping will return just the entity ID in the values field and throw and unknown entity error upon save.

Rerolls for 8.3.5 support.

Let's see what the testbots have to say.

#32 has worked for me.

#31's 8.4.x doesn't work for me, I get this error when I tried to filter counted relationship data.

Before patch:
InvalidArgumentException: The configuration property display.page_search_ideas.display_options.filters.title.value.value doesn't exist. in Drupal\Core\Config\Schema\ArrayElement->get() (line 76 of D:\phptest\theideaproject\web\core\lib\Drupal\Core\Config\Schema\ArrayElement.php).

After patch:
InvalidArgumentException: The configuration property display.page_search_ideas.display_options.filters.title.value.min doesn't exist. in Drupal\Core\Config\Schema\ArrayElement->get() (line 76 of D:\phptest\theideaproject\web\core\lib\Drupal\Core\Config\Schema\ArrayElement.php).

Core 9.1.8 has this applied. I would recommend changing the status to Fixed.

@rfmarcelino nope, this is still broken, new Test-only patch to show this still breaks

reroll and update to remove deprecated methods.

Fixed CS

I reproduced the same issue when was investigating another one: #1810148: Grouped exposed taxonomy term filters do not work because the group key is added to the query and not the taxonomy ID.

It looks like it's applicable to every filter, which is using autocomplete widget for group items. For instance, I have the same issue when trying to add a filter by Author, which is using \Drupal\user\Plugin\views\filter\Name class.

It makes me think that the fix itself should be either in another place, or we should also take care of other plugins in addition to \Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTid.

I'm just updating the issue summary, because and I don't have any code suggestions for now.

I also meant to change the status :)
@lendude, please let me know if you'd like to continue here. Otherwise, we can swap and I'll try to come up with some patch.

After further investigation, I agree with the approach taken by @lendude in previous patches - value normalization should happen in valueValidate method.

I applied the same changes to \Drupal\user\Plugin\views\filter\Name class, so we should probably need a test coverage for this plugin as well.

I also had to revert some changes in \Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTid that are already covered by #1810148: Grouped exposed taxonomy term filters do not work because the group key is added to the query and not the taxonomy ID. Unfortunately, tests here won't pass until we land #1810148: Grouped exposed taxonomy term filters do not work because the group key is added to the query and not the taxonomy ID.

I also created another issue that I faced along the way: #3250352: Username views filter should not process default value twice .

Next steps:
1) Add similar test coverage for \Drupal\user\Plugin\views\filter\Name plugin;
2) Transfer issue credits from #2920039: Views' User Name exposed group filter validation and close it as a duplicate;
3) Resume when #1810148: Grouped exposed taxonomy term filters do not work because the group key is added to the query and not the taxonomy ID is in;

Removing references to \Drupal\user\Plugin\views\filter\Name plugin that will be fixed in #2920039: Views' User Name exposed group filter validation.
We'll resume here when #1810148: Grouped exposed taxonomy term filters do not work because the group key is added to the query and not the taxonomy ID is done.

It looks like a test failure is a random one.

It was a bit hard to rebase the MR after #1810148: Grouped exposed taxonomy term filters do not work because the group key is added to the query and not the taxonomy ID and #3248295: Taxonomy tests should not rely on Classy. I decided to start from scratch and create a patch.

Added one more condition to handle the issue reported in #3280477: Name and TaxonomyIndexTid filter plugins have incorrect default values in grouped filters.

Really nitty nitpicks only, probably only removing stuff I added myself in the first place :D

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -290,13 +290,30 @@ protected function valueValidate($form, FormStateInterface $form_state) {
   +    // Autocomplete puts the values in target_id. Move the values to the
   +    // expected depth.
   

   Might be nice to point to what is expecting this

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -359,11 +376,15 @@ public function validateExposed(&$form, FormStateInterface $form_state) {
   +      // Set the validated_exposed_input to the selected group values.
   

   Not sure we need this comment? Seems pretty obvious what this does :)

3. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -429,4 +450,21 @@ public function calculateDependencies() {
   +    if ($this->options['type'] == 'textfield') {
   

   We can do === here I think?

4. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
   @@ -283,6 +283,7 @@ public function testExposedGroupedFilter() {
   +    $filter_settings_path = '/admin/structure/views/nojs/handler/test_taxonomy_exposed_grouped_filter/page_1/filter/field_views_testing_tags_target_id';
   
   @@ -292,7 +293,7 @@ public function testExposedGroupedFilter() {
   -    $this->drupalGet('/admin/structure/views/nojs/handler/test_taxonomy_exposed_grouped_filter/page_1/filter/field_views_testing_tags_target_id');
   +    $this->drupalGet($filter_settings_path);
   

   Hmmmm borderline unrelated I think, but lets keep it :) No action required.

Done! Following the example on #2920039: Views' User Name exposed group filter validation I also changed the usage of static methods to term storage methods.
Technically, this is out of scope, but why don't we change it here? :)

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -290,13 +289,30 @@ protected function valueValidate($form, FormStateInterface $form_state) {
+    // Autocomplete puts the values in target_id. Move the values as expected by
+    // Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTid::validateExposed() method.

This seems to be longer than 80 chars but the bot doesn't seem to be tripping over this, so ¯\_(ツ)_/¯

Down to more minor nits here

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -221,7 +220,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   +        $terms = $this->termStorage->loadMultiple($query->execute());
   

   $query->execute can return NULL, which would cause an error for ::loadMultiple

   But I see that's an existing coddepath so perhaps follow-up?

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -290,13 +289,30 @@ protected function valueValidate($form, FormStateInterface $form_state) {
   +    if ($this->isAGroup()) {
   

   let's return early inside this if and avoid the else while we're touching this code, it makes it much easier to read as the cyclic complexity is reduced

3. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -290,13 +289,30 @@ protected function valueValidate($form, FormStateInterface $form_state) {
   +            foreach ($item['value'] as $value) {
   +              $tids[] = $value['target_id'];
   +            }
   

   We can write this as $tids = array_column($item['value'], 'target_id');

4. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -290,13 +289,30 @@ protected function valueValidate($form, FormStateInterface $form_state) {
   +        foreach ($form_state->getValue(['options', 'value']) as $value) {
   +          $tids[] = $value['target_id'];
   +        }
   

   same here re array_column

1) I'm not sure about this. The interface declares the following:

/**
   * Execute the query.
   *
   * @return int|array
   *   Returns an integer for count queries or an array of ids. The values of
   *   the array are always entity ids. The keys will be revision ids if the
   *   entity supports revision and entity ids if not.
   */
  public function execute();

I also found few more cases matching this pattern: loadMultiple($query->execute()).
Can it really return NULL in some cases?

2) Done
3-4) Good call on using array_column

I also made some minor rearrangements around the lines I was already modifying, hopefully I didn't go too far :)

In 9.4 there was a bug:
TypeError: Illegal offset type in Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTid->validateExposed() (line 364 of /var/www/docroot/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php)
I'm sending a new patch fixing this bug.

Can we add/edit the test to see the bug and make sure it was caught?

@rubens.arjr, can you add some steps to reproduce the issue mentioned in your comment?
As you might see, I queued a test of my previous patch for Drupal 9.4.x and it's green. Therefore, your issue probably requires some additional steps. It might be worth moving this into a follow-up, but we need to see what's the root cause.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Tested #64 as #65 steps were not provided.

Can confirm the issue described in the IS and the steps were perfect.
Applied patch
Now am able to save the group filter without issue.

Searching for loadMultiple($query->execute()); only none test file I saw was TermStorage.php

Just to be extra safe could we do something like

        $terms = [];
        $results = $query->execute();
        if (isset($results)) {
          $terms = $this->termStorage->loadMultiple($results);
        }