-
Notifications
You must be signed in to change notification settings - Fork 138
feat: fixes errors on complex where statements and tests required #349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
garak
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply suggestions given by copilot
it breaks the tests if I apply that since It will be wrong typed |
…y/WhereWalkerTest.php Co-authored-by: Copilot <[email protected]>
…y/WhereWalkerTest.php Co-authored-by: Copilot <[email protected]>
…RM/Query/WhereWalker.php Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Enhance the WhereWalker to support nested ConditionalTerm nodes and add tests for complex and simple filter scenarios in WHERE AST traversal.
- Extend
primaryContainsFilterto detect filter expressions withinConditionalTermnodes - Add unit tests covering complex
ConditionalTermcombinations and single-factor filters - Ensure existing filter logic remains intact with additional assertions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/Test/Pager/Event/Subscriber/Filtration/Doctrine/ORM/Query/WhereWalkerTest.php | Add tests for complex conditional terms and simple conditional factors in WHERE clauses |
| src/Knp/Component/Pager/Event/Subscriber/Filtration/Doctrine/ORM/Query/WhereWalker.php | Update primaryContainsFilter to handle ConditionalTerm branches alongside ConditionalExpression |
Comments suppressed due to low confidence (2)
src/Knp/Component/Pager/Event/Subscriber/Filtration/Doctrine/ORM/Query/WhereWalker.php:226
- The newly added
ConditionalTermbranch is not exercised by current tests. Add a test that triggerstermContainsFilter()to verify correct handling of this case.
} elseif ($node->conditionalExpression instanceof ConditionalTerm) {
src/Knp/Component/Pager/Event/Subscriber/Filtration/Doctrine/ORM/Query/WhereWalker.php:224
- The
ConditionalTermbranch may never be reached becauseconditionalExpressionis always aConditionalExpression. Consider refactoring to traverse the expression's terms and invoketermContainsFilter()where appropriate.
if ($node->conditionalExpression instanceof ConditionalExpression) {
Improves #206 adding the missing tests required