WIP v3 compatibility

[hummer2k]

See issues: #188 #192

[RWOverdijk]

Oh woa.

I'll have to take a look at this when I get back from my trip, which is 9 days. I'll review the crap out of this after, pinky promise (unless @Ocramius wants to chip in).

[pimjansen]

@RWOverdijk so confirmed it will be ZF3 compatible? Since its tagged as a ZF2 module in the description :)

[RWOverdijk]

@pimjansen Yep, but we'll check the compatibility, and maybe go for separate tags for both versions.

[stijnhau]

Isn't is possible to keep the BC compability and require any versions older than 2.x of components (for example factories have BC as you did it with zf2

[Ocramius]

Yes, but it's not really worth it.

[stijnhau]

@Ocramius I disagree about that.(depending on the time it takes) Because every module has to make changes to upgrade to the newer version of the framework. So if only 1 dependecy doesn't do the upgrade (or is still in progress) then you can't update the rest of you modules because they are all going to require zf3.

I agree with the part that this should be for a hort period only (2 months maybe?)

[Ocramius]

I disagree about that.(depending on the time it takes)

You may disagree, but maintaining compat with multiple dependency versions is a mess, from a maintainer's perspective. Been there, done it, never again (for me, at least)

[RWOverdijk]

The zf2 version will probably just be maintenance (fixes and such). We can't just drop it. I agree it's not worth maintain features etc for both, so that's off the table.

[pimjansen]

Yeah agree that it is ebst to just go on with zf3 and keep a seperate release branch going for f2 for bug/compatibility fixes. Even though i dont think much will change on zf2 as well structure wise besides the current migration changes to zf3.

[weierophinney]

I'll weigh in, if I may.

zfcampus/zf-apigility currently depends on this module. We're trying to provide an upgrade path for existing users, so they can gradually upgrade to v3 components, and have done so gracefully with all other components so far. However, if this module requires v3 components, we lose that ability entirely, and have essentially wasted a week of effort trying to provide an upgrade path.

I'd argue for a two-tiered strategy:

• a new minor release of assetmanager that supports both v2 and v3 components, followed by
• another minor release that removes support for v2 components.

This would give us an approach for updating zf-apigility, while removing the maintenance overhead for your own project.

Thoughts?

[RWOverdijk]

@weierophinney That sounds fair. I must admit I forgot about that, so thanks for chiming in.

[RWOverdijk]

This branch seems to now have conflicts. @hummer2k https://github.com/hummer2k/AssetManager/pull/1

[hummer2k]

@RWOverdijk: rebased I will address the 2 skipped tests this evening.

@weierophinney How to install zend-mvc-console with v2 components?

[hummer2k]

@RWOverdijk I think this PR is ready for review.

[RWOverdijk]

@hummer2k Thank you, I'm really curious! I'll take a look later today. :)

[RWOverdijk]

This looks pretty good to me. I'll have to test if it still works.

@Ocramius I know you're busy, but seeing the size of this PR, I'd like some input. @weierophinney Perhaps you could provide some, too?

[wshafer]

I'm pretty sure we can drop all the FactoryInterfaces in addition we can also pull out the createService() functions. The __invoke is already callable in ZF2.

In addition the signatures for the __invokes should just include the container since the other options aren't being used.

public function __invoke(ContainerInterface $container) {}

[weierophinney]

I'm pretty sure we can drop all the FactoryInterfaces in addition we can also pull out the createService() functions.

Yes, possible, but be careful with any that are invoked by a plugin manager (e.g., ControllerManager), as the behavior is different between v2 and v3.

In v2, the factory is passed the plugin manager, so if you need to grab any application level dependencies, you need to call $pluginManager->getServiceLocator() to get the parent locator. In v3, the parent locator is passed instead. For this reason, in the ZF components and in the Apigility components, we've tended to implement Zend\ServiceManager\FactoryInterface, and implement each of __invoke() and createService(); the latter typically becomes:

public function createService(ServiceLocatorInterface $container)
{
    if ($container instanceof AbstractPluginManager) {
        $container = $container->getServiceLocator() ?: $container;
    }
    return $this($container, NameOfPlugin::class);
}

This has another benefit: when you're ready to drop support for the v2 components, you will only need to update the FactoryInterface import and remove that method (vs. editing to remove one or more conditionals in your __invoke() method, which is more error prone).

For application-level service factories, however, yes: a plain invokable class is the easiest way to ensure compatibility.

[hummer2k]

Thanks for the input. I followed the official migration guide of zend-servicemanager (http://zendframework.github.io/zend-servicemanager/migration/#factories). I could refactor the factories but would rather do this in a separate PR removing support for v2.

[wshafer]

That's good to know @weierophinney. And as a consumer of Apigility, thanks for not letting shot myself in foot. ;)

[RWOverdijk]

Because of my current schedule I'm a bit quiet here. Could you ping me when it's ready so I can do my final review? :)

[hummer2k]

ping @RWOverdijk :)

[RWOverdijk]

This looks good to me. I'll merge / tag (minor bump) Monday around lunch time unless someone stops me by noticing something I missed. I'm only being careful because this module is being used in production :)

Thanks for all your efforts! Much appreciated.

[RWOverdijk]

Just to verify, this can be tagged as a minor? It works in my example project, and the tests pass.

[hummer2k]

Yes, can be tagged as minor. Thanks for merging!

[RWOverdijk]

The tests seem to be failing.

Sent from my iPhone

On 15 Aug 2016, at 11:43, Corny notifications@github.com wrote:

Yes, can be tagged as minor. Thanks for merging!

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

[hummer2k]

Can you restart the build on travis? Looks like some file system issues

[RWOverdijk]

That looks better :) I'll tag momentarily (after lunch)

[RWOverdijk]

Done