TermName views argument_validator is not working as expected

Mh, are you sure this is the expected behaviour?

I would assume that you want to validate all entries. Do you remember how the behaviour has been in Drupal 7?

Can we create some tests for the problem described in IS?

If we have multiple terms existing we need to check that at least one term match one of the vocabulaires not all of them.

Honestly, I would expect that all the terms match the configured list of vocabularies ... because this is what you want to filter by at the end ...

That argument is convincing ... yeah that code is just no obvious to read.
I think instead we should not add the todo here but rather make the entire logic easier to read, until we potentially have multiple terms support.

We have multiple term support. The validator is called once per value in the argument.

(I just updated the IS with a bunch of information)

Fixed pseudo code indentation.

Fixed the IS, sorry for that misunderstanding.

We are going to finalize this patch this afternoon.

This certainly is an improvement, fixes a bug and adds test coverage, even I still think that using the foreach which technically isn't is really confusing.

+++ b/core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php
@@ -71,22 +71,23 @@ public function validateArgument($argument) {
-}
+}
\ No newline at end of file

Do you mind adding a newline? I guess someone can do that on commit as well.

Testbot is still happy and the newline is there.
So, let's RTBC and get this bug fixed!

+++ b/core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php
@@ -71,22 +71,24 @@ public function validateArgument($argument) {
-    $terms = $this->termStorage->loadByProperties(array('name' => $argument));
...
+    $bundles = $this->options['bundles'];
+    if (count($bundles)) {

So if $bundles is empty this will fatal because $terms is not defined. We're obviously missing test coverage.

Modified patch #21 for empty bundles.

That looks good to me.
Thank you @drubb

Looks great, just a bit of nitpicking:

1. +++ b/core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php
   @@ -71,22 +71,26 @@ public function validateArgument($argument) {
   +    // If restricted by bundle.
   

   Can we make this a proper english sentence that actually describes what's happening?

2. +++ b/core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php
   @@ -71,22 +71,26 @@ public function validateArgument($argument) {
   +    if (empty($terms)) {
   

   maybe change this to !empty and move the foreach into this, bit more concise.

to make it more concise added !empty condition

@Shreya Shetty thank you for your work but I think that there is something wrong in the patch you posted.
Previous patch file was 10.1KB, yours is 4.13KB and contains a lot of unwanted changes (mostly on coding standards but not only).

You should restart from #28 and just change what's asked in #33. If you do that, please also post an interdiff file.

If needed, you can find a good documentation of what to do on these pages :
- https://www.drupal.org/contributor-tasks/create-patch
- https://www.drupal.org/novice

I think this is almost ready, but needed a bit of a clean up I feel. Fixed some comments, added some comments, took out the mockStandardInstall bit and just moved it to setup.

Also added a test only patch cause we didn't have one of those yet.

Excellent! Thank you @Lendude!
I made a round of manual testing and the bug is still fixed by the last patch.
Nothing to add about the code neither.
Let's get it commited! :)

1. +++ b/core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php
   @@ -69,22 +69,26 @@ public function validateArgument($argument) {
   -    // Not knowing which term will be used if more than one is returned check
   -    // each one.
   -    foreach ($terms as $term) {
   -      if (!$this->validateEntity($term)) {
   -        return FALSE;
   -      }
   ...
   +      foreach ($terms as $term) {
   +       if ($this->validateEntity($term)) {
   +         // We only need one of the terms to be valid, so return TRUE when we
   +         // find one.
   +         return TRUE;
   +       }
   

   Not the same logic... why has this changed? And is this change correct?

2. +++ b/core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php
   @@ -69,22 +69,26 @@ public function validateArgument($argument) {
   +    if (!empty($terms)) {
   

   loadByProperties always returns an array so checking if empty is not needed.

1. Yeah that's the whole point of this fix, instead of returning FALSE when one term doesn't pass, return TRUE when one term does pass and only return FALSE when all terms don't pass.

2. Good point, fixed.

also put the use statements in the right order.

Still working :)

FILE: ...odules/taxonomy/src/Plugin/views/argument_validator/TermName.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
 84 | ERROR | [x] Line indented incorrectly; expected 8 spaces, found
    |       |     9
 85 | ERROR | [x] Line indented incorrectly; expected 8 spaces, found
    |       |     9
 86 | ERROR | [x] Line indented incorrectly; expected 8 spaces, found
    |       |     9
----------------------------------------------------------------------
PHPCBF CAN FIX THE 3 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------


FILE: ...dules/taxonomy/src/Tests/Views/ArgumentValidatorTermNameTest.php
----------------------------------------------------------------------
FOUND 3 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
 18 | ERROR | [x] Separate the @group and @see sections by a blank
    |       |     line.
 55 | ERROR | [ ] If the line declaring an array spans longer than 80
    |       |     characters, each element should be broken into its
    |       |     own line
 64 | ERROR | [x] Missing function doc comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 82ms; Memory: 5.75Mb

Some minor code style things to fix up.

fixed code style per #45

Thanks @alexpott and @Lendude

$ git ac https://www.drupal.org/files/issues/drupal-2494617-46_0.patch

$ git st
On branch 8.0.x
Your branch is up-to-date with 'origin/8.0.x'.
Changes to be committed:
  (use "git reset HEAD <file>..." to unstage)

	modified:   core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php
	new file:   core/modules/taxonomy/src/Tests/Views/ArgumentValidatorTermNameTest.php
	new file:   core/modules/taxonomy/tests/modules/taxonomy_test_views/test_views/views.view.taxonomy_term_name_argument_test.yml

$ phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,js,css,info,txt,md `git diff --name-only --cached`

$ phpcs --standard=DrupalPractice --extensions=php,module,inc,install,test,profile,theme,js,css,info,txt,md `git diff --name-only --cached`

All clear!

+++ b/core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php
@@ -69,22 +69,24 @@ public function validateArgument($argument) {
-    // Not knowing which term will be used if more than one is returned check
-    // each one.
+    // $terms are already bundle tested but we need to test access control.
     foreach ($terms as $term) {
-      if (!$this->validateEntity($term)) {
-        return FALSE;
+      if ($this->validateEntity($term)) {
+        // We only need one of the terms to be valid, so return TRUE when we
+        // find one.
+        return TRUE;
       }
     }
-
-    return TRUE;
+    return FALSE;

Looking at other argument validators (eg. \Drupal\user\Plugin\views\argument_validator\UserName) I think we should revert this and check emptiness of $terms above. This means that if we ever make the TermName multiple capable then this logic will still work correctly. Also (and more importantly) if the result is not limited by bundle and there a two terms with the same name in different bundles - one which the user has access to and one which they don't - the current behaviour in HEAD is to return FALSE. With the current patch it would return TRUE. I don't think this behaviour change is desired.

@alexpott let's split your comment in two parts.

1- regarding the multiple capacity.
This validator does not need to be multiple capable as Views sends it the values one by one. It might allow to optimize a bit but I'm not even sure it's possible. The change you ask for is quite simple so we could do it, though.

2- regarding the access control.
The purpose of this validator is to validate that the given term name belong to a term in the wanted vocabularies and of which the user has access. Somewhere else or somehow else, Views uses the proper term for its request.
Currently, if two terms have the same name, one you can access and the other you cannot, this validator is going to deny you the access to the View. I don't think it's the behavior we want.

Currently, if two terms have the same name, one you can access and the other you cannot, this validator is going to deny you the access to the View. I don't think it's the behavior we want.

Well at the very least we need to add a test around this behaviour BUT also this is different what happens with Term for example where if you can't access any of the terms it will prevent access. I'm not convinced the behaviour change is desired - if you have per bundle access on Terms you need to limit views using this argument to a specific bundle (imo).

@alexpott about your comparison with the Term validator, I think we cannot really do the same. The thing is that the Term validator take a TID as argument which cannot match more than one term. So it's normal that if the user cannot access this term we deny immediately return FALSE. In our case, the same term name can match a lot of different terms, even in the same vocabulary. Plus, I agree with @Bès about the regression, I made the same tests as him under D7 and it used to work well.

Do we still need to revert something as you said in #48 or is the last patch OK?

@Bès nice work testing this on D7.

So this is tricky because something that was an empty view currently in D8 will now have a response but it appears that this is the way it was in D7. I definitely think we need a test of the behaviour regardless of how we solve it and I would like the views maintainers to have a think about what is best to do...

1. +++ b/core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php
   @@ -69,22 +69,24 @@ public function validateArgument($argument) {
        foreach ($terms as $term) {
   -      if (!$this->validateEntity($term)) {
   -        return FALSE;
   +      if ($this->validateEntity($term)) {
   +        // We only need one of the terms to be valid, so return TRUE when we
   +        // find one.
   

   Regarding changing the logic back to d7. Beside earlier disagreements in the issue, I agree now, especially because we just support the single usecase currently, this change is what we want. What matters is to make it possible to see information, those would be otherwise exposed on the site most probably anyway.

2. +++ b/core/modules/taxonomy/src/Tests/Views/ArgumentValidatorTermNameTest.php
   @@ -0,0 +1,118 @@
   +    // Test a term that exists in only one vocabulary.
   +    $this->assertTrue($view->argument['name']->setArgument($this->term2->getName()));
   +    $this->assertEqual($view->argument['name']->getTitle(), $this->term2->label());
   +    $view->argument['name']->validated_title = NULL;
   +    $view->argument['name']->argument_validated = NULL;
   +
   +    // Test a term that exists in at least two vocabularies.
   +    $this->assertTrue($view->argument['name']->setArgument($this->term1->getName()));
   +    $this->assertEqual($view->argument['name']->getTitle(), $this->term1->label());
   +    $view->argument['name']->validated_title = NULL;
   +    $view->argument['name']->argument_validated = NULL;
   +
   +    // Pass in an invalid term.
   +    $this->assertFalse($view->argument['name']->setArgument(rand(1000, 10000)));
   +    $view->argument['name']->validated_title = NULL;
   +    $view->argument['name']->argument_validated = NULL;
   

   Yeah adding some test coverage for multiple passed in names would be nice, just to check/document what happens (it won't be splitted). Adding test coverage for the bundle filtering would be nice as well.

56.1 Not quite sure what you mean. Is it ok as it is, or should we change anything?

56.2 Added two tests.

56.1 Not quite sure what you mean. Is it ok as it is, or should we change anything?

Yes, it is okay. I was just thinking loud.

+++ b/core/modules/taxonomy/src/Tests/Views/ArgumentValidatorTermNameTest.php
@@ -109,10 +118,23 @@ public function testArgumentValidatorTermName() {
+
+    // Test filtering by bundle by passing a term from a vocabulary that is not
+    // in the bundle settings.
+    $this->assertFalse($view->argument['name']->setArgument('nonbundle'));
+    $view->argument['name']->validated_title = NULL;
+    $view->argument['name']->argument_validated = NULL;

What about adding the term to two vocabularies, one which is allowed and one which isn't as well?

+++ b/core/modules/taxonomy/src/Tests/Views/ArgumentValidatorTermNameTest.php
@@ -0,0 +1,140 @@
+    // Test a term that exists in at least two vocabularies.
+    $this->assertTrue($view->argument['name']->setArgument($this->term1->getName()));
+    $this->assertEqual($view->argument['name']->getTitle(), $this->term1->label());
+    $view->argument['name']->validated_title = NULL;
+    $view->argument['name']->argument_validated = NULL;

that's this one, was already in there, might need to update the comment to make that a little more clear

that's this one, was already in there, might need to update the comment to make that a little more clear

Oh interesting, but I thought about the case where this is valid ... when the term actually exists.

Sorry, I don't follow your line of thought. In that test the terms actually exists in both vocabularies, one that is in the the bundle settings, and one that isn't.

Please call be stupid, but why is this then an invalid term name?

Is it a simple way in Core to deny access to a particular taxonomy term? It'd make that test easier.

Dumb question: does using the term name validator works for you using the patch? I mean, the path is working fine and fixes the validator in general, but ... the term name is never converted to a term ID and this filters the result query using a wrong value.
Using the validator and a simple term reference field as argument, the query looks like this:
WHERE (( (node__field_channel.field_channel_target_id = 'ideas' ) ) ...

In Drupal 7 we had an additional option how to handle the argument ("Filter value type"). Do we want to convert the term name always to the ID or do we need to implement the value type selector?

Added the conversion to always use the term ID.

No, I've used the term field directly without any reference. I expected it to work, since Drupal 7 acted like this ...

In D7 the behaviour was you get a name and it'll be convert to an ID, right?

if in D8 we know that looking for ID is working fine... why duplicate code?

Ok, I see the point here.
So its either using different validators for different requirements (TermNameToTid, etc.) or implement something like the type-selector as in Drupal 7.

Rerolled @stBorchert's patch from #65 without the term name to term ID conversion.

Rerolled against 8.4.x

I did some manual testing while writing the steps to reproduce and reviewed the coding standards while rerolling.
If the testbot is OK, it should be good to go.

#76 has been working for us for many months.

Agreed, this validator is definitely not working as expected. Especially when used with the most obvious possible contextual filter: core's "Has taxonomy term ID". In D7, there was a single taxonomy term validator that let you choose the style of validation you wanted, and it had an option for 'Term name converted to Term ID' which is actually what you want (since it leads to the most efficient queries). Sadly, all that is (for now) gone from D8 core.

I'd like to completely fix this at some point, consolidate all the term-related argument validation into a single plugin that lets you configure what kind of validation (and optional transformation) you want. It would simultaneously let this use case work smoothly, and simplify the UI by removing the 2 different 'Taxonomy term name' and 'Taxonomy term ID' choices (presented to all possible contextual filters).

Until then, for anyone else who needs the good ole' 'Term name converted to Term ID' functionality, enjoy:

https://www.drupal.org/project/views_taxonomy_term_name_into_id

The patch in #75 is working for me as well. I'm testing against a very basic case that fails currently with a term name in 2 different vocabularies.

This bug can be very confusing for site builders so hopefully we can get this into 8.6 before code freeze.

I encountered this and it definitely caused a good moment of confusion for me too.

The patch in #75 still applied cleanly to 8.5.x and 8.6.x for me. It's working to resolve the same case, of a single-value argument validated against one vocabulary, and with two terms with the same name in different vocabularies.

Considering #78, #81 and #83, I can also confirm this is working for me, and am bumping to RTBC for maybe the next step review...?

+++ b/core/modules/taxonomy/src/Plugin/views/argument_validator/TermName.php
--- /dev/null
+++ b/core/modules/taxonomy/src/Tests/Views/ArgumentValidatorTermNameTest.php

+++ b/core/modules/taxonomy/src/Tests/Views/ArgumentValidatorTermNameTest.php
@@ -0,0 +1,135 @@
+/**
+ * Tests the plugin of the taxonomy: term argument validator.
+ *
+ * @group taxonomy
+ *
+ * @see Views\taxonomy\Plugin\views\argument_validator\Term
+ */
+class ArgumentValidatorTermNameTest extends TaxonomyTestBase {

This needs to be converted to use \Drupal\Tests\taxonomy\Functional\TaxonomyTestBase and moved to core/modules/taxonomy/tests/src/Functional/Views/ArgumentValidatorTermNameTest.php as WebTestBase tests are deprecated. At this point we should not be adding more.

FWIW #75 still works great for me on 8.6.10!

I would like to confirm #75 is working for me on 8.6.13.

#75 works for me on 8.7.3.

(#75 still works on 8.7.7!)

Duplicates: #3202689: Term name as contextual filter doesn't work if there are terms in other taxonomies and #3029807: Views Taxonomy contextual filter issue closed

Adding a patch with a working test for this.

I had raised the dupplicate #3029807 and can verify that as far as I can see, #75 is doing exactly the same thing as the patch I had provided there.

#99 worked great. Attempting to mark Reviewed and Test by the Community.

I'm interested in helping to get this issue over the finish line. The latest patch has tests, but there are two problems with the tests:

1. They have never been run by the testbot without the fix applied
2. The tests could more closely imitate the tests for other argument_validators.

I'm attaching a test-only patch that uses the test View that is present in that latest patch (with the change of the name tags to views_testing_tags). I have removed the Functional test and replaced it with a Kernel test that is very closely modeled off of the test for the taxonomy term id argument validator. Kernel is faster than Functional, so this gains us speed in addition to consistency.

There's no interdiff because I don't think it would help.

Update: This is a long and winding issue, no? I just found comment #57 which included a test that is nearly identical to the one I added, and a slightly less ambitious one was added in #39. That one was even run as a test-only fail patch. Somehow that test got lost and was replaced with the Functional test that was still present in #99.

Fixing spelling...

The new test failed just as anticipated. Removing the Needs tests tag.

Now I'm adding the fix from #99. This should pass.

A couple things I'm thinking about now that I've spent more time reading through the issue:

1. The test in #57 has a nice test about passing multiple names (which is not supported). It may be worth adding that in. (I essentially reproduced the patch in #57 independently in #104 and #105).
2. There is discussion in #48 and #49 about a scenario involving certain permissions configurations. As far I as I can tell there was never testing related to this discussion but tests were suggested for this scenario.

We could go on all day with adding tests though. Part of the problem here is that there is apparently no existing (direct) test coverage for the taxonomy_term_name argument validator. So how much coverage do we need to add before we are happy committing this fix that has been used by many sites for many years? It's a tough situation.

I figured it wouldn't hurt to just go ahead and bulk up the test coverage. Here's a new patch with more tests. If you look at the interdiff, you'll see that this is purely additions, with the exception of a changed comment. Therefore we don't need to do a new test-only patch.

I think the new access check tests give us a very good frame of reference for further discussion if we need it. There's a few more additions too including validation of multiple names (not supported) and tests that change the bundles array.

Confirm the patch #109 works great. Thanks a lot!

That was a random fail. Resetting status.

Committed/pushed to 10.1.x and cherry-picked back through to 9.4.x, thanks!

Removing the 'needs subsystem maintainer' review tag since both dawehner and lendude have reviewed this in the six years since that tag was added.