Fix DefaultParametersListener and add Mink test

[maccabeelevine]

As far as I can tell, default parameters are not working at all. It seems that applying the command pattern to the search listeners in #1967 missed DefaultParametersListener.

As defined in searches.ini, this should work

;default_parameters[*] = "echoParams=none&debugQuery=false"
;default_parameters[search] = "sow=false"

[maccabeelevine]

Before I fix the unit tests for this, and then rewrite both the listener and the tests for #4991, I would appreciate a sanity check that a) the feature is actually broken and b) it's worth fixing if no one has noticed since 2021. @EreMaijala ?

[maccabeelevine]

b) it's worth fixing if no one has noticed since 2021.

I'm asking this partially because #4991 will require changing how the configuration of this feature works anyway -- JSON-style configuration instead of URL query parameters -- with a backwards compatibility layer. So if no one is currently using it but we do want to keep it, maybe I care less about building the backwards compatibility part.

[EreMaijala]

@maccabeelevine You're right, it's totally broken. And I'm really surprised that nobody has noticed. But since it obviously hasn't worked for several years, maybe we could just fix it with #4991 without back-compatibility?

[demiankatz]

Thanks, @maccabeelevine and @EreMaijala. I'd propose two possible paths forward:

1.) If the work here solves the problem and is nearly complete, I don't see harm in backporting it to the release-11.0 branch so we can have a fix sooner rather than later.

2.) If it would be significant effort to finish this up, and since no one is actually complaining, we could defer to #4991 as @EreMaijala proposes -- but if we do that, I think we should at least go to the effort of opening a JIRA ticket to document which versions are impacted by the bug, in case somebody notices in the future.

I slightly prefer option 1 since psychologically, I hate having known bugs sitting around in the code... but I'm willing to be pragmatic if letting this one go helps us make better progress in more important areas. :-) Let me know if you need my help with anything, whichever path you choose!

[maccabeelevine]

I decided to go ahead and fix the tests, since most of that work (changing to use command pattern) will have to be done later anyway if we're keeping the feature around.

No decision yet on how much backwards compatibility to maintain later, can defer that.

[demiankatz]

Thanks, @maccabeelevine, this looks good to me and seems to be working as expected now. A few thoughts/questions:

1.) We should probably backport this to the release-11.0 branch. I can help if you want me to!

2.) I wonder if the unit test would be more readable if we broke the test cases down into smaller parts. Right now, some of the tests are testing multiple things, and it might not be too hard to break them down into separate functions for greater clarity. Not necessary or high priority, but if you're feeling a perfectionist streak, maybe worth considering.

3.) We should create a Mink test to confirm that this really works, since this is a perfect case of a unit test continuing to pass even though the integration was broken. Something simple, like setting the search default parameters to fq=id:testbug2 and then confirming that a blank search retrieves that one record might be an easy start.

Please let me know how you'd like to proceed; I can help with any part of this if you don't have time. In any case, thanks for catching and fixing this long-standing problem!

[maccabeelevine]

Thanks, @maccabeelevine, this looks good to me and seems to be working as expected now. A few thoughts/questions:

I refactored the unit test and wrote the simple mink test you suggested -- good call.

I tried the rebase but hit some merge errors, would not mind if you handle this one!

[demiankatz]

Thanks, @maccabeelevine, this looks great!

The conflicts you ran into were caused by Rector test improvements that were added to dev after the release of 11.0; I simply backported them into this branch since they're harmless changes.

I've also made a couple of minor tweaks to your Mink test -- your config setting was using bracket syntax, and I changed it to a real deeply-nested array for consistency; I also added an extra check to confirm that we're not only showing exactly 1 record but also that we're showing the expected record.

All of this gives me confidence that this is safe to merge. Thank you again!