[DI] Suggest typed argument when binding fails with untyped argument #34223
Conversation
There was a problem hiding this comment.
Here is a code review, but before fixing it: this will break configurations that target different arguments when the type is here or not. To them, the exception will be a false positive.
A safer approach might be to improve the message where the current exception is thrown. Because we might have less context there, the message might be more generic - but that's the point of not triggering false positives and still giving some hint.
src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/DependencyInjection/Compiler/ResolveBindingsPass.php
Outdated
Show resolved
Hide resolved
I guess it shouldn't break, because exception will trigger only if we have different argument type in binding and in constructor. But I also don’t like throwing an exception in this place I can suggest to add error message. It will look like this http://joxi.net/YmEyjBYtw5O7Bm |
|
Let's give it a try :) |
3daf1ed to
5cc69c8
Compare
5cc69c8 to
0e92399
Compare
There was a problem hiding this comment.
I pushed an alternative implementation. I also removed the case where both a binding and an argument have a type but they don't match: I'm not sure we should suggest these - it's not the job of bindings to decide for the API of a class.
@OskarStark please confirm that this fixes your concern.
|
Thank you Nicolas I am able to test this in January but from the code pov it looks good👍🏻 |
|
friendly ping @OskarStark |
|
Thank you @gudfar. |
…d argument (gudfar) This PR was merged into the 4.3 branch. Discussion ---------- [DI] Suggest typed argument when binding fails with untyped argument | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | #33470 | License | MIT I've added a condition that looks for arguments and if the typehint doesn’t match, throws an `InvalidArgumentException` Commits ------- 0e92399 [DI] Suggest typed argument when binding fails with untyped argument
I've added a condition that looks for arguments and if the typehint doesn’t match, throws an
InvalidArgumentException