First implementation of symfony fixture loader + group support #461

[arendjantetteroo]

A first implementation, basically copy/paste from the fixturesbundle.

Couple questions:

• Do we want to keep depending on the fixturesBundle or should we copy any files over so we can avoid the fixturesBundle which also installs the orm stuff which if you only use odm you don't need? (for example the FixtureGroupInterface)
• i based it on 3.5.3 as that is what i have locally, it would be nice if this could be included in 3.6, but not sure we can keep it backward compatible? Maybe if we keep fixtures/bundle options and keep their old implementation and if you provide an --autowire option it uses the new one?

Thoughts?

Fixes #461

[alcaeus]

Sorry for the close/reopen spam - travis-ci needed another nudge to check this PR.

Do we want to keep depending on the fixturesBundle or should we copy any files over so we can avoid the fixturesBundle which also installs the orm stuff which if you only use odm you don't need? (for example the FixtureGroupInterface)

We don't depend on the fixtures bundle, only on the data-fixtures library, which doesn't have a hard requirement on ORM. Since we don't want to introduce one, copying the necessary files from the fixtures bundle is the way to go for now. I'm afraid I won't be able to implement #509 since it most likely requires changes to the fixtures bundle as well.

i based it on 3.5.3 as that is what i have locally, it would be nice if this could be included in 3.6, but not sure we can keep it backward compatible? Maybe if we keep fixtures/bundle options and keep their old implementation and if you provide an --autowire option it uses the new one?

If there's going to be a 3.6, it will be in there. 3.6 will happen if I find things dropped in 4.0 that I haven't deprecated in 3.5 yet (I remember there were some). I'll give you an update once I've checked that.

I'll do a comprehensive code review shortly, just wanted to give you a quick update since you opened a pull request so quickly! 🎉

[alcaeus]

And a quick update right behind it: there will definitely be a 3.6 release which we'll get going shortly. Once that's done you can change the base on the PR to 3.6 and it will be there 👍

[arendjantetteroo]

Do we want to keep depending on the fixturesBundle or should we copy any files over so we can avoid the fixturesBundle which also installs the orm stuff which if you only use odm you don't need? (for example the FixtureGroupInterface)

We don't depend on the fixtures bundle, only on the data-fixtures library, which doesn't have a hard requirement on ORM. Since we don't want to introduce one, copying the necessary files from the fixtures bundle is the way to go for now. I'm afraid I won't be able to implement #509 since it most likely requires changes to the fixtures bundle as well.

I copied the Fixture class + the fixtureGroupinterface so it works without the FixturesBundle now.
So it only depends on the doctrine/data-fixtures library.

[arendjantetteroo]

I fixed the issue with the tests by adding a mocked loader, the functionality of the loader is not tested.
I saw that the FixturesBundle has an integration test for it, should we copy that as well?

[arendjantetteroo]

While fixing the command tests i figured out the current loader code is php 7.1 and up with type checking.

2 options:

1. Remove all type stuff so 5.6 is still valid (assuming 3.6 should still be 5.x compatible?)
2. Postpone to 4.0

[alcaeus]

I'm very inclined to postpone to 4.0 - the trouble doesn't seem to be worth it.

[arendjantetteroo]

@alcaeus
After a few more changes, i've reverted the old way in the command with --bundles and --fixtures.

If a user wants to use the autowired way, he can use --services.
The old and new options are incompatible, so if both --bundles/--fixtures and --services are used, an error is thrown indicating it.
if --services is provided, we assume the user knows that he needs to use the fixtures as services.
I guess this should be added to the 3.6 upgrade file?
In 4.0 you could then remove the --bundles and --fixtures options if you want to and make this only based on services and the --group feature to define which to load.

It's working on both my old application way of doing fixtures and working with the new autowired fixtures in a new branch.

Let me know what else is needed?

[alcaeus]

@arendjantetteroo I'll take a deeper look at the PR tonight. If we can keep this entirely BC I'm all for it.

[alcaeus]

Good job getting this done in a backwards compatible way!

As mentioned in the review, I'm not excited about duplicating some of the logic from the fixtures bundle, which is why I wanted to extract the functionality. However, since this most likely involves a larger change in both DoctrineFixturesBundle and data-fixtures itself, we may just roll with this for now.

Please note that this PR will remain sitting idle for a few more days as I first need to prepare a 3.6 branch and prepare the necessary deprecation/upgrade files. Once that's done, we can change the base of this PR to 3.6 and get it on its way! 🎉

[arendjantetteroo]

@alcaeus thanks for the review, updated and rebased.

Thinking about the ODM/ORM stuff, the only clean way would be extracting the loading of fixtures into a seperate symfony bundle that can work with the same Fixture/FixtureGroup interface and then both the FixtureBundle and this bundle/a new ODMFixture bundle depending on this FixtureLoaderBundle.

I just don't know if that is worth all the hassle though...

[arendjantetteroo]

@alcaeus what's the plan, anything i can do to help?

[alcaeus]

@alcaeus what's the plan, anything i can do to help?

No, I just need to finish looking through this and merge it. I'll try to get that done soon, even if it means copy/pasting large parts of the ORM fixtures bundle. We should clean that up afterwards.

[alcaeus]

@arendjantetteroo I've fixed a couple of minor review issues. From a code perspective this looks good, but I think we need to add some tests for this. In DoctrineFixturesBundle, there's an IntegrationTest that we could base our tests on. Do you want to give this a shot or would you rather have me do it? I won't have time for it before the weekend though...

[arendjantetteroo]

I'll see what i can do, not sure i get to it before the weekend either :)

[arendjantetteroo]

@alcaeus I copy/pasted and then changed the IntegrationTest, works for me locally, waiting for the travis checks now.
I added a seperate commit for the type removal so it supports 5.6 in 3.6 version of the bundle.
This commit can then be reverted for 4 to have types if wanted. If we squash it, this info is lost.

[arendjantetteroo]

Fails on pre 7.1 with :
1) Doctrine\Bundle\MongoDBBundle\Tests\IntegrationTest::testFixturesLoader Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: Unable to replace alias "doctrine.odm.mongodb.document_manager" with actual definition "doctrine_mongodb.odm.document_manager".

Any ideas?

[arendjantetteroo]

@alcaeus From what i understand i need to load a config into the Extension to get this document manager definition in the service container. However i don't understand why this works fine in 7.1 and 7.2?

[alcaeus]

I'm taking a look what's going on - no idea why it isn't working. We'll see 👍

[arendjantetteroo]

@alcaeus I got a message to force push but i think you already corrected it right?

[alcaeus]

@alcaeus I got a message to force push but i think you already corrected it right?

Yeah, I fixed it. Currently trying to fix the alias issue you mentioned above.

[alcaeus]

Looks good, I added:

• a temporary alias until we remove that pesky service alias that was causing failures
• the logic to handle fixtures with dependencies since we're dependent on an old version of doctrine/data-fixtures in 3.x.

With that, the logic looks good and can be merged into 3.6; all legacy code will then be dropped for 4.0. Thank you @arendjantetteroo for the heavy lifting on this!