Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Do not suggest writing an implementation when multiple exist#26595
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
javiereguiluz commentedMar 19, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@chalasr much better! If I may, I'd like to propose another tweak to your latest proposal: |
| }else { | ||
| $alternatives =$this->createTypeAlternatives($reference); | ||
| $message =$this->container->has($type) ?'this service is abstract' :'no such service exists'; | ||
| $message =sprintf('references %s "%s" but %s.%s',$r->isInterface() ?'interface' :'class',$type,$message,$this->createTypeAlternatives($reference)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$alternatives to be used on this line, no need to call createTypeAlternatives twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
fixed
| } | ||
| $this->assertNotNull($e); | ||
| $this->assertSame('Cannot autowire service "setter_injection_collision": argument "$collision" of method "Symfony\Component\DependencyInjection\Tests\Compiler\SetterInjectionCollision::setMultipleInstancesForOneArg()" references interface "Symfony\Component\DependencyInjection\Tests\Compiler\CollisionInterface" but no such service exists. You should maybe alias this interface to one of these existing services: "c1", "c2".',$e->getMessage()); |
nicolas-grekasMar 19, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What's the difference with the previous way this test was written?
This one doesn't tell aboutDid you create...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@expectedExceptionMessage usesassertContains IIRC
chalasr commentedMar 29, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@javiereguiluz Not that I dislike your suggestion, but it applies for interfaces only whereas this code deals with classes as well and I'm just not in the mood for adding more conditions into it. This PR removes a bad suggestion, please feel free to improve the correct one in another :) Ready on my side. |
nicolas-grekas commentedMar 29, 2018
Thank you@chalasr. |
…e exist (chalasr)This PR was merged into the 3.4 branch.Discussion----------[DI] Do not suggest writing an implementation when multiple exist| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aBefore:> Cannot autowire service "App\Decorator2": argument "$inner" of method "__construct()" references interface "App\SomeInterface" but no such service exists. You should maybe alias this interface to one of these existing services: "App\Decorator1", "App\Decorator2", "App\Original". Did you create a class that implements this interface?After:> Cannot autowire service "App\Decorator2": argument "$inner" of method "__construct()" references interface "App\SomeInterface" but no such service exists. You should maybe alias this interface to one of these existing services: "App\Decorator1", "App\Decorator2", "App\Original".Commits-------1ffdb50 [DI] Do not suggest writing an implementation when multiple exist
…bbuh)This PR was merged into the 4.1-dev branch.Discussion----------[DependencyInjection] fix expected exception message| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets || License | MIT| Doc PR |Updates a test that was introduced in#25631 to match the changes made in#26595.Commits-------042eb4f fix expected exception message
Before:
After: