Add support for name converter in GraphQL #2765
Conversation
|
Nice! For the unit tests I'm refactoring the schema builder, it will be a lot more easier to add them then. I hope to do the PR this weekend. |
|
Can you rebase @antograssiot? It would be nice to have it in the next patch release. |
antograssiot
force-pushed
the
graphql-name-converter
branch
from
2611fec
to
5f91337
Compare
|
rebased and added on top of the recent refactoring. |
|
This PHPStan error appear often recently on master, I'm really not sure why but it is definitely not related to this PR I think 🤔 |
| @@ -15,6 +15,7 @@ | |||
| <argument type="service" id="serializer" /> | |||
| <argument type="service" id="api_platform.metadata.resource.metadata_factory" /> | |||
| <argument type="service" id="api_platform.security.resource_access_checker" on-invalid="ignore" /> | |||
| <argument type="service" id="api_platform.name_converter" on-invalid="ignore" /> | |||
There was a problem hiding this comment.
I think we could add it before and remove the on-invalid. There is no BC policy for GraphQL and we have already done a lot of BC breaks in master anyway.
There was a problem hiding this comment.
Actually there's not always a name_converter in every app, that's why it is nullable @alanpoulain, not really for BC reason bit I might be wrong...
There was a problem hiding this comment.
OK. Could you just change its position then (before a parameter for instance)?
There was a problem hiding this comment.
Well optional parameters are always supposed to be the last ones, and parameter are not optional unless I'm wrong @alanpoulain ?
There was a problem hiding this comment.
Because you are adding on-invalid="ignore", it will be replaced by null if the service doesn't exist.
IMO we have to use an optional parameter at the last and with a default value only for BC policy: we don't want a decorator to break for instance.
| @@ -26,6 +27,7 @@ | |||
| <argument type="service" id="api_platform.security.resource_access_checker" on-invalid="null" /> | |||
| <argument type="service" id="request_stack" /> | |||
| <argument>%api_platform.collection.pagination.enabled%</argument> | |||
| <argument type="service" id="api_platform.name_converter" on-invalid="ignore" /> | |||
antograssiot
force-pushed
the
graphql-name-converter
branch
2 times, most recently
from
305a65a
to
5c5db23
Compare
|
I guess this one is ready. I've added support for nested filtering even when using name converters like done in #2897 To do so I had to create a new configuration value because the way we support nested properties (using the underscore (_) syntax instead of the dot (.) syntax ) is problematic when the property name contains underscore(s). To avoid breaking BC and to allow users that need this feature, I've introduced the possibility to choose the separator that should be used using a new configuration option. I don't think that it is worth deprecating using the actual value ( |
antograssiot
force-pushed
the
graphql-name-converter
branch
from
5c5db23
to
c194662
Compare
|
I suggest to I lu support the dot. Underscore is weird, and was not intended imho. Let’s not complexify the code, BC breaks are allowed for GraphQL. |
@dunglas, as far as I remember the dot is not allowed in GraphQl queries, that's why underscores where used in the first place, this PR didn't changed that. ping @alanpoulain |
|
Yes the dot is not allowed in names: https://github.com/graphql/graphql-spec/blob/master/spec/Section%202%20--%20Language.md#names |
antograssiot
force-pushed
the
graphql-name-converter
branch
2 times, most recently
from
c9a84d7
to
4fa26ba
Compare
alanpoulain
force-pushed
the
graphql-name-converter
branch
from
4fa26ba
to
64a33a8
Compare
|
Thanks a lot @antograssiot! It would be great if you could do the doc PR too 🙂 |
That will also make filtering possible with #2751
I could add unit tests if you feel it is needed and the idea/implementation is OK