-
-
Notifications
You must be signed in to change notification settings - Fork 958
test: add nested property test for uuidfilter #7668
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
base: main
Are you sure you want to change the base?
Conversation
|
looks nice thanks! |
| ), | ||
| 'myDevice.externalId' => new QueryParameter( | ||
| filter: new UuidFilter(), | ||
| property: 'myDevice.externalId', |
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.
You give the property here while you didn't have the need for
'myDevice' => new QueryParameter(
filter: new UuidFilter(),
),
for instance ; is it needed ?
We could have a test with something like
'myDevice.externalId' => new QueryParameter(
filter: new UuidFilter(),
),
'myDeviceExternalIdAlias' => new QueryParameter(
filter: new UuidFilter(),
property: 'myDevice.externalId',
)
no ?
Same for nameConverted like
'nameConverted.nameConverted' => new QueryParameter(
filter: new UuidBinaryFilter(),
),
'nameConvertedAlias' => new QueryParameter(
filter: new UuidBinaryFilter(),
property: 'nameConverted.nameConverted',
),
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.
Without the property, I have the error
{"@context":"\/contexts\/Error","@id":"\/errors\/500","@type":"hydra:Error","detail":"ApiPlatform\\Doctrine\\Orm\\Filter\\AbstractUuidFilter::filterProperty(): Argument #1 ($property) must be of type string, null given, called in xxxxsrc\/Doctrine\/Orm\/Filter\/AbstractUuidFilter.php on line 62","status":500,"type":"\/errors\/500","trace":[{"file":"xxxxsrc\/Doctrine\/Orm\/Filter\/AbstractUuidFilter.php","line":62,"function":"filterProperty","class":"ApiPlatform\\Doctrine\\Orm\\Filter\\AbstractUuidFilter","type":"-\u003E"},{"file":"xxxxsrc\/Doctrine\/Orm\/Extension\/ParameterExtension.php","line":72,"function":"apply","class":"ApiPlatform\\Doctrine\\Orm\\Filter\\AbstractUuidFilter","type":"-\u003E"},{"file":"xxxxsrc\/Doctrine\/Orm\/Extension\/ParameterExtension.php","line":83,"function":"applyFilter","class":"ApiPlatform\\Doctrine\\Orm\\Extension\\ParameterExtension","type":"-\u003E"},{"file":"xxxxsrc\/Doctrine\/Orm\/State\/CollectionProvider.php","line":73,"function":"applyToCollection","class":"ApiPlatform\\Doctrine\\Orm\\Extension\\ParameterExtension","type":"-\u003E"},{"file":"xxxxsrc\/State\/CallableProvider.php","line":43,"function":"provide","class":"ApiPlatform\\Doctrine\\Orm\\State\\CollectionProvider","type":"-\u003E"},{"file":"xxxxsrc\/State\/Provider\/ObjectMapperProvider.php","line":41,"function":"provide","class":"ApiPlatform\\State\\CallableProvider","type":"-\u003E"},{"file":"xxxxsrc\/State\/Provider\/ReadProvider.php","line":84,"function":"provide","class":"ApiPlatform\\State\\Provider\\ObjectMapperProvider","type":"-\u003E"},{"file":"xxxxsrc\/Symfony\/Security\/State\/AccessCheckerProvider.php","line":68,"
I put it in the draft because of that, I'd like to investigate why?
Yes, I can add a test for
'myDeviceExternalIdAlias' => new QueryParameter(
filter: new UuidFilter(),
property: 'myDevice.externalId',
)
and
'nameConverted.nameConverted' => new QueryParameter(
filter: new UuidBinaryFilter(),
),
nameConvertedAlias' => new QueryParameter(
filter: new UuidBinaryFilter(),
property: 'nameConverted.nameConverted',
),
But, I think that with nameConverted.nameConverted, the query parameter will be nameConverted.nameConverted and not name_converted.name_converted....
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.
I put it in the draft because of that, I'd like to investigate why?
This is is maybe the same issue than the one I found in my draft or a related one #7667 ?
You could try removing these lines
https://github.com/api-platform/core/pull/7667/files#diff-5e7788e120b00f67edb2447978c868aac8c117f505e150dbf859f603581a9c55L262-L264
to see if it works better for you without the property (?)
But sure your investigation will be interesting ^^
| filter: new UuidFilter(), | ||
| property: 'myDevice.externalId', | ||
| ), | ||
| 'name_converted.name_converted' => new QueryParameter( |
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.
Can we add a third filter
'nameConverted.nameConverted' => new QueryParameter(
filter: new UuidBinaryFilter(),
property: 'nameConverted.nameConverted',
),
?
Cause the key here is just a "name" for the filter no ? It shouldn't have any impact on the query and therefor a camelCase key should be allowed too no ?)
(In the same idea than #7667)
See #7628 (comment)