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

Automatically extract fixture dependencies #421

Merged
merged 1 commit into from May 2, 2024

Conversation

cs278
Copy link
Contributor

@cs278 cs278 commented Jan 16, 2024

Fixes #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 cs278 marked this pull request as ready for review February 8, 2024 17:36
@cs278
Copy link
Contributor Author

cs278 commented Feb 8, 2024

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

@greg0ire
Copy link
Member

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

@cs278 cs278 force-pushed the load-dependencies branch 3 times, most recently from eeb35dc to 73c2923 Compare March 28, 2024 17:54
@cs278
Copy link
Contributor Author

cs278 commented Mar 28, 2024

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

@greg0ire 👍 updated

@greg0ire
Copy link
Member

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

@cs278
Copy link
Contributor Author

cs278 commented Apr 2, 2024

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
Copy link
Member

greg0ire commented Apr 2, 2024

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

@greg0ire
Copy link
Member

greg0ire commented Apr 2, 2024

Here is another PR I just made: #429
It's green.

@cs278
Copy link
Contributor Author

cs278 commented Apr 2, 2024

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 greg0ire changed the base branch from 3.5.x to 3.6.x April 2, 2024 20:20
@greg0ire
Copy link
Member

greg0ire commented Apr 2, 2024

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

@cs278
Copy link
Contributor Author

cs278 commented Apr 4, 2024

@greg0ire updated

@cs278 cs278 requested a review from greg0ire April 4, 2024 13:49
@greg0ire
Copy link
Member

greg0ire commented Apr 4, 2024

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.

It is now no longer required to declare groups on all dependent
fixtures, the fixture loader will now automatically find the required
dependencies and load them without them being explicitly added
to the groups being loaded.

Before you were forced to declare groups all the way down the
dependency chain, for example:

```
TopLevelFixture (groups: default)
├─ SecondLevelFixture1 (groups: default, alternative)
│  ├─ ThirdLevelFixture1 (groups: default, alternative)
├─ SecondLevelFixture2 (groups: default)
│  ├─ ThridLevelFixture2 (groups: default)
│  ├─ ThirdLevelFixture3 (groups: default, alternative)
```

Now you only need to specify the groups at the highest level:

```
TopLevelFixture (groups: default)
├─ SecondLevelFixture1 (groups: alternative)
│  ├─ ThirdLevelFixture1 (groups: none)
├─ SecondLevelFixture2 (groups: none)
│  ├─ ThridLevelFixture2 (groups: none)
│  ├─ ThirdLevelFixture3 (groups: alternative)
```

In both examples the groups `default` and `alternative` will load the
same fixtures.

Adds an additional PHPStan error to the baseline which appears to be
caused by the backwards compatibility layer.
@greg0ire greg0ire added this to the 3.6.0 milestone May 2, 2024
@greg0ire greg0ire merged commit b7f5ed4 into doctrine:3.6.x May 2, 2024
13 checks passed
@greg0ire
Copy link
Member

greg0ire commented May 2, 2024

Thanks @cs278 !

@cs278 cs278 deleted the load-dependencies branch May 2, 2024 14:29
@cs278
Copy link
Contributor Author

cs278 commented May 2, 2024

Thanks for your assistance in getting this merged @greg0ire 😃

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.

Possibility to add fixtures into dependent group
3 participants