SM Refactor

[bakura10]

Hi everyone,

This is my first PR for ZF3. It is a direct backport of my PR on ZF2 repo.

Component is not yet fully tested but coverage should be already quite good.

Let me quickly recap the major changes of this refactor.

Motivation

The initial motivation was to dramatically increase the performance of this core component, while making it easier to use.

Performance wise, this refactor is dramatically faster, consume less memory (benchmark can be found on ZF2 repo).

Breaking changes

Removal of invokables

The concept of invokables no longer exist. Now, everything is a factory. This simplifies the creation code by having a single main way to create object.

A built-in factory to create invokables has been added.

ZF2 code:

return [
    'invokables' => [
        MyClass::class => MyClass::class
    ]
];

ZF3 code:

return [
    'factories' => [
        MyClass::class => InvokableFactory::class
    ]
]

Factories are invokables

To increase the performance and reduce branching, most interfaces (FactoryInterface, DelegatorFactory, AbstractFactory and Initializer) must be callable by implementing __invoke.

This offer a unified way to create objects and dramatically increase performance be removing a lot of instanceof checks.

Removal of aliases

Now that this component is 5.5+, we can take advantage of ::class, and we should encourage this usage.

The usage of aliases has been removed from the main service locator where ::class must be encouraged.

For plugin managers, a specialized plugin manager that supports alias has been introduced (AbstractAliasedPluginManager) while a more efficient, alias-free AbstractPluginManager is still here.

Ability to map multiple class to same factory

This was previously possible in ZF2 but in a hacky way. Now you can map multiple class names to the same factory, and it will be supported by default in the interface signature.

Plugin manager change

Previously, a plugin manager had a method getServiceLocator to retrieve the main service locator.

This was confusing for new-comers, and could lead to hard to debug error because the constructor didn't neforce the need to pass a service locator as a dependency.

Now, what you receive in the factory is ALWAYS the main service manager, not the plugin manager one.

If you create an object through a plugin manager in ZF2 and want to retrieve a dependency in the main service locator you had to do:

class MyFactory implements FactoryInterface
{
    public function createService(ServiceLocatorInterface $sl)
    {
        $mainSl = $sl->getServiceLocator();

        // ...
    }
}

In ZF3:

class MyFactory implements FactoryInterface
{
  public function __invoke(ServiceLocatorInterface $sl, $className, array $options = [])
  {
    // $sl is already the main service locator.
    // If you want a dependency coming from the same plugin manager, you'll need to fetch it first

    $pluginManager = $sl->get(MyPluginManager::class)
  }
}

Unified way of having options

In ZF2 it was very hard to have options on factories. This is made easy now, because each interface have a last parameter called options.

Usage:

$object = $sl->get(MyObject::class, ['key' => 'value']);

// Options are passed as part of the options

Thanks

[weierophinney]

Why? :)

Seriously, requiring a FQCN seems like a massive break for no clear benefit. The Config we currently have in ZF2 is an array; if we give it a FQCN, folks will look for a class matching that name. Additionally, it means every. single. factory. that uses configuration in ZF2 cannot be cleanly migrated to ZF3 due to the fact that the well-known Config service is now… no longer well-known.

Again: I'm fine with BC breaks for ZF3. But we really, really, really, really (I can't repeat that word enough) need to keep a constant eye on migration.

[bakura10]

Make sense, I'm okay for keeping Config here.

[weierophinney]

@bakura10 Overall, a very nice vision, and I'm impressed by how slim the code is! My main concern is migration, and I've outlined the various places where I have specific concerns. The migration scripts can be part of a later pull request, but the specific BC breaks MUST be outlined, either as part of a README or part of a "migration" document (we'll be adding component-specific docs to all component repos soon, and this could be part of that) before acceptance for merge. (If you go the route of a migration document, push it into a doc/book/ tree.)

Finally, I'd really like to see the PR synopsis updated to show the final status of the PR before we merge. Since we're now using the keep a changelog format, for many people, the PR will be the first, most comprehensive place to understand the changes, so having the information up front will help aid comprehension of the changes.

Thanks a ton; again, this is stellar work!

[bakura10]

Hi,

Sorry for the late reply, I won't be able to work on that until next week, but here are a few answers.

Do you mean literally __invoke(), or would the code be using the callable typehint?

Yes, the idea is that factory (or delegator factory, or abstract factory) are just callable. This allow to remove all the check (no need any more to check if instance of FactoryInterface then call createService...).

Also, what will the defined behavior be if you call something that has different numbers and/or types of arguments? As an example, DelegatorFactory accepts:

That's not a problem, because you still need to register factories, abstract factories... separately into the SM. So first the SM will iterate through factories, so it knows the expected order (it's part of the interface), then traverses all the delegators, then the abstract factories.

Additionally, the AbstractFactoryInterface defines two methods, one for creating an instance, but the other for determining if the factory is capable of creating an instance for the named service. How would naming be affected in this case?

The idea is that the creation itself is made through the __invoke. The method that check is still called canCreateServiceWithName. Not really a problem.

Last but not least: what's the migration plan for people who have implemented these interfaces previously? A script can take care of scanning classes implementing the interfaces and then rename the methods, but I'd like to see that script as part of the proposal. As noted in some other comment threads, one of the things I want to avoid is the migration debacle that was ZF1 -> ZF2. Incremental improvements like this proposal are fantastic, but will be all the better if they help define and automate that migration for users.

For now I didn't write one, but to be honest it's less dramatic than the EVM.

I like this change. However, again: how do we manage migration? Likely we can do a script that looks for calls to getServiceLocator() within various factory implementations, but we definitely need to address how that will happen.

Same, I didn't write one, and while I can undertsand the need for this I'm really not interested in taking time for writing this. So it will depend on my motivation.

[bakura10]

@weierophinney , I've made some changes, specifically:

• Fix the feedback
• I've added back native support for aliases right into the SM. I could therefore removed the AbstractAliasedPluginManager class that I introduced. Honestly, the new alias mechanism is really fast and very lightweight, so it should not be a problem in terms of performance.
• I've added a changelog. It does not contain all the changes but should be a good first step.

I still need to update the tests. The only thing that I'm still highly unsure is the options thing. What is your opinion on this @weierophinney ?

[sandrokeil]

Same, I didn't write one, and while I can undertsand the need for this I'm really not interested in taking time for writing this. So it will depend on my motivation.

@bakura10 @weierophinney You are not alone. If this will be accepted, I think somebody can write a migration tool. That's not the problem. Maybe it could be usefull to write PHPCS sniffs which detects BC breaks?

[weierophinney]

👍 Thanks for adding this; I think it will be a fantastic reference.

[bakura10]

This PR now adds support for Interop container. As it will be v3 and allow BC, I was unsure if I shouldn't just remove ServiceLocatorInterface, and make the SM implement ContainerInterface only?

[samsonasik]

should related with #4 that merged to develop

[weierophinney]

This PR now adds support for Interop container. As it will be v3 and allow BC, I was unsure if I shouldn't just remove ServiceLocatorInterface, and make the SM implement ContainerInterface only?

You should rebase; I just merged #4 a short bit ago to the develop branch, and it already does this. :)

In terms of ServiceLocatorInterface, I think we should keep it; it provides a package-specific context.

[bakura10]

Okay, I'll change that tomorrow. It seems that the interop also has exception interface. I've updated this PR but I'm a bit unsure of the hierarchy

[bakura10]

But actually i find it a bit strange to keep both interface. If serviceLocatorInterface extends containerInterface it will just have the same methods. If ServiceManager implement both, it will implement two interfaces that have the exact same content. Isn't it a bit redundant actually?

[samsonasik]

rebasing will little bit hard as it should be pointed to develop, the reason it was pointed to master because develop didn't exists yet. Probably the only way is create new PR...

[bakura10]

Replaced by #6