Allow multiple vocabularies in the taxonomy filter

Rerolled from my previous patch in #219.

Fix CS violations

The last submitted patch, 227: 2784233-227.patch, failed testing. View results →

NW still.

Drupal\Tests\taxonomy\Functional\Update\TaxonomyTermIdFilterUpdateTest::testPostUpdateTaxonomyIndexFilterMultipleVocabularies
There should be no errors in configuration 'views.view.test_filter_taxonomy_index_tid'. Errors:
Schema key views.view.test_filter_taxonomy_index_tid:display.default.display_options.filters.tid.vid failed with: missing schema

Failed asserting that Array &0 (
'views.view.test_filter_taxonomy_index_tid:display.default.display_options.filters.tid.vid' => 'missing schema'
) is true.

This can be fixed by the following:

+  /**
+   * {@inheritdoc}
+   */
+  protected function getConfigSchemaExclusions() {
+    return ['views.view.test_filter_taxonomy_index_tid'];
+  }

But testPostUpdateTaxonomyIndexFilterMultipleVocabularies still fails. Attaching a new patch for who can help or continue.

+++ b/core/modules/views/src/ViewsConfigUpdater.php
@@ -260,8 +260,45 @@ protected function processOembedEagerLoadFieldHandler(array &$handler, string $h
+    return $this->processDisplayHandlers($view, TRUE, function (array &$handler, string $handler_type): bool {

Finally fixed the test by changing the 2nd parameter from TRUE to FALSE

+++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -65,12 +67,18 @@ class TaxonomyIndexTid extends ManyToOne {
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, VocabularyStorageInterface $vocabulary_storage, TermStorageInterface $term_storage, AccountInterface $current_user, protected ?EntityRepositoryInterface $entityRepository = NULL) {
...
+      @trigger_error('Calling ' . __METHOD__ . '() without the $entityRepository argument is deprecated in drupal:10.1.0 and is required in drupal:11.0.0. See https://www.drupal.org/node/3162414', E_USER_DEPRECATED);

Removed the injected service here to make this patch easier to review. Also, fixed failed tests introduced in #231

Change record needs updating. Talks about removing something in D10 for something I don't know is deprecated anymore? Didn't see it.

As a side effect, the taxonomy_index_tid views filter plugin class should be instantiated by passing an additional $entity_repository parameter. Not passing this parameter is deprecated and will be removed in Drupal 10.0.0.

Removed the above from the CR, and the related code was removed in #232 for easier review. This one is optional, which can be done in a child issue of #2729597: [meta] Replace \Drupal with injected services where appropriate in core →

Filed 📌 [11.x] Inject entity.repository service instead of using \Drupal::service('entity.repository') in non-tests Needs work for #234

Been a while since I last looked at this. Went through it again, looks good, follow ups make sense.

I think the test coverage is sufficient, but not very extensive. The only thing I can think of that feels missing is the actual testing of the UI when selecting, but the current implementation doesn't have that either, so doesn't feel like it is mandatory, but others might not agree.

Thanks @Lendude. Let's see how it goes.

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

Should start with "Tests", changing it to "Tests the filter UI with multiple vocabularies.", Keep RTBC.

The last submitted patch, 238: 2784233-238.patch, failed testing. View results →

1) Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testLinkManualDecorator with data set "restricted" (false)

Behat\Mink\Exception\ElementNotFoundException: Element matching css ".ck-balloon-panel_visible .ck-balloon-rotator__content > .ck.ck-link-actions" not found.

It seems irrelevant. Queued another test.

@jungle Given patch #238 is not working for version 10.0.x.

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

   There are at least two contrib projects which reference this schema - https://git.drupalcode.org/search?group_id=2&scope=blobs&search=type:%20...

   If we make this change, their data/schema is now invalid.

   Should we instead be deprecating views.filter.taxonomy_index_tid and adding a new schema type? or leaving vid behind and deprecating it?

   I even wonder if we should be adding a new plugin here, leaving the old one and deprecating it, so we don't break contrib and N other custom plugins people will have written that extend this one.

2. +++ b/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
   @@ -185,30 +193,26 @@ protected function valueForm(&$form, FormStateInterface $form_state) {
   +              return $term->isPublished() || $this->currentUser->hasPermission('administer taxonomy');
   

   Shouldn't this just be using $term->access('view label')?

3. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -260,8 +260,45 @@ protected function processOembedEagerLoadFieldHandler(array &$handler, string $h
   +    if ($this->processTidFilterWithMultipleVocabulariesHandler($handler, $handler_type)) {
   +      $changed = TRUE;
   +    }
   

   this hunk looks to be in the wrong spot now, shouldn't it be inside ::updateAll?

1. Re #241, per a quick manual testing made yesterday, the vid key was still there after running the database update.. #242.1 may be relevant.

2. +++ b/core/modules/taxonomy/taxonomy.post_update.php
   --- a/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_filter_taxonomy_index_tid.yml
   +++ b/core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_filter_taxonomy_index_tid.yml
   

   Meanwhile, found that the view exported is outdated. Maybe a follow-up to update them as they were not introduced by this issue -- out of scope here.

3. diff --git a/core/modules/taxonomy/config/schema/taxonomy.views.schema.yml b/core/modules/taxonomy/config/schema/taxonomy.views.schema.yml
   index 9ec9758185..398b7a130b 100644
   --- a/core/modules/taxonomy/config/schema/taxonomy.views.schema.yml
   +++ b/core/modules/taxonomy/config/schema/taxonomy.views.schema.yml
   @@ -116,9 +116,36 @@ views.field.taxonomy_index_tid:
            type: string
            label: 'Vocabulary'
    
   +views.filter.taxonomy_index_tid_vids:
   +  type: views.filter.many_to_one
   +  label: 'Taxonomy term ID'
   +  mapping:
   +    vid:
   +      type: string
   +      label: 'Vocabulary'
   +    type:
   +      type: string
   +      label: 'Selection type'
   +    hierarchy:
   +      type: boolean
   +      label: 'Show hierarchy in dropdown'
   +    limit:
   +      type: boolean
   +      label: 'Limit to vocabulary'
   +    error_message:
   +      type: boolean
   +      label: 'Display error message'
   +    value:
   +      type: sequence
   +      label: 'Values'
   +      sequence:
   +        type: integer
   +        label: 'Value'
   +
    views.filter.taxonomy_index_tid:
      type: views.filter.many_to_one
      label: 'Taxonomy term ID'
   +  deprecated: "The 'views.filter.taxonomy_index_tid' config schema is deprecated in drupal:10.0.0 and is removed from drupal:11.0.0. Use the 'views.filter.taxonomy_index_tid_vids' config schema instead. See https://www.drupal.org/node/3162414."
      mapping:
        vid:
          type: string
   

   Can we agree on #242.1 to add a new plugin, e.g. name it taxonomy_index_tid_vids with the new schema above and continue?

Thanks, @Lendude for your review.

jungle → closed merge request !386

MR !3582 added a new filter plugin and intended to deprecate the filter plugin taxonomy_index_tid, because there are usages of taxonomy_index_tid in contrib modules pointed by @Lendude in #242.1.

function taxonomy_field_views_data_alter(array &$data, FieldStorageConfigInterface $field_storage) {
  if ($field_storage->getType() == 'entity_reference' && $field_storage->getSetting('target_type') == 'taxonomy_term') {
    foreach ($data as $table_name => $table_data) {
      foreach ($table_data as $field_name => $field_data) {
        if (isset($field_data['filter']) && $field_name != 'delta') {
          $data[$table_name][$field_name]['filter']['id'] = 'taxonomy_index_tid';
        }
      }
    }
  }
}

BUT we can only have one filter in use. The two filters can't coexist well. See the code above in taxonomy.views.inc

So, MR !3681 was opened, which added the vids key to the schema but kept the vid key as deprecated.

All points from #242 were addressed in MR !3681

CR updated

MR !3681 is ready for review.

Updating IS

Functionality wise I still appear to be able to select multiple taxonomies

Deprecation appears to be added per #242

Reading 247 the solution makes sense to me since we can only have 1. Marking this for committer review again.

I was searching the Draft change records and see that this one has the wrong branch and version. Tagging for an update. Does anything else need to be changed in the CR?

Think the CR catches the bulk of what is happening in being able to use multiple taxonomies now.

Updated the branch for 10.1.x but feel it will be 10.2.x in a few days.

See my remark in MR

fixed the mistake I made in previous patch.

I have re-roll the patch for 9.5.x from MR 3681 (https://git.drupalcode.org/project/drupal/-/merge_requests/3681)

I have re-roll the patch for D10.1.1 from MR 3681

Patch in #257 is rejected when applied to Drupal 10.2.0. I could try to reroll it myself, but I'm afraid I might not know exactly what this code is meant to do.

The reason is in `web/core/modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php`. The `*.rej` file reads as follows:

--- modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
+++ modules/taxonomy/src/Plugin/views/filter/TaxonomyIndexTid.php
@@ -160,19 +155,32 @@ public function buildExtraOptionsForm(&$form, FormStateInterface $form_state) {
     ];
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function submitExtraOptionsForm($form, FormStateInterface $form_state) {
+    $vids = $form_state->getValue(['options', 'vids']);
+    $form_state->setValue(['options', 'vids'], array_keys(array_filter($vids)));
+  }
+
   protected function valueForm(&$form, FormStateInterface $form_state) {
-    $vocabulary = $this->vocabularyStorage->load($this->options['vid']);
-    if (empty($vocabulary) && $this->options['limit']) {
+    $vocabularies = $this->vocabularyStorage->loadMultiple($this->options['vids']);
+    if (empty($vocabularies) && $this->options['limit']) {
       $form['markup'] = [
-        '#markup' => '<div class="js-form-item form-item">' . $this->t('An invalid vocabulary is selected. Please change it in the options.') . '</div>',
+        '#markup' => '<div class="js-form-item form-item">' . $this->t('Invalid or no vocabularies are selected. Please select valid vocabularies in filter settings.') . '</div>',
       ];
       return;
     }
 
+    $form['value'] = [
+      '#title' => $this->options['limit'] ? $this->formatPlural(count($vocabularies), 'Select terms from vocabulary @vocabs', 'Select terms from vocabularies @vocabs', [
+        '@vocabs' => "'" . implode("', '", $this->getVocabularyLabels($vocabularies)) . "'",
+      ]) : $this->t('Select terms'),
+    ];
+
     if ($this->options['type'] == 'textfield') {
-      $terms = $this->value ? Term::loadMultiple(($this->value)) : [];
-      $form['value'] = [
-        '#title' => $this->options['limit'] ? $this->t('Select terms from vocabulary @voc', ['@voc' => $vocabulary->label()]) : $this->t('Select terms'),
+      $terms = $this->value ? $this->termStorage->loadMultiple($this->value) : [];
+      $form['value'] += [
         '#type' => 'textfield',
         '#default_value' => EntityAutocomplete::getEntityLabels($terms),
       ];

alt.dev → made their first commit to this issue’s fork.

I created a new branch and re-rolled code to the 10.2.x. core branch.

Attaching the re-rolled patch for the Drupal 10.2.0 version(taken from the new MR !6023)

As mentioned on #258 the patch from #257 can't be applied to Drupal 10.2, therefore I created a new patch for it.

#261 and #262 will not apply to 10.2.x, needs a reroll.

It looks like /core/modules/taxonomy/tests/src/Functional/Views/TaxonomyTermFilterDepthTest.php was removed from core 10.2.3 so I created a patch by removing the diffs pertaining to that file from #262 ✨ Allow multiple vocabularies in the taxonomy filter Needs work to see if it helps.

patch #264(2784233-264-from-262.patch) worked for me in D 10.2.4.
thanks

Updated patch for 10.2.6

Carlos Romero → made their first commit to this issue’s fork.

Updated patch for 10.3.1

Since the merge request changes these test_views do we think that the deprecation message should be expected in core/modules/views/tests/src/Kernel/TestViewsTest.php

• core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_filter_taxonomy_index_tid.yml
• core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_filter_taxonomy_index_tid__non_existing_dependency.yml
• core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_filter_taxonomy_index_tid_depth.yml
• core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.test_taxonomy_exposed_grouped_filter.yml

Here is the failure I am referring to:

---- Drupal\Tests\views\Kernel\TestViewsTest ----
Status    Group      Filename          Line Function                            
--------------------------------------------------------------------------------
Fail      Other      phpunit-149.xml      0 Drupal\Tests\views\Kernel\TestViews
    PHPUnit Test failed to complete; Error: PHPUnit 10.5.35 by Sebastian
    Bergmann and contributors.
    
    Runtime:       PHP 8.3.12
    Configuration: /builds/issue/drupal-2784233/core/phpunit.xml.dist
    
    F                                                                   1 / 1
    (100%)
    
    Time: 00:08.355, Memory: 10.00 MB
    
    There was 1 failure:
    
    1) Drupal\Tests\views\Kernel\TestViewsTest::testDefaultConfig
    Failed asserting that string matches format description.
    --- Expected
    +++ Actual
    @@ @@
     @expectedDeprecation:
    -%A  The 'vid' key in 'views.filter.taxonomy_index_tid' config schema is
    deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. Update your
    view to use the 'vids' key instead. See https://www.drupal.org/node/3162414
    -  Method "Twig\Extension\ExtensionInterface::getFunctions()" might add
    "array" as a native return type declaration in the future. Do the same in
    implementation "Drupal\Core\Template\TwigExtension" now to avoid errors or
    add an explicit @return annotation to suppress this message.
    +  The "Twig\Environment::getTemplateClass()" method is considered
    internal. It may change without further notice. You should not extend it
    from "Drupal\Core\Template\TwigEnvironment".
    +  Method "Twig\Extension\ExtensionInterface::getFunctions()" might add
    "array" as a native return type declaration in the future. Do the same in
    implementation "Drupal\Core\Template\TwigExtension" now to avoid errors or
    add an explicit @return annotation to suppress this message.
    +  Method "Twig\Extension\ExtensionInterface::getFilters()" might add
    "array" as a native return type declaration in the future. Do the same in
    implementation "Drupal\Core\Template\TwigExtension" now to avoid errors or
    add an explicit @return annotation to suppress this message.
    +  Method "Twig\Extension\ExtensionInterface::getNodeVisitors()" might add
    "array" as a native return type declaration in the future. Do the same in
    implementation "Drupal\Core\Template\TwigExtension" now to avoid errors or
    add an explicit @return annotation to suppress this message.
    +  Method "Twig\Extension\ExtensionInterface::getTokenParsers()" might add
    "array" as a native return type declaration in the future. Do the same in
    implementation "Drupal\Core\Template\TwigExtension" now to avoid errors or
    add an explicit @return annotation to suppress this message.
    +  Method "Twig\Loader\FilesystemLoader::findTemplate()" might add
    "?string" as a native return type declaration in the future. Do the same in
    child class "Drupal\Core\Template\Loader\FilesystemLoader" now to avoid
    errors or add an explicit @return annotation to suppress this message.
    +  Method "Twig\Loader\LoaderInterface::exists()" might add "bool" as a
    native return type declaration in the future. Do the same in implementation
    "Drupal\Core\Template\Loader\StringLoader" now to avoid errors or add an
    explicit @return annotation to suppress this message.
    +  Method "Doctrine\Common\Annotations\Reader::getClassAnnotations()" might
    add "array" as a native return type declaration in the future. Do the same
    in implementation
    "Drupal\Component\Annotation\Doctrine\SimpleAnnotationReader" now to avoid
    errors or add an explicit @return annotation to suppress this message.
    +  Method "Doctrine\Common\Annotations\Reader::getMethodAnnotations()"
    might add "array" as a native return type declaration in the future. Do the
    same in implementation
    "Drupal\Component\Annotation\Doctrine\SimpleAnnotationReader" now to avoid
    errors or add an explicit @return annotation to suppress this message.
    +  Method "Doctrine\Common\Annotations\Reader::getPropertyAnnotations()"
    might add "array" as a native return type declaration in the future. Do the
    same in implementation
    "Drupal\Component\Annotation\Doctrine\SimpleAnnotationReader" now to avoid
    errors or add an explicit @return annotation to suppress this message.
    +  Method "Doctrine\Common\Annotations\Reader::getClassAnnotation()" might
    add "?T" as a native return type declaration in the future. Do the same in
    implementation
    "Drupal\Component\Annotation\Doctrine\SimpleAnnotationReader" now to avoid
    errors or add an explicit @return annotation to suppress this message.
    +  Method "Doctrine\Common\Annotations\Reader::getMethodAnnotation()" might
    add "?T" as a native return type declaration in the future. Do the same in
    implementation
"Drupal\Component\Annotation\Doctrine\SimpleAnnotationReader" now to avoid
errors or add an explicit @return annotation to suppress this message.
+  Method "Doctrine\Common\Annotations\Reader::getPropertyAnnotation()"
might add "?T" as a native return type declaration in the future. Do the
same in implementation
"Drupal\Component\Annotation\Doctrine\SimpleAnnotationReader" now to avoid
errors or add an explicit @return annotation to suppress this message.
/builds/issue/drupal-2784233/core/tests/Drupal/TestTools/Extension/DeprecationBridge/ExpectDeprecationTrait.php:78
FAILURES!
Tests: 1, Assertions: 284, Failures: 1.

We should still consider @larolan's suggestion to deprecate the existing plugin and create a new plugin with this functionality. See https://www.drupal.org/project/drupal/issues/2784233#comment-14949645 ✨ Allow multiple vocabularies in the taxonomy filter Needs work

This makes even more sense now that ✨ Continuation Add Views EntityReference filter to be available for all entity reference fields Needs work has landed with the follow-up 📌 Convert TaxonomyIndexTid to use new EntityReference filter Active