Skip to content
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: Change default value for BUTLER_GRAPHQL_NAMESPACE. #24

Merged
merged 2 commits into from
Jan 30, 2020

Conversation

emil-nasso
Copy link
Contributor

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.

@brother brother requested a review from wecc January 22, 2020 08:23
@Artmann
Copy link
Contributor

Artmann commented Jan 22, 2020

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
Copy link
Contributor Author

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
Copy link
Member

wecc commented Jan 23, 2020

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

Emil Andersson and others added 2 commits January 28, 2020 11:17
As this is a library, we have no need for a checked in lock-file.
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.
@emil-nasso emil-nasso merged commit 44348b1 into master Jan 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants