search

doctrine / DoctrineFixturesBundle

Symfony integration for the doctrine/data-fixtures library
MIT License
2.44k stars 201 forks source link

Automatically extract fixture dependencies #421

Closed cs278 closed 2 months ago

cs278 commented 5 months ago

Fixes https://github.com/doctrine/DoctrineFixturesBundle/issues/371

This improves the cooperation between the native fixture dependencies and the bundle provided groups. It is now no longer necessary to add the dependencies of fixtures in a group into the group as well.

I've not tested this extensively, or added tests for this yet but if the maintainers are happy with the change in behaviour I can do that.

cs278 commented 5 months ago

I'd appreciate some feedback on the behaviour change before I do any more work making this suitable to be merged.

greg0ire commented 3 months ago

I don't know much about this bundle, but I don't see a problem with this, please go ahead.

cs278 commented 3 months ago

I don't know much about this bundle, but I don't see a problem with this, please go ahead.

@greg0ire :+1: updated

greg0ire commented 3 months ago

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

cs278 commented 3 months ago

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

PHPStan is failing but it seems to be it doesn't understand what the parent class is so it thinks the methods are undefined, it's doing the same on the 3.5.x branch so nothing I've done is causing it.

greg0ire commented 3 months ago

Are you sure? Here is a 12 hours old build not exhibiting any issue: https://github.com/doctrine/DoctrineFixturesBundle/actions/runs/8513966759

greg0ire commented 3 months ago

Here is another PR I just made: https://github.com/doctrine/DoctrineFixturesBundle/pull/429 It's green.

cs278 commented 3 months ago

Apologies, I merged from the wrong remote which meant I didn't have b83a046 I've ignored the PHPStan error I've introduced which is the same class as the existing ignored problems.

greg0ire commented 3 months ago

Retargeting on 3.6.x since this is no bugfix. Please rebase and force push.

cs278 commented 3 months ago

@greg0ire updated

greg0ire commented 3 months ago

I don't think it makes sense to have 2 commits here: we're never going to want to revert one and keep the other, right? If so, please squash them, preserving the excellent commit message body.

greg0ire commented 2 months ago

Thanks @cs278 !

cs278 commented 2 months ago

Thanks for your assistance in getting this merged @greg0ire :smiley: