-
-
Notifications
You must be signed in to change notification settings - Fork 848
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
fix: allow search filter to use an int for its value #4295
Conversation
1. First test will fail because the relatedDummies.age is an int bug reported here api-platform#4290 2. Second test will work because the relatedDummies.name is an string.
Fixes test that fails in 2.6: d33159f Related/Reported in api-platform#1633 api-platform#4196 api-platform#2190 api-platform#2308 api-platform#4290
@@ -136,7 +136,7 @@ protected function getIdFromValue(string $value) | |||
protected function normalizeValues(array $values, string $property): ?array | |||
{ | |||
foreach ($values as $key => $value) { | |||
if (!\is_int($key) || !\is_string($value)) { | |||
if (!\is_int($key) || \is_array($value)) { |
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.
What value will not fit the first part of the IF then ? I mean an array will already be validated by the first part of the OR condition isn't it ?
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.
First part of the IF it's the $key, second part it's about the $value. If $value is an INT then a notice will appear. I wrote a test about that here:
features/graphql/collection.feature
Outdated
@@ -909,3 +909,65 @@ Feature: GraphQL collection support | |||
Then the response status code should be 200 | |||
And the response should be in JSON | |||
And the JSON node "data.fooDummies.collection" should have 1 element | |||
|
|||
@createSchema | |||
Scenario: Retrieve a collection with a nested collection filtering by age through a GraphQL query |
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.
Wouldn't it be better in filters.feature
instead?
features/graphql/collection.feature
Outdated
} | ||
} | ||
""" | ||
|
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.
Line to remove.
features/graphql/collection.feature
Outdated
|
||
Then the JSON node "data.dummies.totalCount" should be equal to 1 | ||
And the JSON node "data.dummies.edges[0].node.relatedDummies.totalCount" should be equal to 1 | ||
Then the JSON node "data.dummies.edges[0].node.relatedDummies.edges[0].node.age" should be equal to "31" |
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.
Then the JSON node "data.dummies.edges[0].node.relatedDummies.edges[0].node.age" should be equal to "31" | |
And the JSON node "data.dummies.edges[0].node.relatedDummies.edges[0].node.age" should be equal to "31" |
Thank you for the Behat tests! I think it would be nice to have unit ones too. I will handle them. |
Thanks! I just added a fix with your suggestions. |
I just fix for the behat test (subresource.feature) that was failing because the age filter was added. |
Thank you @jmontoyaa. |
Same fix as #4290 but with tests and in branch 2.6