Combined: Keep standard recommendations that are not overridden

[maccabeelevine]

Fixing an issue discovered in #3258

In AbstractSearch->getSeachSetupCallback(), any recommendOverride settings in combined.ini overrride all of the default recommendation modules from searches.ini / EDS.ini etc. So setting even a top recommendation in combined.ini overrides the noresults recommendations from EDS.ini.

[demiankatz]

Thanks, @maccabeelevine, this looks like a promising start! I'll do some deeper testing on this tomorrow if time permits, but in the meantime, one small suggestion...

[demiankatz]

Maybe if (is_array($override)) { to prevent unexpected behavior if invalid parameters are passed in?

[maccabeelevine]

I made this change. I'm a little skeptical because that check wasn't done in the legacy code so it changes the behavior, and I'm not sure $override being an array tells us too much about it having valid content. But it's still incremental improvement, so probably no harm.

[demiankatz]

My goal is to avoid fatal errors. If you give the code a junk array, the junk data will be ignored. But if you pass a non-array to array_merge, you'll blow everything up entirely. :-)

[demiankatz]

Thanks, @maccabeelevine, this seems to be working as expected for me.

However, while testing it, I discovered a semi-related problem -- the "include_recommendations = true" setting no longer worked correctly -- it was broken in #3135. I've fixed the problem here, but I'm also going to open a PR targeting release-9.1 to get that bug sorted out. Once we get that approved and merged, I think we can go ahead and merge this too.

[maccabeelevine]

However, while testing it, I discovered a semi-related problem -- the "include_recommendations = true" setting no longer worked correctly -- it was broken in #3135. I've fixed the problem here, but I'm also going to open a PR targeting release-9.1 to get that bug sorted out. Once we get that approved and merged, I think we can go ahead and merge this too.

Sorry I missed that! 'true' is actually the first value documented in combined.ini, not sure how I let that slip.

[demiankatz]

Thanks for the fix, @maccabeelevine!