search

glesys / butler-graphql

An opinionated GraphQL package for Laravel.
MIT License
34 stars 5 forks source link

fix: Change default value for BUTLER_GRAPHQL_NAMESPACE. #24

Closed emil-nasso closed 4 years ago

emil-nasso commented 4 years ago

The value of the BUTLER_GRAPHQL_NAMESPACE environment variable is used when resolving graphql queries, mutations and types.

The default value for this namespace is prefixed by \\. When this library resolves a query from the container it resolves object with the \ prefix included. The problem arises when you try to bind or resolve something in the container using ExampleQuery::class as this does not use \ as a prefix.

Example: The namespace in our config is \App\Http\Graphql and we are trying to resolve the Example query. This library will resolve \App\Http\Graphql\Example in the container. We need to inject a specific instance of a class to the constructor of the query so we declare the following:

$app->when(Example::class)->needs(Dep::class)->give(...)

Example::class will expand to App\Http\Graphql\Example. Because this doesn't match \App\Http\Graphql\Example, the binding won't work.

This commit changes the default namespace in the config to use the standard format in php.

Artmann commented 4 years ago

Will this break implementations relying on the current default value? It might be worth adding test cases that describes the old and new behaviour :)

emil-nasso commented 4 years ago

Will this break implementations relying on the current default value? It might be worth adding test cases that describes the old and new behaviour :)

It can definitely break existing implementations, yes. If someone added a $app->when('\App\Something\Something')->needs(..)->give(..) it would work before this patch but not after.

It's kind of an laravel detail that is the issue, not sure how we could add test-cases for that... Will have to think about that one..... :)

❤️

wecc commented 4 years ago

Will this break implementations relying on the current default value? It might be worth adding test cases that describes the old and new behaviour :)

It can definitely break existing implementations, yes. If someone added a $app->when('\App\Something\Something')->needs(..)->give(..) it would work before this patch but not after.

It's kind of an laravel detail that is the issue, not sure how we could add test-cases for that... Will have to think about that one..... :)

❤️

It can break if you manually have registered \App\... in your container AND you use the default butler-guru configuration. If you still have the "\\App\\..." in your config it should still work.

@emil-nasso this PR should also update the README and CHANGELOG