Fix FluentResolver when tagging two services with same class name #297
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
mcg-web
commented
Mar 1, 2018
mcg-web
commented
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | yes |
| Deprecations? | no |
| Tests pass? | yes |
| Documented? | yes |
| Fixed tickets | #296 |
| License | MIT |
mcg-web
changed the title
This bug was introduce because we was relying on class name to create FluentResolver id. We now relying on the service id as it is unique.
mcg-web
force-pushed
the
fluent-resolver-use-service-id
branch
from
29b1e94
to
7c887f8
Compare
barduck007
approved these changes
Mar 1, 2018
ooflorent
suggested changes
Mar 1, 2018
| @@ -15,8 +15,9 @@ Resolvers can be define 2 different ways | |||
| or `Overblog\GraphQLBundle\Definition\Resolver\MutationInterface`) | |||
| in `src/*Bundle/GraphQL` or `app/GraphQL` and they will be auto discovered. | |||
| Auto map classes method are accessible by: | |||
| * the class method name (example: `AppBunble\GraphQL\CustomResolver::myMethod`) | |||
| * the FQCN for callable classes (example: `AppBunble\GraphQL\InvokeResolver` for a resolver implementing the `__invoke` method) | |||
| * double colon (::) to separate service id (class name) and the method names | |||
There was a problem hiding this comment.
Use double-colon to match PHP doc
| * the class method name (example: `AppBunble\GraphQL\CustomResolver::myMethod`) | ||
| * the FQCN for callable classes (example: `AppBunble\GraphQL\InvokeResolver` for a resolver implementing the `__invoke` method) | ||
| * double colon (::) to separate service id (class name) and the method names | ||
| (example: `AppBunble\GraphQL\CustomResolver::myMethod` or `appbunble\graphql\customresolver::myMethod` for Symfony < 3.3) |
There was a problem hiding this comment.
You shouldn't talk about the lower case version
| @@ -133,7 +134,7 @@ Resolvers can be define 2 different ways | |||
| ``` | |||
|
|
|||
| **Note:** | |||
| * When using FQCN in yaml definition, backslash must be correctly quotes, | |||
| * When using service id as FQCN in yaml definition, backslash must be correctly quotes, | |||
There was a problem hiding this comment.
backslashes must be correctly escaped
| @@ -102,7 +102,7 @@ Types can be define 3 different ways: | |||
| **Note:** | |||
| * Types are lazy loaded so when using Symfony DI `autoconfigure` or this bundle auto mapping, the | |||
| only access to type is FQCN (or aliases if implements the aliases interface). | |||
| * When using FQCN in yaml definition, backslash must be correctly quotes, | |||
| * When using service id as FQCN in yaml definition, backslash must be correctly quotes, | |||
There was a problem hiding this comment.
backslashes must be correctly escaped
| @@ -22,7 +23,7 @@ public function __construct(ResolverResolver $resolver) | |||
| ], | |||
| 'resolve' => function ($root, $args) use ($resolver) { | |||
| return $resolver->resolve([ | |||
| EchoResolver::class, | |||
| version_compare(Kernel::VERSION, '3.3.0') < 0 ? strtolower(EchoResolver::class) : EchoResolver::class, | |||
UPGRADE-0.11.md
Outdated
| ### Change fluent resolvers id | ||
|
|
||
| The use of class name as prefix of fluent resolver id remove the possibility to use same class as 2 different services. | ||
| see this [issue for more detail](https://github.com/overblog/GraphQLBundle/issues/296) |
There was a problem hiding this comment.
See issue #296 for more detail
UPGRADE-0.11.md
Outdated
|
|
||
| The use of class name as prefix of fluent resolver id remove the possibility to use same class as 2 different services. | ||
| see this [issue for more detail](https://github.com/overblog/GraphQLBundle/issues/296) | ||
| That the reason from 0.11 we using service id as prefix (like in Symfony 4.1)... |
There was a problem hiding this comment.
That's the reason why starting v0.11 we are using …
mcg-web
force-pushed
the
fluent-resolver-use-service-id
branch
from
f1a81b7
to
04e48ae
Compare
ooflorent
approved these changes
Mar 5, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.