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

dependentfixtureinterface return type #337

Closed
mmarton opened this issue Jan 14, 2020 · 7 comments
Closed

dependentfixtureinterface return type #337

mmarton opened this issue Jan 14, 2020 · 7 comments

Comments

@mmarton
Copy link

mmarton commented Jan 14, 2020

Hi!

this commit added 'class-string[]' as return type to getDependencies: e12d512

I already had return type hint in my fixture classes like:
public function getDependencies(): iterable

And now phpstan complains because:

Return type (iterable) of method ...\AdminMenuFixtures::getDependencies() should be covariant with return type (Doctrine\Common\DataFixtures\class)

It may be not a BC break, it "just" broke the deployment and not the working of the code.

Do you have any suggestion? Adding fixtrues to ignore list or removeing the return typehints seems walking backwards.

regards

@greg0ire
Copy link
Member

greg0ire commented Jan 14, 2020

@ruudk can you please have a look? @mmarton maybe it will work with the latest version of phpstan and that's not what you are using?

@mmarton
Copy link
Author

mmarton commented Jan 14, 2020

@greg0ire I've updated to the latest version (0.12.5)

The message changed a bit, but the error was still there:

37 Return type (iterable) of method App\DataFixtures\AdminMenuFixtures::getDependencies() should be covariant with return type (array) of method Doctrine\Common\DataFixtures\DependentFixtureInterface::getDependencies()

changing the typehint from iterable to array solved the problem form me
(but it takes away the ability to use any other iterable structure)

@greg0ire
Copy link
Member

I think iterable could have been used… can you edit the vendor file directly and tell us the results?

@ruudk
Copy link
Contributor

ruudk commented Jan 14, 2020

I changed the previous PHPDoc from array to array of type class-string. Since it was already an array, and never an iterable, I should not have broken anything.

We could change it to Iterable<class-string> for people that want to use anything else than array, but this was never the design anyway. The fact that this worked is because there was no strict return type. If we change it to Iterable<class-string> it will probably break more phpstan configurations.

@greg0ire
Copy link
Member

Oh right, so it was already not covariant anyway. @mmarton if you keep the PHPStan upgrade and downgrade data-fixtures, you should get a similar error about covariance I think… it's just that PHPStan got better at this, isn't it?

@alcaeus
Copy link
Member

alcaeus commented Jan 16, 2020

The previous return type (array) is a subtype of iterable, so adding that return type is a violation of covariance which is required for return types: https://3v4l.org/B7gTS (ignore PHP < 7.4 as it can't handle covariance either). In this case, there's nothing for us to fix, as this comment also highlights.

@alcaeus alcaeus closed this as completed Jan 16, 2020
@mmarton
Copy link
Author

mmarton commented Jan 17, 2020

Oh right, so it was already not covariant anyway. @mmarton if you keep the PHPStan upgrade and downgrade data-fixtures, you should get a similar error about covariance I think… it's just that PHPStan got better at this, isn't it?

Yes, itt seems like the issue was on my side.
thanks

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

No branches or pull requests

4 participants