Fix duplicate 'rows' param from SimilarBuilder

[maccabeelevine]

SimilarBuilder->build intends to avoid setting a rows param if it already exists

        if (null === $params->get('rows')) {
            $params->set('rows', $this->count);
        }

But $params is constructed fresh in the same method, so the condition is always true and rows is always set. Then in Backend->similar, these new params from SimilarBuilder are merged with any that were passed into similar, which sometimes already contains rows. This leads to two rows params, and ultimate query params like:

/select?fl=%2A&rows=24&rows=5&wt=json&json.nl=arrarr&q=id%3A%22topcollection2%22&qt=morelikethis

(see the duplicate rows)

This fixes the original logic by passing in any existing $params, so that rows is actually not set if it shouldn't be.

[maccabeelevine]

This becomes a problem with #4991 as Solr's JSON API will complain if we pass in the parameter twice (now called limit).

[demiankatz]

Thanks, @maccabeelevine, this looks reasonable to me, though I haven't had a chance to run tests on it yet -- probably won't have a chance until next week.

Curious about what timing you'd like to see on this -- I see that you targeted it against release-11.0, but since it changes an interface, it's technical a backward-breaking change... and since this is fixing a problem that (as far as I can tell) doesn't have a serious impact until work slated for 12.0 comes into play, maybe it would be more conservative to target this to dev-12.0. That being said, if the fix is more critical than I'm interpreting, I'd be willing to bend the rules -- but if it's not, we might as well go by the book and merge to a later stage of the process.

What do you think?

[maccabeelevine]

Thanks, @maccabeelevine, this looks reasonable to me, though I haven't had a chance to run tests on it yet -- probably won't have a chance until next week.

Curious about what timing you'd like to see on this -- I see that you targeted it against release-11.0, but since it changes an interface, it's technical a backward-breaking change... and since this is fixing a problem that (as far as I can tell) doesn't have a serious impact until work slated for 12.0 comes into play, maybe it would be more conservative to target this to dev-12.0. That being said, if the fix is more critical than I'm interpreting, I'd be willing to bend the rules -- but if it's not, we might as well go by the book and merge to a later stage of the process.

What do you think?

Argh, you're right, I didn't consider the breaking change aspect. I've submitted so many bug fix PRs recently that I had to rebase to 11.0 later that I just assumed. I don't think the bug is really critical to current behavior, as Solr just ignores one of the two query parameters -- I'm honestly not sure which one, but if it was doing things backwards, someone would have noticed in the last decade...

I'm fine re-targeting to dev-12, unless someone wants to look first to see if there's any serious impact currently.

[maccabeelevine]

I'm fine re-targeting to dev-12, unless someone wants to look first to see if there's any serious impact currently.

With no objections, I did rebase this.

[demiankatz]

Thanks, @maccabeelevine, this looks good to me now! (I mean, it already looked good, but now it's in a place where I feel safer merging it. ;-) ).

[demiankatz]

(Also, for the record, the only place in the code I'm aware of that uses the rows parameter is the Similar channel provider, and I confirmed that it works the same both before and after the changes here... so I don't think there are any obvious bugs impacting existing code, and this is just future-proofing us for the JSON behavior).