Upgrade to 7.x-3.23 breaks glossary mode

Please test the latest 7.x-3.23 release, it included a number of fixes to how queries were built and should resolve the problem. Thanks.

Ok, thanks for the clarification.

I'll check with the d.o folks on why the 7.x-3.23 release doesn't show in the version selector.

Might it be possible for you to provide an export of the View configuration that shows the problem? Would you mind cloning the existing view and trimming it down to just the bare fields that show the problem?

Same issue here.
My glossary broke, not allowing multiple arguments.

/glossary-view/a
Worked fine. But
/glossary-view/a,b,c
Only returned "no results behavior".
and preview in views returned this error.
SQLSTATE[HY093]: Invalid parameter number: parameter was not defined

Rolling back to 3.22 solved my issue.
Hoping for resolution in next version.

Thanks.

This regression has been introduced in commit 69c99e6d. The relevant issue was #1930472: Add exclude support to string contextual filter handler. Reverting that commit fixes it. I haven't looked into how to best fix this, yet.

Patch against 7.x-3.x attached. I'll see, if I have the time to add a test.

Added some tests to the existing test for this handler that should cover this regression.

You can reproduce this bug using just the default Glossary view after making a small modification to that view. The modification will allow you to enter multiple arguments separated by + or ,.

In the Edit screen of the view Glossary, under Advanced / Contextual filters, click on Content:title.

In the Content: Title pop-up, under More check Allow multiple values.

Click Apply (all displays).

---

Trying the default Glossary view modified as outlined above produces the results the original reporter and #10 reported.

---

Applying the patch from #13 allows me to view multiple glossary items again.

It also makes the error message on the view edit screen go away.

---

I haven't fully tested the patch, because I don't know well enough what glossary mode is supposed to do.

Two things I noticed that may not be a problem:

- The test is part of the string argument handler tests. Why not the glossary tests?

- According to the setting described above, "if selected, users can enter multiple values in the form of 1+2+3 (for OR) or 1,2,3 (for AND)." I get the same results regardless of whether I use comma (,) or plus (+). In both cases the system uses OR, i.e. displays matches for each argument in the list. To be honest, I have no idea what AND is even supposed to do.

Anyone have time to rewrite the tests per #16? Thanks.

The problem with AND and OR operators in the argument seems to be a much older one, so I think we can assume that wasn't introduced with 7.x-3.23 and is therefore not part of this bug: Contextual filter (string) won't accept multiple values.

Considering that tests remain to be written, I have set the status to Needs Work.

Sorry for taking that long to respond. I've been busy with other stuff...

The test is part of the string argument handler tests. Why not the glossary tests?

I think you are suggesting that we should remove the string argument handler test entirely and merge everything into the glossary test, right? If this is not what you wanted to be done, it would be nice if you could elaborate a little bit on what exactly you think should be done. No interdiff, since it's trivial.

My apologies for the confusion.

There is no "what I think needs to be done". I have only little experience with testing (in Drupal or in general), and practically none with testing in Views. It simply struck me as odd that tests pertaining to glossaries were in the string argument handler tests, but I admit this may very well be fine.

As a result we get into this game of Chinese whispers where I say: "I don't know anything about the subject matter, but this feels strange", the maintainer concludes "the tests must be rewritten", and you end up rewriting tests on the basis of what you think my hunches mean.

Unless I only write about what I know, I don't see how to avoid this, but maybe you could ask what I mean before rewriting tests.

I think you are suggesting that we should remove the string argument handler test entirely and merge everything into the glossary test, right?

I don't know who added the string argument handler tests and why; nor if these tests are referenced elsewhere.

So no, that is not what I was suggesting.

I hadn't really looked deeply in how all the Views tests interact and how they are structured. All I saw was that the assertions you had added in #13 were part of a class ViewsHandlerArgumentStringTest in a file views_handler_argument_string.test, which struck me as odd, because the reporter was having problems with their glossary and the assertions seemed to deal with glossaries.

Let me see if I can come up with a patch that does what I was thinking of at the time.

Don't worry, it's fine with me :). I had made changes to the string handler, so when looking where to best add tests, I immediately found the string handler test and the only thing it was testing was the glossary mode, so I thought it was ok to just extend that. Only after reading your comment I became aware of the glossary test and since everything that was tested in the original string handler test was already tested by the glossary test (actually the original string handler test didn't do very much), I thought that your comment, supposedly about merging everything into one test, was reasonable, but I wasn't entirely sure.

Anyway, feel free to provide a new patch and I'll see if I have time to review it. I guess next time I'll maybe try to track you down in Slack or just ask in the issue, if I'm not entirely sure.

I immediately found the string handler test and the only thing it was testing was the glossary mod

This is the piece of the puzzle that only became clear to me now.

Given that the string handler test only tests glossaries and given that having two different places for testing glossaries has already proven to be confusing, I prefer patch #20 over patch #13.

I can confirm that patch #20 allows me to view multiple glossary items.

I am not going to come up with my own patch as I think #20 works just fine.

This is a full review of patch #20:

- No (new) standards issues found.

- Line 192: I would write 'char' out full as 'character' unless 'char' has a specific meaning here that is different from 'character'.

- Line 225: why no $view->destroy(); here?

- Maybe each cluster of assertions should have a comment explaining what is being tested.

E.g.

// Checks that a glossary view with an argument containing one letter
// returns only the nodes that start with that letter.

// Checks that a glossary view with an argument containing multiple
// values returns only and all nodes that start with these values.

// Checks that a glossary view with a phrase as an argument does
// not interpret that phrase as multiple values.

// Checks that a glossary view with an argument containing one letter
// excludes all nodes that start with that letter.

// Checks that a glossary view with an argument containing multiple
// values excludes all nodes that start with these values.

I do not think this is mandatory as per the coding guidelines, but to me it is not immediately clear from reading the assertions only what each test does; for this I need to read the code. That is why I think comments could be valuable.

In the examples above I have employed terminology as used by Views UI, i.e. 'multiple arguments' instead of 'break phrase', 'exclude' instead of 'not', and so on. This may help others find the tests for each setting.

Thank's for the review!

Line 192: I would write 'char' out full as 'character' unless 'char' has a specific meaning here that is different from 'character'.

Done.

Line 225: why no $view->destroy(); here?

Good catch!

Maybe each cluster of assertions should have a comment explaining what is being tested.

I agree that it is a good idea, also using the wording from the UI to make it more discoverable. I've added the comments you suggested and an extra one for the sorting.

And another change I made on top: When I was retesting the changes locally I discovered that MySQL sorts a little bit different than my PHP implementation, so #20 could have caused random test fails. MySQL places upper case letters first, PHP places lower case letters first. To avoid that I just transform the titles to lowercase now.

Whoops, don't know why I used test as extension for the interdiff instead of txt. Sorry!

I can confirm that after applying patch #25, I can filter on multiple values in a glossary view.

Committed. Thank you.