Skip to content

[2.4] Allow autowiring of class extending SearchFilter #2643

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
Mar 25, 2019

Conversation

antograssiot
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Remove a BC break introduced in #2630

@soyuka
Copy link
Member

soyuka commented Mar 25, 2019

#2640 related? @meyerbaptiste can we merge this?

@meyerbaptiste
Copy link
Member

meyerbaptiste commented Mar 25, 2019

No, it's not related. Did my change break the autowiring @antograssiot? Because apart from that it's not a BC break. 🤔

@soyuka
Copy link
Member

soyuka commented Mar 25, 2019

I'm also asking because it has been changed in the cs fixes you did in: 97fa954#diff-77ec54980605d6062780fcd93ba31c64 :p

@antograssiot
Copy link
Contributor Author

@soyuka No #2640 is not related and on master only.
@meyerbaptiste yes it does break autowiring because the argument ha sno typehint (and it's normal as it is deprecated) but there's no default value anymore. Anyway it is a BC break IMO, it is not compliant with Symfony backward compatibility promise to remove default value of an argument in a public method

@meyerbaptiste
Copy link
Member

meyerbaptiste commented Mar 25, 2019

it is not compliant with Symfony backward compatibility promise to remove default value of an argument in a public method

But a default value here is a nonsense because the following argument is not optional (IriConverterInterface $iriConverter)...

@antograssiot
Copy link
Contributor Author

I know/understand that it is a "non sense" to have this optional value before mandatory arguments but we can't change the argument order either. I don't have a better proposal

Cannot autowire service "Foo": argument "$requestStack" of method "ApiPlatform\Core\Bridge\Doctrine\Orm\Filter\SearchFilter::__construct()" has no type-hint, you should configure its value explicitly.

@meyerbaptiste
Copy link
Member

Ahah, yes I agree with that and your patch regarding the autowiring. Maybe the DependencyInjection component should inject null in this case, but that's another debate and it's not ours. 😄

@meyerbaptiste meyerbaptiste merged commit 61c24bf into api-platform:2.4 Mar 25, 2019
@meyerbaptiste
Copy link
Member

My bad, thx @antograssiot!

@antograssiot antograssiot deleted the bc-searchfilter branch March 25, 2019 13:32
@teohhanhui
Copy link
Contributor

Now it feels like we're writing code for the autowiring lol!

@teohhanhui
Copy link
Contributor

teohhanhui commented Mar 25, 2019

Do we have a way (_bind or something) to let the autowiring know what to inject, without doing this weird hack in our code?

Anyway, I'd strongly argue that breaking autowiring is NOT a BC break. If it's about @ApiFilter annotation, I imagine we could do something about that in our compiler pass...

meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 27, 2019
meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 27, 2019
meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 27, 2019
meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 27, 2019
meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 29, 2019
meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 29, 2019
meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 29, 2019
meyerbaptiste added a commit to meyerbaptiste/core that referenced this pull request Mar 29, 2019
teohhanhui added a commit that referenced this pull request Mar 29, 2019
Alternative fix to #2643: allow autowiring of filter classes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants