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

[DependencyInjection] Bind with type is broken if no typehint used for constructor parameter #34223

Open
wants to merge 5 commits into
base: 4.3
from

Conversation

@gudfar
Copy link

gudfar commented Nov 2, 2019

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

…r constructor parameter
@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Nov 4, 2019
Copy link
Member

nicolas-grekas left a 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.

@gudfar

This comment has been minimized.

Copy link
Author

gudfar commented Nov 5, 2019

@nicolas-grekas

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.

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
WDYT?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 5, 2019

Let's give it a try :)

gudfar added 2 commits Nov 6, 2019
Copy link
Contributor

derrabus left a comment

We should add a test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.