doctrine / DoctrineFixturesBundle

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

Ability to load only one fixture class #246

Closed matthieumota closed 5 years ago

matthieumota commented 6 years ago

I think it can be useful to load only one fixture class. I make it simple and a fixture who is single cannot be a fixture ordered or cannot have dependencies. I've also fix somes typos.

emnsen commented 6 years ago

+1

TerjeBr commented 5 years ago

I think it can be valuable to have this in addition to #260 Another person (@hypnotract in issue #254) was also asking for this.

I can be a useful option if you want to run just one fixture, and you do not want to create a new fixture group for just that purpose.

TerjeBr commented 5 years ago

@alcaeus and @weaverryan could you please have look if this one could be merged too?

alcaeus commented 5 years ago

@TerjeBr before I'll consider this PR, this needs to be updated with more tests to ensure fixtures are loaded properly. For example, what happens when the fixture has dependencies on other fixtures? Tests to demonstrate and ensure correct behaviour are absolutely necessary here.

Besides that, this functionality could easily be built with the functionality in #260: either the user creates a one-off group themselves, or we automatically create a group for each fixture with the class name of the fixture as group name (similar to validation groups in the Validator component, where you have the Default and <className> groups assigned automatically). I'm not sure I'd want to add another CLI option for this.

matthieumota commented 5 years ago

@alcaeus So I'll fix this PR with more tests to ensure fixture doesn't have dependencies or other fixtures ? But like you say, we can use #260 for this feature, so maybe we can just close this PR. I'll try to fix that when I get time.

alcaeus commented 5 years ago

@matthieumota let's wait for #260 and maybe @weaverryan has an idea. I believe that just assigning a fixture-specific group MAY work but we still need to ensure dependencies are fulfilled (e.g. have a clear behavior when you load a single fixture that implements DependentFixtureInterface).

If we decide that we don't want to solve this using groups but rather have a separate option, this PR would need tests, but I think we can hold off on that for now.

TerjeBr commented 5 years ago

I disagree with this thing about checking dependencies and ordering.

Dependencies and ordered fixtures are meant to guide the fixture command when loading a bunch of fixtures. When a developer ask for a single fixture class to be loaded, the command tool should just load that fixture, no questions asked.

I hate it when a dev tool is rendered less useful by trying to baby sit the user and limit what can be done. Take a lesson from unix "rm" command, it just does what is asked and is a very useful tool.

What if the developer is trying to run all the fixtures one by one? What if all the fixtures this one depends on has already run successfully, but because of an error (that has just been fixed) the developer wants to run this dependent fixture again.

Do not try to second guess the developer. If he/she specifies on the command line a single fixture class to load, then assume that devolper knows what he/she is doing and just load that fixture.

alcaeus commented 5 years ago

Do not try to second guess the developer.

If you read my reply carefully, you'll notice that I'm asking for a well-defined behaviour including tests for it:

e.g. have a clear behaviour when you load a single fixture that implements

This doesn't mean that we'll require all other fixtures to be loaded as well. One option would be to throw an exception and not do anything for such fixtures - that would be sensible given that we drop existing data from the database and then realise that we can't load it. Alternatively, ask the user for confirmation in a more strongly-worded message. Last but not least, just try to load it and see where it takes us. All I want is for this behaviour to be properly defined and tested.

assume that devolper knows what he/she is doing and just load that fixture

My work history has taught me to not assume such things, especially not when we can save people from shooting themselves in the foot by just asking "are you sure you want to do this?"

matthieumota commented 5 years ago

@TerjeBr Yes like say @alcaeus for this PR, there only miss such test case. I make choice that class has not dependencies or order because it can cause non integrity in fixtures if developer use that. For use case you explain, developer have to not set order or dependencies and it's okay, I only check that by security.

TerjeBr commented 5 years ago

So, if I want to run a fixture that is marked as ordered or dependent, I am not able to, and the tool will just throw an exception.

Then I will have to ask for a new option to the command then, that just loads the fixture without this check?

alcaeus commented 5 years ago

Well, if you're fixture depends on another fixture being loaded and having shared its data, your dependent fixture is going to crash on its own anyways. If it isn't going to crash, it shouldn't be dependent, right?

TerjeBr commented 5 years ago

What I am trying to say, is that it can be many reasons to load a single fixture. Especially with the --append option a developer could possible try to load the fixtures one by one. That will not be possible if you add this baby sitting checks and try to second guess the developer.

TerjeBr commented 5 years ago

A fixture can be set as dependent, but may not crash. That may happen if the developer has been running the fixture it depends on just before.

That may not work if you have references and stuff like that, but if the dependency is only a dependency on the state of the database it might work. The tool should give develpers freedom to do what they want. You cannot forsee every use case.

weaverryan commented 5 years ago

You cannot forsee every use case.

That’s true. It’s a balance between supporting as many use cases as possible and making it so flexible that it’s easy to make mistakes. I see your argument about ignoring the dependencies. But there’s also a valid counter-argument - someone selects to run one fixture and can’t fiure out why the bundle isn’t smart enough to loads its dependencies.

So, I’m not sure what the best way is. I think the default behavior should be to respect the dependencies, if they’re specified. Probably throw an exception if you try to run a fixture, but not it’s dependencies. With perhaps a flag to avoid that.

I would also want this to use the groups stuff. So the question is just what to do about dependencies, and for those, a flag or two could probably satisfy almost everyone.

alcaeus commented 5 years ago

This was now integrated into #260, so closing this. Thank you for your work on this, @matthieumota!