Fix ResultQuery::findByPath() for nested paths#1631
Fix ResultQuery::findByPath() for nested paths#1631henriquemoody merged 1 commit intoRespect:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1631 +/- ##
============================================
+ Coverage 98.01% 98.03% +0.01%
- Complexity 956 961 +5
============================================
Files 197 197
Lines 2219 2234 +15
============================================
+ Hits 2175 2190 +15
Misses 44 44 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug in the ResultQuery::findByPath() method that prevented it from finding results when using nested dot-notation paths like "user.email" or "items.1". The fix addresses a fundamental mismatch between how paths are stored (as linked lists via parent references) versus how they were being searched (assuming a tree structure).
Changes:
- Refactored
findByPath()to compute full paths by walking up the parent chain instead of attempting tree-based navigation - Added
parsePath()helper to parse dot-separated paths and convert numeric strings to integers - Added
getFullPath()helper to build the complete path array by traversing parent references - Updated unit tests to properly construct path chains with parent references
- Added comprehensive feature tests demonstrating the fix with real validator scenarios
- Added complete documentation for the
ResultQueryclass and its finder methods
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/ResultQuery.php | Refactored findByPath() method and added helper methods (parsePath() and getFullPath()) to correctly handle nested path lookups by computing full paths from the parent chain |
| tests/unit/ResultQueryTest.php | Updated the nested path test to properly construct Path objects with parent references, matching the actual structure created by validators |
| tests/feature/ResultQueryTest.php | Added new feature tests covering real-world scenarios: nested keys, array indices, and edge cases for all finder methods |
| docs/result-query.md | Added comprehensive documentation for the ResultQuery class, including examples of all finder methods and practical usage patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
henriquemoody
force-pushed
the
core/result-query
branch
4 times, most recently
from
6f107d1 to
2f06da9
Compare
| } | ||
|
|
||
| public function isValid(): bool | ||
| public function hasFailed(): bool |
There was a problem hiding this comment.
I'm coming to the realization that we shouldn't even have that method, instead validate() returns null or the failed result. What do you think, @alganet?
There was a problem hiding this comment.
It makes interfaces awkward:
// Would return null, which juggles to false, misleading since I expect `validate` to enter the if passed
if ($obj->validate()) {
}
IMHO ResultQuery should be compatible with Result. All things you could do with Result, you could also do with ResultQuery, like getting ->hasPassed. This makes implementation weirder (likely a proxy of some kind), but the interface cleaner and would unlock things (if we could use them interchangeably).
There are tools we could use to achieve that, such as PHP's new property hooks: https://www.php.net/manual/en/language.oop5.property-hooks.php
I honestly don't know what the best course of action is here.
There was a problem hiding this comment.
I gotta think a bit more about that. I'm not sure what's the best course of action either. Perhaps we should just give it time, and see what users want from ResultQuery after we rollout 3.0.
The `findByPath()` method was failing to return results when using nested dot-notation paths such as `user.email` or `items.1`. However, it’s returning `null` instead of the expected result in some cases. The root cause was a mismatch between how paths are stored vs searched: - Storage: Validators like Key and Each create results where the path is stored as a linked list. For `user.email`, the "email" result has `path="email"` with `parent="user"`. - Search (old): The method expected a tree structure where it would find a child with `path="user"`, then search that child for `path="email"`. But no child had `path="user"` - only "email" (with "user" as its parent). The fix computes each result's full path by walking up the parent chain and compares it against the search path. Also converts numeric strings to integers when parsing paths (e.g., `items.1` → `['items', 1]`) since array indices are stored as integers. While working on this fix, I also realised that to expose the result's status, it’s best to use `hasFailed()` instead of `isValid()` in `ResultQuery`, since users will mostly use results when validation failed, not when it passed. Assisted-by: Claude Code (Opus 4.5)
henriquemoody
force-pushed
the
core/result-query
branch
from
2f06da9 to
00e7f2a
Compare
2 participants
The
findByPath()method was failing to return results when using nested dot-notation paths such asuser.emailoritems.1. However, it’s returningnullinstead of the expected result in some cases.The root cause was a mismatch between how paths are stored vs searched:
Storage: Validators like Key and Each create results where the path is stored as a linked list. For
user.email, the "email" result haspath="email"withparent="user".Search (old): The method expected a tree structure where it would find a child with
path="user", then search that child forpath="email". But no child hadpath="user"- only "email" (with "user" as its parent).The fix computes each result's full path by walking up the parent chain and compares it against the search path. Also converts numeric strings to integers when parsing paths (e.g.,
items.1→['items', 1]) since array indices are stored as integers.While working on this fix, I also realised that to expose the result's status, it’s best to use
hasFailed()instead ofisValid()inResultQuery, since users will mostly use results when validation failed, not when it passed.Assisted-by: Claude Code (Opus 4.5)