Skip to content

[DependencyInjection] Ignore argument type check in CheckTypeDeclarationsPass if it's a Definition with a factory #44879

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

Merged
merged 1 commit into from
Jan 9, 2022

Conversation

fancyweb
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets #35599, #44515
License MIT
Doc PR -

When a definition uses a factory, we don't know what it returns.

…ionsPass if it's a Definition with a factory
@fancyweb fancyweb force-pushed the di/ignore-factory-arg-lint branch from 45f1e38 to b9095e6 Compare December 31, 2021 15:45
@chalasr
Copy link
Member

chalasr commented Dec 31, 2021

What about using the factory return type when it has one?

@fancyweb
Copy link
Contributor Author

Sure, but that would be a feature isn'it? Because right now, it relies only on the definition class and either it's good or not. Fall backing on the return type is a new behavior.

@nicolas-grekas
Copy link
Member

The class is supposed to be used for that purpose. Can't we fix the class of the definition instead?

@fancyweb
Copy link
Contributor Author

fancyweb commented Dec 31, 2021 via email

@chalasr
Copy link
Member

chalasr commented Jan 4, 2022

AbstractAdapter::createConnection from the Cache component which can return
different values depending of the passed dsn.

These various connections instances could probably be wrapped in a core domain object.

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 as-is for 4.4

@fabpot
Copy link
Member

fabpot commented Jan 9, 2022

Thank you @fancyweb.

@fabpot fabpot merged commit c3674b4 into symfony:4.4 Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants