search

thecodingmachine / graphqlite

Use PHP Attributes/Annotations to declare your GraphQL API
https://graphqlite.thecodingmachine.io
MIT License
554 stars 95 forks source link

Bizarre `Cannot query field "settings" on type "...".` error #652

Open josefsabl opened 6 months ago

josefsabl commented 6 months ago

Turns out you cannot query for field starting with set when function is declared like this:

#[Graph\Field]
public function settings(): string

You get Cannot query field "settings" on type "...". error.

Same applies for fields starting with get. function getawayId() ends up in field field awayId.

Obviously there is flaw in how the function names are parsed to match the query fields. Maybe the camel case could be taken into consideration

oojacoboo commented 6 months ago

@josefsabl thanks for reporting. GraphQLite does attempt to parse getters and setters. Overall, I think it does a pretty good job here, but there are exceptions and it's not always transparent. That's an unfortunate aspect of the design. That said, you can specify the field name argument on the Field attribute to override this.

See docs: https://graphqlite.thecodingmachine.io/docs/annotations-reference#field

josefsabl commented 6 months ago

I understand and also understand fixing it would introduce a BC break.

Please note I spent good amount of time trying to figure out what is going on (Double, triple, quadruple checked there is no typo. Double, triple, quadruple checked I am using correct class. Checked whether 'settings'[ is not some sort of reserved word in GraphQL. Invited a colleague to see it and exclude possibility I am crazy. Do a hopeless xdebug code tracing. Then it struck me.) The point is this may happen to more people.

Maybe in some major update I would think about introducing a deprecation for 'setsomething' while 'setSomething' would still work. Maybe add option for the 'setsomething' to continue work.

Also there are not many words except "settings" that make sense in this context and 'tings' is not a word so maybe just adding "settings" on a blacklist would do the trick. But it is little stupid, I guess :-)

Workaround I used is to rename the function from settings() to getSettings().

Anyway, thanks for great library! Have a nice day.

oojacoboo commented 6 months ago

@josefsabl if you'd like to submit a PR on this, we can put it into the next release. We're targeting 7.0 for the next release, as there is a lot in it. I agree that matching for case would be ideal here and should be done.