Added FQCN aliases to ServiceListenerFactory so some could use e.g. ReflectionBasedAbstractFactories

[boesing]

The ServiceListenerFactory is actually missing some aliases.
I've added the missing ones and fixed the old unit tests which provided non-matching configuration to the unit test either.

For ZF4 we should consider switching the FQCN to the factories and moving the old strings to the aliases configuration.

[thexpand]

@boesing
I've started a PR for the latter a month ago #292

However, I'm not sure if it would ever be merged, because @Xerkus has another PR for zend-mvc v4, which already does that, by defining a ConfigProvider.
Link to the PR: #259
Link to the config provider: https://github.com/zendframework/zend-mvc/blob/bf93aa5e64e688a4f2bc7a50679c152bac289cae/src/ConfigProvider.php

[boesing]

@thexpand Oh, didn't noticed. However, your changes supposed to be BC Break since you are moving the factories to alias and vice versa.
I would prefer having a working logic for ZF3 and I'm not sure if there is already any ZF4 ETA at all?

[thexpand]

There is no ZF3, actually. There won't be ZF4. Zend Framework is no longer a monolithic framework, but rather a component-based one, as it is now divided into separate repositories, each with its own versioning (i.e. zend-mvc could be 3.1.1, but zend-form may be on 5.0.0).

So I guess you are talking about v4 of zend-mvc. You can track down @Xerkus 's work on the PR, as well as this thread on the forums: https://discourse.zendframework.com/t/rfc-zend-mvc-4-design-changes/447
Honestly, I don't know what the status of zend-mvc v4 is, but once it is ready, I don't even think we would need the aliases anymore. The release will introduce many BC Breaks, so I don't see a problem removing the aliases at all.

The only benefit from having class constants over service strings is to let developers prepare for the upcoming zend-mvc v4 - in other words to provide a transitioning step between the current version and the new major version. I would like to see what the others think, too, so that we can decide which PR to use (if we are going to do it in the first place).

PS: @boesing you have wasted some time doing the same I did in my PR.

/cc @Xerkus @froschdesign @xtreamwayz @weierophinney

[froschdesign]

@boesing

However, your changes supposed to be BC Break since you are moving the factories to alias and vice versa.

Can you explain where do you see a BC break? Thanks!

[boesing]

@froschdesign
I've created a unit test to show what I mean:

Actually, we have this configuration:

    public function testDelegatorIsBeingCalled()
    {
        $controllers = $this->prophesize(ControllerManager::class)->reveal();

        $services = new ServiceManager();
        $factory = function () use ($controllers) {
            return $controllers;
        };

        $services->setFactory('ControllerManager', $factory);

        $delegator = $this->createMock(DelegatorFactoryInterface::class);
        $delegator->expects($this->once())->method('__invoke')->willReturn($controllers);
        $services->addDelegator('ControllerManager', $delegator);

        $this->assertSame($controllers, $services->get('ControllerManager'));
    }

If you change this to the following (which does that PR we are talking about [#292]), it will look like this:

    public function testDelegatorIsBeingCalled()
    {
        $controllers = $this->prophesize(ControllerManager::class)->reveal();

        $services = new ServiceManager();
        $factory = function () use ($controllers) {
            return $controllers;
        };

        $services->setFactory(ControllerManager::class, $factory);
        $services->setAlias('ControllerManager', ControllerManager::class);

        $delegator = $this->createMock(DelegatorFactoryInterface::class);
        $delegator->expects($this->once())->method('__invoke')->willReturn($controllers);
        $services->addDelegator('ControllerManager', $delegator);

        $this->assertSame($controllers, $services->get('ControllerManager'));
    }

Since there is no delegator registered for ControllerManager::class (FQCN), the delegator is never called.

So, in fact, every project which implemented own delegators for the ControllerManager string service name, will break. Or am I missing something?

But if this PR (#294) will get merged, anything supposed to be fine until zend-mvc v4 (where we can swap aliases to make anything correct).

    public function testDelegatorIsBeingCalled()
    {
        $controllers = $this->prophesize(ControllerManager::class)->reveal();

        $services = new ServiceManager();
        $factory = function () use ($controllers) {
            return $controllers;
        };

        $services->setFactory('ControllerManager', $factory);
        $services->setAlias(ControllerManager::class, 'ControllerManager');

        $delegator = $this->createMock(DelegatorFactoryInterface::class);
        $delegator->expects($this->once())->method('__invoke')->willReturn($controllers);
        $services->addDelegator('ControllerManager', $delegator);

        $this->assertSame($controllers, $services->get(ControllerManager::class));
    }

[thexpand]

@boesing I think that the main problem is that ServiceManager::addDelegator() does not resolve the name, that was passed to it. I have manually added the following line to the method and ran your test:
$name = isset($this->resolvedAliases[$name]) ? $this->resolvedAliases[$name] : $name;

It worked okay, but I'm not sure that I am right. Anyone?

EDIT: Extending a little bit more.
Here the delegator is added, without resolving the name.
https://github.com/zendframework/zend-servicemanager/blob/master/src/ServiceManager.php#L472-L475

However, when creating a service, the already resolved name is used to access its delegator.
https://github.com/zendframework/zend-servicemanager/blob/master/src/ServiceManager.php#L761-L767

[thexpand]

@froschdesign
I'm summarizing our short discussion with @boesing on Slack.

Delegators are added to the service manager by the actual name of the service and not by their aliases. Moving the actual service names in the aliases array will be a BC Break, as previously registered delegators to that service won't be called at all.

What we can do is already implemented with this PR. The missing FQCN class constants are added as aliases to the actual service names, which we don't change to keep backwards compatibility.

As we discusses, @boesing remembered doing a mistake more than an year ago with zend-validator, where he moved the ValidatorManager actual service name to the aliases and introduced a new name for the service (FQCN). You can take a look at the commit zendframework/zend-validator@d8f6e43

What I suggest is that we close my PR (#292) and leave this one, as it does not introduce a BC Break.

As for the failing Travis build - I've created a hotfix on the master branch (#295). When accepted and merged to the master branch, it can be merged to this PR, too, so that the tests may pass.

[froschdesign]

@boesing
Thanks for the explanation and the included unit tests, they help more than 1000 words. 👍

@thexpand
Notice: If you open an issue report or a pull request, you can also close it yourself.

[Xerkus]

On hold for now as it potentially conflicts with my work in progress for next major.

[weierophinney]

This repository has been closed and moved to laminas/laminas-mvc; a new issue has been opened at laminas/laminas-mvc#10.

[weierophinney]

This repository has been moved to laminas/laminas-mvc. If you feel that this patch is still relevant, please re-open against that repository, and reference this issue. To re-open, we suggest the following workflow:

• Squash all commits in your branch (git rebase -i origin/{branch})
• Make a note of all changed files (`git diff --name-only origin/{branch}...HEAD
• Run the laminas/laminas-migration tool on the code.
• Clone laminas/laminas-mvc to another directory.
• Copy the files from the second bullet point to the clone of laminas/laminas-mvc.
• In your clone of laminas/laminas-mvc, commit the files, push to your fork, and open the new PR.
  We will be providing tooling via laminas/laminas-migration soon to help automate the process.