fix: send search_type=query_then_fetch as a top-level param in Search::count()#2302
fix: send search_type=query_then_fetch as a top-level param in Search::count()#2302ruflin wants to merge 1 commit into
Conversation
…::count()
The `$params` literal in `Search::count()` wrapped the search-type option
in a numeric-keyed sub-array:
$params = [
'body' => $query->toArray(),
[self::OPTION_SEARCH_TYPE => self::OPTION_SEARCH_TYPE_QUERY_THEN_FETCH],
];
The upstream `Client::search()` ignores numeric-keyed sub-arrays, so the
option never reached Elasticsearch. Hoist the entry to a string-keyed
member of `$params` and add a functional test asserting it lands in the
HTTP request's query string.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis change fixes a bug in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 4/8 reviews remaining, refill in 23 minutes and 6 seconds.Comment |
There was a problem hiding this comment.
Pull request overview
Fixes Elastica\Search::count() so the search_type=query_then_fetch option is sent as a proper top-level request parameter (instead of being wrapped in a numeric-keyed sub-array that gets ignored by the upstream client), and adds a functional test to assert the query parameter is present on the outgoing HTTP request.
Changes:
- Flattened the
Search::count()$paramsarray sosearch_typeis a top-level key. - Added a functional test verifying
search_type=query_then_fetchis present in the last request’s query string. - Documented the fix in the Unreleased changelog.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Search.php |
Fixes request parameter construction in Search::count() so search_type is actually transmitted. |
tests/SearchTest.php |
Adds a functional regression test asserting search_type is present in the outgoing request. |
CHANGELOG.md |
Records the bugfix in the Unreleased “Fixed” section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### Deprecated | ||
| ### Removed | ||
| ### Fixed | ||
| * Fixed `Elastica\Search::count()` silently dropping the `search_type=query_then_fetch` option due to a malformed array literal that wrapped it in a numeric-keyed sub-array. |
There was a problem hiding this comment.
In the Unreleased section, other changelog bullets include a PR/issue reference link; this entry currently has none. Consider appending the PR number link for consistency and easier traceability.
Summary
Elastica\Search::count()built its parameter array like this:That second entry is a numeric-keyed sub-array, which the upstream
elasticsearch-phpClient::search()ignores. The intendedsearch_type=query_then_fetchquery parameter never made it toElasticsearch.
The fix flattens it back to a string-keyed entry. A new functional test
asserts that the option arrives in the HTTP request's query string.
Identified during a P0/P1 code-review pass.
Test plan
Summary by CodeRabbit
Bug Fixes
search_type=query_then_fetchoption was not being properly preserved due to incorrect parameter structure.Tests
search_typeparameter is correctly set and included in search requests.