API-Skeletons / zf-doctrine-data-fixture

Doctrine Data Fixture Console Route with Service Manager configured Fixtures
BSD 2-Clause "Simplified" License
9 stars 4 forks source link

any zf3 version yet ? #4

Open Perfect-Web opened 7 years ago

TomHAnderson commented 7 years ago

I need the ServiceManager instances to become containers: https://github.com/API-Skeletons/zf-doctrine-data-fixture/blob/master/src/DataFixtureManagerFactory.php#L6

With that in place I'll be fine with updating the version. Upgrading this library isn't high on my priority list right now and a PR would be greatly appreciated.

TomHAnderson commented 6 years ago

I think the 5.0.x branch is ZF3 compliant. Will you test it?

internalsystemerror commented 6 years ago

I think what's needed is that DataFixtureManager should extend AbstractPluginManager instead of ServiceManager and that this will introduce a BC break since the factories used will then no longer require the use of $container->getServiceLocator() to access the main service manager.

As I look into it, it seems that this could greatly simplify and reduce the code quite significantly.

If you're interested, I could work up a PR?

TomHAnderson commented 6 years ago

Could we still set a group config? https://github.com/API-Skeletons/zf-doctrine-data-fixture/blob/master/src/DataFixtureManagerFactory.php#L45

What I think you're suggesting is this approach using a service listener: https://github.com/API-Skeletons/zf-doctrine-criteria/blob/master/src/Module.php#L27

I agree the current implementation is, um, janky. If you can resolve the data fixture manager into an AbstractPluginManager while maintaining the ability to switch whole service manager configs then I'd be very interested in a PR.

internalsystemerror commented 6 years ago

I've just seen ur latest comment after I posted the PR, I wasn't aware of the zf-doctrine-criteria repo, does this break BC there? The only thing I ended up removing was the reliance on getServiceLocator(), by using the AbstractPluginManager the factories will have the full container passed directly to them.

As for being able to change whole service manager configs, I'm not sure how that's presently possible with the current state of DataFixtureManager. If you could add a test that demonstrates it, I could ensure that this change allows the test to pass.

TomHAnderson commented 6 years ago

zf-doctrine-criteria Module.php was only used as an example of a ServiceListener configured service manager. The two projects have no relation.

Here https://github.com/API-Skeletons/zf-doctrine-data-fixture/blob/master/src/DataFixtureManagerFactory.php#L26 the $options are used to create the DataFixtureManager and that's what I need to ensure continues to work.

TomHAnderson commented 6 years ago

Your PR works just fine w/ the unit tests and I think it's a fine change. I'll take this time to phpstan the project; give it a face lift if you will.

Thanks for the contribution!

internalsystemerror commented 6 years ago

Nice, looking forward to it. On an aside, but related(ish), with the deprecation of zend-mvc-console, would you be interested in moving to symfony/console?

TomHAnderson commented 6 years ago

I'm fine with moving to symfony/console. Just don't expect me to do it as I expect it'll have a learning curve I haven't attempted yet.

internalsystemerror commented 6 years ago

I can do that then, it will be easier for you to learn from my changes.

TomHAnderson commented 6 years ago

Merged in those changes; please use a new branch for console work.

internalsystemerror commented 6 years ago

Ok, this is trickier than I thought since DoctrineModule does not itself contain the cli configuration to register the commands, this is delegated to DoctrineORMModule and DoctrineODMModule. Adding another binary to run instead of doctrine-module would be annoying and in that case it may be better to stay with zend-mvc-console since the integration is easier.

I'll have a think about this and see what I come up with.

internalsystemerror commented 6 years ago

Although I've just seen: https://github.com/doctrine/DoctrineModule/pull/462/files

internalsystemerror commented 6 years ago

I think this issue can be closed now, all support for zf3 is now in place.