Allow multiple vocabularies in the taxonomy filter

Yeah this change makes sense.

Currently looks like this:

Couple of things

1. +++ b/core/modules/taxonomy/config/schema/taxonomy.views.schema.yml
   @@ -123,6 +123,11 @@ views.filter.taxonomy_index_tid:
        vid:
          type: string
          label: 'Vocabulary'
   

   this can be removed I'd say

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -147,9 +151,18 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
   +    $vocabulary = reset($vocabularies);
   

   This is still used for the 'Dropdown' option. So with the current patch the Dropdown option still just uses one vocabulary.

And it obviously needs tests and an upgrade path, but the idea makes sense.

Looking good. Couple of things I see:

1. +++ b/core/modules/taxonomy/config/schema/taxonomy.views.schema.yml
   @@ -123,6 +123,11 @@ views.filter.taxonomy_index_tid:
        vid:
          type: string
          label: 'Vocabulary'
   

   'vid' should now be redundant so we should remove it

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -147,9 +155,33 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
   +      $form_state->setErrorByName('options][vids', $this->t("You cannot select more than one vocabulary, when choosing 'Dropdown' as selection type."));
   

   Any reason for not allowing this other then 'limited use-case' (large lists in select == bad idea)?

3. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -158,8 +190,11 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   +        '#title' => $this->options['limit'] ? $this->t('Select terms from vocabularies @vocs', array('@vocs' => implode(', ', $vocabulary_labels))) : $this->t('Select terms'),
   

   Any easy way to make this into a better sentence when we have multiple vocabs? now it would be "voc1, voc2, voc3" but it would be nice to have "voc1, voc2 and voc3"

4. +++ b/core/modules/taxonomy/src/Tests/Views/Update/TaxonomyIndexTidUpdateTest.php
   @@ -0,0 +1,41 @@
   +    // Check that the field is using the expected base table.
   

   copy/paste leftover? 'is using vids' or something.

5. +++ b/core/modules/taxonomy/src/Tests/Views/Update/TaxonomyIndexTidUpdateTest.php
   @@ -0,0 +1,41 @@
   +    $this->assertTrue(empty($data['display']['default']['display_options']['filters']['tid']['vid']));
   

   shouldn't this be more strict and use !isset() instead of empty()? We want to make sure the 'vid' key is gone from config, not just that it's empty.

6. +++ b/core/modules/taxonomy/taxonomy.post_update.php
   @@ -0,0 +1,41 @@
   + * Update the cacheability metadata for all views.
   

   copy/paste leftover, needs update.

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -96,13 +100,16 @@ protected function defineOptions() {
   +    $options['vids'] = array('default' => []);
   

   Weird that we keep the array() syntax on the outer array while using the square bracket syntax on the inner array :)

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -147,9 +155,33 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
   +  }
   +
   +
   +  /**
   

   Extra empty line.

let's see

@dawehner, well, it looks good to me and I think it makes sense. I did some cleanup, improved the update path test, nits, etc.

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -78,7 +79,10 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
+    if (empty($this->options['vids']) && !empty($this->options['vid'])) {
+      $this->options['vids'] = [$this->options['vid']];
     }

I removed this because I felt that was a "non-transparent" try to hide an update path :) Now we have a dedicated update path so it doesn't make sense anymore.

Good to go, IMO.

Even more cleanup.

Why do we have to do that? This particular view should not have tid stuff in the first place.

@dawehner, if we put vid: tags (the old schema) directly in the .yml then Drupal\Tests\views\Kernel\TestViewsTest::testDefaultConfig() fails:

$ ./vendor/bin/phpunit -c core --filter TestViewsTest
PHPUnit 4.8.11 by Sebastian Bergmann and contributors.

F

Time: 9.19 seconds, Memory: 291.00Mb

There was 1 failure:

1) Drupal\Tests\views\Kernel\TestViewsTest::testDefaultConfig
Schema key views.view.test_filter_taxonomy_index_tid_pre_update:display.default.display_options.filters.tid.vid failed with: missing schema

core/modules/config/src/Tests/SchemaCheckTestTrait.php:43
core/modules/views/tests/src/Kernel/TestViewsTest.php:52

See https://www.drupal.org/pift-ci-job/428187.

That's why we need to add the new schema in the .yml and then just swap to the old one when the fixture applies.

Looks good now. All feedback has been addressed, we have a fix, we have tests, we have an update path (+test). Manually tested this and that works too.

Added before/after screenshots to the IS.

I'd just like to chime in to say I've been testing this patch and it all (finally) works as advertised. Thank you!

What happens if you have a module providing default config after this change (if they don't update it)?

Yes I think that's worth doing. The existing configuration works - i.e. it's not stored incorrectly, so we should try to keep those views working.

Is it worth looking at isolating the bc layer to the config installer somewhere? Every other case is covered by either the upgrade path or the protections against deploying between different versions of core.

Is there some form of event which is just fired on config import?

Took a stab at this with an EventSubscriber fired on ConfigEvents::IMPORT, works nicely for config imported, but doesn't fire on module install, so it doesn't cover all the cases. Also it gives a lot of duplicated code, which is not pretty.

Just thinking out loud here, I'd be against adding BC code like this

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -81,9 +81,6 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
    if (empty($this->options['vids']) && !empty($this->options['vid'])) {
      $this->options['vids'] = [$this->options['vid']];
    }

That would lead to BC code scattered throughout the code base, which sounds like a cleanup nightmare. Having one place where we deal with updating outdated config sounds much better to me. Something like running post_updates on config import and module install. Something like that would also allow us to give a nice message that some outdated config was imported and alert module maintainers that something needs to be updated.

With more config updates in the making should we look at a way to handle this properly. #2459289: Boolean default values are not saved ran into the same thing. There an update wasn't needed, but it still leads to outdated config being imported. I can only imagine this happening more in the future.

Adding a patch with the eventlistener just for reference here. Interdiff looks a bit weird

Repost patch from #32.

@dawehner I agree that the most important scenario to cover is the install one, I was hoping ConfigEvents::IMPORT would fire on install, but alas....

hook_view_insert could work, but I think would add unnecessary overhead in scenarios where it's not needed (most commonly, just creating a new view through the UI). Since I think we will see more of these config changes in the future, this overhead might become significant.

Maybe hook_modules_installed? which at least only fires when we want it to, but would need to check all config and not just the new one, which sounds bad.

But to be honest, we need something generic to deal with this. There are 3 ways that out-of-date config can occur:

1. Already installed when the update occurs
2. On import
3. On module install

Currently only #1 is covered. And we need some way to cover all three going forward, and preferably not by copy/pasting the same code in 3 different hooks, right?

I doubt doing this in hook_view_insert() would have much overhead even if we're not able to differentiate between default config and config created via the UI in there - should only be an isset/is_array check anyway. Agreed that seems like a good place to do it. If we do eventually end up with lots of these and there's noticeable overhead, we can at least remove them in 9.x.

This from ConfigInstaller::createConfiguration() is the only thing I can find differentiating between new config via module defaults vs. new config from anywhere else.

        // Add a hash to configuration created through the installer so it is
        // possible to know if the configuration was created by installing an
        // extension and to track which version of the default config was used.
        if (!$this->isSyncing() && $collection == StorageInterface::DEFAULT_COLLECTION) {
          $new_config->set('_core.default_config_hash', Crypt::hashBase64(serialize($config_to_create[$name])));
        }

@dawehner Yeah, fair enough. The edge case where someone imports outdated config after already running update.php sounds like something we shouldn't really worry about too much (and sounds more like a 'fix your workflow' issue).

Attached is a reroll of the patch for 8.3.1.

Here is a reroll to support 8.3.5 just to keep this up to date. Active dev should still move to 8.4.x though.

[deleted]

I've used this Patch until now... but the last comment is 5months ago :O
Isn't the Feature of having multiple taxonomy terms focused anymore or did i miss a new feature which makes the pathc redundant?

Reroll for 8.5.1.

Made some changes to allow the dropdown widget in views when selecting multiple vocabularies. The exposed form options will contain all terms from the selected vocabularies.

Reroll against 8.5.x

@kennethos patch #56 failed for 8.7.x-dev.

[deleted]

return $vocabulary->label();q
q is removed. from patch

Rerolled 57 to apply to 8.6 / 8.7. I'm using with 8.6.2 and seems to be working so far.

This might fix the schema test errors.

Chipping away at test errors. This adds a cast in the checkboxes render element that might be out of scope for this.

Removing some cruft and no longer messing with render elements. Local tests look good, so fingers crossed.

OK, one more -- found a node Views UI wizard using the old vocab config.

[deleted]

Someone for this important feature ?

I have version 8.6 of Drupal and for the moment the patch have not passed validation

65 should work for 8.6.10, so I'm going to hide 66 since it doesn't apply.

#65 fails for new Drupal 8.7.0 core with revisioned taxonomy terms

Reroll for 8.7.x.

Thanks a lot ahebrank, #71 Works !

One thing: Git moaned about trailing whitespace @line 528

Fixing the trailing whitespace in the patch file. Fingers crossed that's what testbot is not liking, because otherwise I'm not sure what that's about. TaxonomyIndexTidUiTest passes locally.

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -169,21 +192,26 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
+                  ->getTranslationFromContext($term)

The testbot is failing because of deprecations:

Remaining deprecation notices (72)

72x: EntityManagerInterface::getTranslationFromContext() is deprecated in 8.0.0 and will be removed before Drupal 9.0.0. Use \Drupal\Core\Entity\EntityRepository::getTranslationFromContext() instead. See https://www.drupal.org/node/2549139.
60x in TaxonomyIndexTidUiTest::testExposedFilter from Drupal\Tests\taxonomy\Functional\Views
12x in TaxonomyIndexTidUiTest::testFilterUI from Drupal\Tests\taxonomy\Functional\Views

Ah, I see the regression now. Thanks.

This is basically a subset of #2429699: Add Views EntityReference filter to be available for all entity reference fields. Let's see which one is ready to get in first, though.

Resolved some coding standard issues & added an interdiff.

Patch looks good, almost RTBC. Some minor points left for me:

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -78,7 +79,10 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
   +    if (empty($this->options['vids']) && !empty($this->options['vid'])) {
   +      $this->options['vids'] = [$this->options['vid']];
        }
   

   I would like to see a comment why we have this piece of code here.

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -114,18 +121,19 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
   +          '#required' => TRUE,
   

   Why does the option vids need to be required, when the option vid was not?

3. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
   @@ -61,29 +95,26 @@ protected function setUp($import_test_views = TRUE) {
   -    $this->drupalGet('admin/structure/views/nojs/handler/test_filter_taxonomy_index_tid/default/filter/tid');
   +    $out = $this->drupalGet('admin/structure/views/nojs/handler/test_filter_taxonomy_index_tid/default/filter/tid');
   

   Why do we add the variable $out? It is not used anywhere.

Forgot a nitpick:

+++ b/core/modules/taxonomy/config/schema/taxonomy.views.schema.yml
@@ -120,9 +120,12 @@ views.filter.taxonomy_index_tid:
-    vid:

+++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
@@ -126,6 +157,75 @@ public function testFilterUI() {
+  public function testFilterUIWithMoreVocabularies() {

The test has no docblock.

Huh. It appears that this patch made it into Drupal 8.8.0 -- or if not this patch, something extremely similar? Patch appears to be already applied....

Hi,

I have reviewed the patch #81 and it had failed with 8.9x.

I have rerolled it and uploaded the patch with interdiff.

After reroll I have checked the patch and it worked as designed.

Before applying the patch:

I have added the has taxonomy field in the admin content view. And there was no option to add multiple vocabularies in the taxonomy filter.

Below are the output screenshot before applying the patch.

There is the option to add multiple vocabularies in the taxonomy filter after applying the patch.

Below is the output screenshot after applying the patch.

The patch is working great.

Thanks,
Ren

Made a patch for Drupal 8.8.1, modified from #81, no interdiff as it's hard for me to make one.

Key changes:

1. renamed testFilterUIWithMoreVocabularies to testFilterUIWithMultipleVocabularies, and added docblock for it according to the comment #83

Why do we add the variable $out? It is not used anywhere.

2. Removed variable $out according to comment #82

Try to fix the CI error by adding the following lines into taxonomy_post_update_taxonomy_index_filter_multiple_vocabularies()

  // If Views is not installed, there is nothing to do.
  if (!\Drupal::moduleHandler()->moduleExists('views')) {
    return;
  }

I applied patch #90, It's working for me.
I am attaching a screenshot before and after applying the patch.

I applied patch #90, and it's also working for me.

Thanks!

I tested patch #90 on Drupal Core 8.8.1 and it's working as expected for me.

Thank you @jungle for the patch. Great job!

Tested patch #90 (on simplytest.me)

8.8.3 - applied cleanly but doesn't seem to change anything (taxonomy vocabularies are still radio buttons and I can only choose one)
Could be the patch didn't actually apply, but tried it twice with a fresh install.

8.9.dev - applied cleanly and works to give checkboxes instead of radio buttons.

I'm new to this so I apologize if I'm doing it wrong!

8.8.3 - applied cleanly but doesn't seem to change anything (taxonomy vocabularies are still radio buttons and I can only choose one)

Re #94, have you run the database update? By drush, it's drush updb or visit /update.php

That would be really a great enhancement for taxonomy 🤩.

I applied patch #90, looks good to me working fine..

I applied patch in #90 at simplytest.me (Drupal 8.8.5) and it's working fine for me. Please commit it. thanks.

We had an upgrade path test in #7 and that seems to have gotten lost somewhere, we need that back.

Also all the discussion earlier in the issue about updating config provided by modules has lead to the creation of \Drupal\views\ViewsConfigUpdater, so we probably need to use that for the update. That way we can update existing config but also config that gets added by newly installed modules.

Re-rolled up to 9.1

Thanks, @aleevas for the re-roll. Is #99 addressed? If #101 is just a Re-roll, the proper status should be NW. If it has been addressed, please set back to NR.

Neither patch 90 or 101 can be applied to 8.9.2.

core/modules/taxonomy/taxonomy.post_update.php has lots of changes between 8.9 and 9.1 branch.
So I have rerolled two separate patches, Please review.
Verified it on both the versions attaching the screenshot.

After adding a patch, i have tested the above mentioned bug on drupal 9.1 and its resolved.
Please find before and after screenshots attached below

Added back the tests and used ViewsConfigUpdater. However, feels strange to put taxonomy logic in Views module.

Few remarks:

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -78,7 +79,10 @@ public function init(ViewExecutable $view, DisplayPluginBase $display, array &$o
   +    if (empty($this->options['vids']) && !empty($this->options['vid'])) {
   +      $this->options['vids'] = [$this->options['vid']];
        }
   

   This was an early BC assurance. Now we rely on ViewsConfigUpdater.

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -114,18 +121,19 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
   -      if (empty($this->options['vid'])) {
   +      if (empty($this->options['vids'])) {
            $first_vocabulary = reset($vocabularies);
   -        $this->options['vid'] = $first_vocabulary->id();
   +        $this->options['vids'] = [$first_vocabulary->id()];
          }
   

   I don't think it's the case. If `limit` is off, we use all vocabs. If it's on and no vocab is selected, we should, probably, add a form validation error. @Lendude, could you, please, make a point here, as module maintainer?

3. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -149,19 +160,31 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
            '#markup' => '<div class="js-form-item form-item">' . $this->t('An invalid vocabulary is selected. Please change it in the options.') . '</div>',
   

   This text should probably reflect that we're using more than one vocabulary

4. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -149,19 +160,31 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
   +    $vocabulary_labels = array_map(function (VocabularyInterface $vocabulary) {
   

   We can use typed return for the closure. But we compute the list of vocabulary labels also in TaxonomyIndexTid::buildExtraOptionsForm(). Isn't it worth to move the closure as protected method and use it there too?

5. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -149,19 +160,31 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
   -      $terms = $this->value ? Term::loadMultiple(($this->value)) : [];
   +      $terms = $this->value ? Term::loadMultiple($this->value) : [];
   

   Similarly when loading vocabularies, we should use the taxonomy_term storage instead of Term

6. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -149,19 +160,31 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
   -        '#title' => $this->options['limit'] ? $this->t('Select terms from vocabulary @voc', ['@voc' => $vocabulary->label()]) : $this->t('Select terms'),
   +        '#title' => $this->options['limit'] ? $this->t('Select terms from vocabularies @vocs', ['@vocs' => implode(', ', $vocabulary_labels)]) : $this->t('Select terms'),
   
   @@ -238,7 +273,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   -        '#title' => $this->options['limit'] ? $this->t('Select terms from vocabulary @voc', ['@voc' => $vocabulary->label()]) : $this->t('Select terms'),
   +        '#title' => $this->options['limit'] ? $this->t('Select terms from vocabulary @vocs', ['@vocs' => implode(', ', $vocabulary_labels)]) : $this->t('Select terms'),
   

   Shouldn't we enclose with quotes the each vocabulary label?

7. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -195,16 +224,22 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   -        if ($this->options['limit']) {
   -          $query->condition('vid', $vocabulary->id());
   +        $conditions = $query->orConditionGroup();
   +        foreach ($vocabularies as $vocabulary) {
   +          if ($this->options['limit']) {
   +            $conditions->condition('vid', $vocabulary->id());
   +          }
   ...
   +        $query->condition($conditions);
   

   Why not using an IN operator? Then we can pass all vids in only one condition.

8. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -365,7 +400,9 @@ public function adminSummary() {
   -        $this->valueOptions[$term->id()] = \Drupal::service('entity.repository')->getTranslationFromContext($term)->label();
   +        $this->valueOptions[$term->id()] = \Drupal::service('entity.repository')
   +          ->getTranslationFromContext($term)
   +          ->label();
   

   Better not touch this line. Otherwise I'm forced to request this service to be injected :)

Fix tests, #108, coding standards. Needs change record for introduced deprecation.

Oops!

Rerolled.

• Added change notice https://www.drupal.org/node/3162414
• Updated the IS.
• Added release snippet.
• Replaced other 2 usages of non-injected entity.repository service.

Looks really great, just one question really:

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -149,19 +163,31 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
-        '#title' => $this->options['limit'] ? $this->t('Select terms from vocabulary @voc', ['@voc' => $vocabulary->label()]) : $this->t('Select terms'),
+        '#title' => $this->options['limit'] ? $this->t("Select terms from vocabularies @vocabs", [
+          '@vocabs' => $vocabulary_labels,
+        ]) : $this->t('Select terms'),

Should we have a singular/plural here, cause this will look weird if you only have one vocab selected? And since we are generating this title twice maybe do it in a protected method?

On #108.2, I agree with the path you've chosen @claudiu.cristea, showing an error when you have chosen to limit options and then not chosen any vocabs makes sense to me.

@Lendude

Should we have a singular/plural here, cause this will look weird if you only have one vocab selected?

Indeed. Fixed.

And since we are generating this title twice maybe do it in a protected method?

I went with a different approach to resolve the code duplication.

@Lendude, now I see a UX problem when more than one vocabulary is selected. With these vocabs selected:

I see a list of terms but it might be difficult for the site builder to distinguish which term belong to which vocabulary. Moreover, as an edge case, there might be terms with the same name in different vocabularies.

Do you think that we should group select options by vocabulary names? However, I don't see how we can resolve this in the case of autocomplete. Should we show an autocomplete for each vocabulary?

When using this as an exposed filter, I would not want multiple autocomplete fields. The user is going to do a search based on any of the terms with one search filter and suddenly having 5 would be a really weird UX issue.

Could we possibly use something like the Hierarchical Term Formatter to show the terms as 'parent >> child (id#)'

or:

Do something a bit more fancy like integrate functionality similar to the following modules to easily show the hierarchy when searching for terms?
Entity Reference Tree
Hierarchy Manager

I agree with shelane that having multiple autocompletes suddenly appear would be awful ux, and also that having something a bit more than just the taxonomy term id attached - example (21) - would be easier to understand and use.

I agree with @shelane. @W01F, those widgets are looking good. However, I don't think they can be added to Drupal core.

@claudiu.cristea well spotted. For core I don't think we should do more than maybe use optgroups in the selector and only if there are multiple vocabularies selected. If people need something more fancy, they can alter the form and take it from there. How does that sound?

For the autocomplete I don't feel we need to do anything. Yes this can be confusing, but that is not something specific to views, and I don't think we should try to fix it here.

Hi and thank you, everyone!

Back on July 11, we still had an 8.9.x version of this patch -- if I helped create an updated version with the latest changes, would it be supported, will there be a "backport" of this functionality, or no?

Rerolled and attaching a patch for 8.9.x

Re #121, IMO, this is applicable to 8.9.x. 9.1.x is the dev branch currently, in general, patch will be committed to 9.1.x first, and will be backported to 9.0.x and 8.9.x if applicable. So no worries, please.

Forgot uploading raw-interdiff-9.1.x-115-122.txt

@jungle, could you please post a better patch for interdiff using ‘diff -up’ or ‘git diff’ to better understand your changes? Only for 9.1.x would be enough

@claudiu.cristea, the interdiff for 9.1.x is less than 1K, so I would like to try diff -up next time (new to me). As #122 is a reroll (#115 failed to apply), I could not use git diff to make a better interdiff.

@jungle, oh, it’s only reroll? Then there’s no need for interdiff

Re #26, yes, reroll only.

Fixing CS violations and tests failed.

While working on this, I've discovered #3163740: Validation errors of extra options form are muted

Fixed the UX issue as per #120.

Note: The vocabularies field from the extraform is a required field. If you don't select any vocabulary, the form is not submitted and this is expected, as the field is required. However, there's no error message popping-up anywhere. This is a bug and I've opened a new issue to fix that in #3163740: Validation errors of extra options form are muted.

A few generic improvements, please see the interdiff attached for details.

No, please, all these are out-of-scope and make the patch review very hard.

Let me list what I did in #130 based on #129.

I do not think all changes are out of scope, but feel free to ignore it if you/others persist on that it makes reviewing harder.

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -51,11 +61,18 @@ class TaxonomyIndexTid extends ManyToOne {
   +      @trigger_error('Calling TaxonomyIndexTid::__construct() without the $entity_repository argument is deprecated in drupal:9.1.0 and the $entity_repository argument will be required in drupal:10.0.0. See https://www.drupal.org/node/3162414', E_USER_DEPRECATED);
   

   Added __NAMESPACE__

2. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyTermIdFilterUpdateTest.php
   @@ -0,0 +1,62 @@
   +      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.8.0.bare.standard.php.gz',
   ...
   +    $view_as_array = Yaml::decode(file_get_contents(__DIR__ . '/../../../modules/taxonomy_test_views/test_views/views.view.test_filter_taxonomy_index_tid.yml'));
   

   replaced with dirname(__DIR__, LEVELS);

3. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
   @@ -99,6 +123,8 @@ protected function setUp($import_test_views = TRUE): void {
   +    $this->assertFieldByXpath('//select[@id="edit-options-value"]', NULL);
   

   assertFieldByXpath is deprecated. And this line looks unnecessary, so removed

4. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
   @@ -140,6 +166,78 @@ public function testFilterUI() {
   +   * Test filter UI with multiple vocabularies
   

   Should start with a verb "Test" -> "Tests"

5. +++ b/core/modules/taxonomy/tests/src/Functional/Views/TaxonomyIndexTidUiTest.php
   @@ -140,6 +166,78 @@ public function testFilterUI() {
   +    $this->assertIdentical(3, count($xpath));
   ...
   +    $this->assertIdentical(2, count($xpath));
   ...
   +    $this->assertIdentical(1, count($xpath));
   ...
   +    $this->assertIdentical(1, count($xpath));
   ...
   +    $this->assertIdentical(2, count($xpath));
   ...
   +    $this->assertIdentical(1, count($xpath));
   ...
   +    $this->assertIdentical(1, count($xpath));
   ...
   +    $this->assertIdentical(1, count($xpath));
   

   Replaced with assertCount(), a more appropriate one.

6. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -187,7 +187,7 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   -    if ($this->options['type'] == 'textfield') {
   +    if ($this->options['type'] === 'textfield') {
   

   Replaced with ===

I know, but that should be fixed in other issue. Adding out-of-scope code is making hard for the reviewer to read the code.

Adding out-of-scope code is making hard for the reviewer to read the code.

Agree. But all in #132/ #130 are improving newly added code. not adding out-of-scope new code.

Sorry for all the noises here.

@jungle, reviewers are looking to the patch (and intediff). If there are more changes then there are more diffs in the patch. A good patch is one that has minimal changes, so that the reviewer is able understand exactly how the scope of the issue has been achieved by patch. Out-of-scope changes are adding more and more diffs (noise) to the patch, making the review process harder.

Some more stuff I see:

1. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -51,11 +61,18 @@ class TaxonomyIndexTid extends ManyToOne {
   +      @trigger_error('Calling ' . __NAMESPACE__ . '\TaxonomyIndexTid::__construct() without the $entity_repository argument is deprecated in drupal:9.1.0 and the $entity_repository argument will be required in drupal:10.0.0. See https://www.drupal.org/node/3162414', E_USER_DEPRECATED);
   

   Do we need a deprecation test for this?

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -98,34 +116,28 @@ protected function defineOptions() {
              '#description' => $this->t('Select which vocabulary to show terms for in the regular options.'),
   

   should this now be plural?

3. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -133,7 +145,10 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
   -      '#options' => ['select' => $this->t('Dropdown'), 'textfield' => $this->t('Autocomplete')],
   +      '#options' => [
   +        'select' => $this->t('Dropdown'),
   +        'textfield' => $this->t('Autocomplete'),
   +      ],
   

   unrelated change I think?

4. +++ b/core/modules/taxonomy/tests/src/Functional/Update/TaxonomyTermIdFilterUpdateTest.php
   @@ -0,0 +1,62 @@
   +   * Tests that field handlers are updated properly.
   

   c/p left over, it's not a field handler here

5. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -365,7 +388,9 @@ public function adminSummary() {
   @@ -390,8 +415,10 @@ public function getCacheContexts() {
   
   @@ -390,8 +415,10 @@ public function getCacheContexts() {
   +    foreach ($vocabularies as $vocabulary) {
   +      $dependencies[$vocabulary->getConfigDependencyKey()][] = $vocabulary->getConfigDependencyName();
   +    }
   

   Do we need test coverage for this? Probably

6. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -400,4 +427,45 @@ public function calculateDependencies() {
   +  protected function addOption(array &$options, TermInterface $term, array $vocabularies): void {
   ...
   +    if (!empty($this->options['hierarchy']) && $this->options['limit']) {
   

   I think we are missing test coverage for what this method does. Not sure if there is existing coverage for $this->options['hierarchy'] and the - getting added

#127 2784233-8.9.x-127.patch Not working anymore. On Pageload i get this error:

TypeError: Argument 1 passed to Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTid::Drupal\taxonomy\Plugin\views\filter\{closure}() must be an instance of Drupal\taxonomy\Plugin\views\filter\VocabularyInterface, instance of Drupal\taxonomy\Entity\Vocabulary given in Drupal\taxonomy\Plugin\views\filter\TaxonomyIndexTid->Drupal\taxonomy\Plugin\views\filter\{closure}() (line 455 of /web/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php)

• #136.1: No, we don't test parameters additions.
• #136.2-4: Fixed.
• #136.5: Added test coverage. Side note: If a vocabulary is deleted, the whole view gets deleted. But this is true also with the current code, it's not something in the scope of this ticket. As a fix I would be tempted to implement DependentWithRemovalPluginInterface::onDependencyRemoval() but then I realize that removing a filter handler might be a security issue (i.e. suddenly the view shows records that should not be viewable). We need a way to go, in a follow-up.
• #136.6: I'm not sure I get your point. What exactly do we expect to test here? And if the current code lacks coverage, isn't that coverage out-of-scope for this issue?

Account changes from #3101738: Exposed term filters should not show term options that the user does not have access to.

I updated the #127 8.9 patch, so it does not fail any more.

Sorry, missed a couple of files. Will update in a few

Thanks perelesnyk :)

In the meanwhile i've learned one could also dispense this patch and just set multiple vids in form-preprocessing like so:

   $form['form_item_id']['#selection_settings']['target_bundles'] = ['list', 'of', 'mulitple', 'vids'];

Hello @claudiu.cristea,

I tested your patch and I think I found one bug. If a content type has one taxonomy term (lets say Tag1) and another content type has two taxonomy terms (Tag1 and Term new vocabulary) and I create a filter for it, the filter is returning the same content type two times. The one that is returned two times is the one that has two terms from two different vocabulary.

Cheers, Paulo.