search

doctrine / data-fixtures

Doctrine2 ORM Data Fixtures Extensions
http://www.doctrine-project.org
MIT License
2.77k stars 224 forks source link

dependentfixtureinterface return type #337

Closed mmarton closed 4 years ago

mmarton commented 4 years ago

Hi!

this commit added 'class-string[]' as return type to getDependencies: https://github.com/doctrine/data-fixtures/commit/e12d5123c3eed6c79027a0eef3e7366a5b9efead

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 commented 4 years ago

@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 commented 4 years ago

@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 commented 4 years ago

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

ruudk commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

mmarton commented 4 years ago

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