ObjectManager's Factory should be replaceable depending on service

[Ocramius]

Currently, the Magento\Framework\ObjectManager\ObjectManager accepts a single Magento\Framework\ObjectManager\Factory as a parameter

I've been trying to build better interceptors on top of ProxyManager, since the current Magento\Interception component is quite naive, and doesn't deal with a lot of corner-cases (different PHP and HHVM versions/bugs/gotchas).

The end aim is avoiding any kind of LSP violation (even for constructors) by having interceptors simply have their own factory type.

In pseudo-code, it would be something like this:

class InterceptorFactory extends Factory
{
    public function create($type, $arguments)
    {
        // either handled via inheritance or composition
        $realInstance = parent::create($type, $arguments);

        return $this->createInterceptorViaOtherLogic($realInstance);
    }
}

The ObjectManager would then need to use the configuration to find out the proper factory to be used with a particular service:

    public function create($type, array $arguments = array())
    {
        $factory = $this->_config->getFactory() ?: $this->_factory;
        return $factory->create($this->_config->getPreference($type), $arguments);
    }

This is just one possible approach, but it would make:

• very complex services easier to instantiate
• complete removal of the Interception component, plugging in a more robust solution
• some overhead in ObjectManager#create() and ObjectManager#get()

I'm still digging through the files, but the main point is that instantiation logic is not always getting parameters from a parameter bag and stuffing them in a constructor, for example like in following pseudo-code:

public function create($objectManager)
{
    $object1 = $objectManager->get('object1');
    $object2 = $objectManager->get('object2');

    return $object1->createFrom($object2); // hard to do with the current ObjectManager
}

Maybe I missed the way that I can achieve this, but the current factory implementation seems quite inflexible.

[verklov]

@Ocramius, thank you for your contribution! The team will analyze it and we will get back to you once we have the results.

[antonkril]

@Ocramius Can you list the corner cases Magento\Framework\Interception doesn't cover?

As for constructors, they are not subject to LSP because they are not object methods and are not part of object's interface.

I'm not sure I understand the benefits of instantiation example you provided. We don't support such cases because it can be achieved with standard approaches. Also, $objectManager should not be used in module code directly, because we try to avoid service location.

[Ocramius]

@antonkril mainly access to properties and non-public methods.

As for constructors, they are not subject to LSP because they are not object methods and are not part of object's interface.

Constructors are subject to the LSP as well. They may not be a part of the API in magento's core, but any usage of the constructor of an interceptor (usually done via get_class) may break your code. Yes, this is arguably wrong, but you can't possibly know how consumer code may use a passed in instance. The assumption of constructor injection only is also fairly bold ;-)

I'm not sure I understand the benefits of instantiation example you provided. We don't support such cases because it can be achieved with standard approaches.

The point here is lazy loading. If I want to skip instantiation of a service to make it lazy then I can't do that without overriding how it gets instantiated. More on this can be found here and here (I implemented it for both containers).

Also, $objectManager should not be used in module code directly, because we try to avoid service location.

Service location is perfectly acceptable in the context of factories. The locator doesn't need to leak outside the factory into services.

[otoolec]

@Ocramius Have you seen the support for Proxy classes that already exists? They should help with the lazy loading, unless I'm missing some detail.

[antonkril]

@Ocramius current implementation of Interception allows pluginization of protected methods conceptually. But this access was limited intentionally to prevent 3PD interaction with protected interface of classes in plugins, because it would be a hack, and would render any post-release magento code modifications backwards incompatible.

Constructors are subject to the LSP as well. They may not be a part of the API in magento's core, but any usage of the constructor of an interceptor (usually done via get_class) may break your code.

We avoid usages of get_class because it makes you depend on implementation instead of abstraction. Can you describe in which cases you want to interact with Interceptor constructor directly? We have ObjectManager that is responsible for instantiation. It abstracts 3PD from constructors. You should create your entities with factories, and services should be asked in constructor. So no direct interaction with either constructor or ObjectManager is required.

The point here is lazy loading.

You can read about lazy-loading in Magento with Proxies here

Service location is perfectly acceptable in the context of factories. The locator doesn't need to leak outside the factory into services.

100% agree. I didn't understand that example is from factory. But Magento 2 provides a way to do this without such custom logic.

[Ocramius]

Do I get this correct when I say that you would explicitly override services with their proxies counterparts?

That's pretty much what I wanted to avoid, and instead silently override a service with a different instance (kind-of like a container compiling pass)

[antonkril]

Different instance of a proxy is created, it substitutes expensive service, and when this service is required, proxy creates it. So this happens transparently to the client code. Client code doesn't distinguish if it works with a proxy or original object. Proxies are auto-generated.

[Ocramius]

@antonkril that makes sense, though it seems that it needs manual overrides on the config (by explicitly using the proxy class instead of the real class). That can be fixed via some pre-compiling passes though.

Getting back to factories, I still don't see a way of using a different factory to build my object. Would something like a "factory" support be acceptable? I realize that this is touching very core components, but I would like to work on these before there's any feature freeze.

[antonkril]

@antonkril that makes sense, though it seems that it needs manual overrides on the config (by explicitly using the proxy class instead of the real class). That can be fixed via some pre-compiling passes though.

Yes, you have to declare proxy in config. We had discussions about just declaring types or arguments as "lazy", but decided to leave it explicit for developers to understand the behavior. I'm not sure I understand, how pre-compiling will change the situation? Now we request minimal information in config.

Getting back to factories, I still don't see a way of using a different factory to build my object. Would something like a "factory" support be acceptable? IO

If you are talking about entity (non-injectable) factories, they are auto-generated, but if you create your own factory for your type and name it {your-classs-name\Factory}, it will be used instead of auto-generated. Of course your factory can contain any custom logic you want.

As for services or injectables (instances that you can ask for in constructor): for now we don't have factories for them. Proper object decomposition allows you to avoid factories for non-injectables. Introduction of such factories would add more "magic" and "invisible" code written by 3PD and would let them think less about their object decomposition.

That said, we still have discussions about these "service factories" and if you provide some valid use cases, we can revisit this.

[verklov]

@Ocramius, could you please provide use cases as asked by @antonkril?

[Ocramius]

If you are talking about entity (non-injectable) factories, they are auto-generated, but if you create your own factory for your type and name it {your-classs-name\Factory}, it will be used instead of auto-generated. Of course your factory can contain any custom logic you want.

@antonkril is there also a way of using a factory without depending on a derived class name?

Proper object decomposition allows you to avoid factories for non-injectables.

It's not a perfect world, heh...

Introduction of such factories would add more "magic" and "invisible" code written by 3PD and would let them think less about their object decomposition

It would actually simplify instantiation for various kinds of services that don't just rely on constructor injection. That is not an issue for core itself, but for any dependency introduced in a project.

@Ocramius, could you please provide use cases as asked by @antonkril?

My particular use-case is about making services lazy/proxied (mainly for performance), as well as using own AOP-ish libraries. To do that, I'd basically need to swap out the default instantiation logic somehow, and defer it to a callable that I can either inject into the manager or pass in as a configuration value and that will be instantiated by the manager itself.

[antonkril]

@antonkril is there also a way of using a factory without depending on a derived class name?

Sure.

It would actually simplify instantiation for various kinds of services that don't just rely on constructor injection. That is not an issue for core itself, but for any dependency introduced in a project.

Adapters should be used for that instead of factories. Adapters are also a better idea because they abstract you from specific implementation of the dependency.

My particular use-case is about making services lazy/proxied (mainly for performance)

Can you describle what are the problems of current implementation of lazy-loading and AOP?

[benmarks]

@antonkril I just was speaking about this with @Ocramius at DPC. Happy toi chat more.

[verklov]

The team has processed this issue. The updated code has just been deployed to the repository under version 0.1.0-alpha97. Thank you for your contribution! This issue is now closed.

[Ocramius]

@verklov thanks! Is there any sha1 reference that can be used to verify what was changed, related with this issue? :-)

[verklov]

@Ocramius, since we are performing deployments as a single merged commit, I guess there is only one way to see this. Please use this link https://github.com/magento/magento2/blame/873d38c884fb56411c4ac3bb46a9d4343e4545fe/lib/internal/Magento/Framework/App/ObjectManagerFactory.php#L105. The string that you are interested in appeared in version 0.1.0-alpha97.

[Ocramius]

@verklov ok! Thanks for that =)

[verklov]

You are welcome! :-)