use callable as well as creator

[PawelSuwinski]

This makes core object creator a more universal factory and its a natural consequence of:

silverstripe-framework/src/Core/Injector/Injector.php

Lines 430 to 432 in cb37869

	// We've removed this check because new functionality means that the 'class' field doesn't need to refer
	// specifically to a class anymore - it could be a compound statement, ala SilverStripe's old Object::create
	// functionality

Example of extended syntax usage:

# app/_config/services.yml
  GuzzleHttp\HandlerStack:
    class: GuzzleHttp\HandlerStack::create

[dhensby]

Looks like a reasonable addition that could be pretty powerful.

I think we'd need to see some decent tests here to cover off the use-cases and that injector does indeed create instances as expected this way as well as additions to the documentation.

[emteknetnz]

Does seems like a good addition

As Dan mentioned, we need tests + docs

The example given in the original description refers to https://github.com/guzzle/guzzle/blob/master/src/HandlerStack.php#L47

[PawelSuwinski]

As Dan mentioned, we need tests + docs

OK, I will add some.

[PawelSuwinski]

Done.

[emteknetnz]

I'm in two minds about this one.

Apologies in advance for any incorrect assumptions I've made here, this is a fairly complex topic

I sort of like this idea here, though I am quite worried this will lead to making the yml config even more confusing than it is

The comment about $class not needing to be a class, instead allowing a compound statement, went in 10 years ago when Injector.php was first created. It's not a new thing - b269bad#diff-fe633772e4ca1ed70c69ad396df29e7447d318434f63deb1b97c13168ef874dbR320

It's kind of messy having create($class, because create($classOrMethod, and then having constructor act as methodParameters. The YML config for service classes is already confusing enough as it is, this will only make it more confusing.

It seems like this would be far cleaner that if instead of essentially high-jacking the keys 'class' and 'constructor', there were additional keys available 'method' and 'arguments' (likely we'd use different key names if we were to do this)

I think this work better if we didn't do this in InjectionCreator as it implements the Factory interface, which we cannot update without breaking API, so would need to happen in CMS5. However it might work if this was instead done directly in Injector::instantiate() instead? e.g.

if (isset($spec['method'])) {
    $method = $spec['method'];
    if (!is_callable($method)) {
        throw new InjectorNotFoundException("Method '{$method}' is not callable");
    }
    $arguments = $spec['arguments'] ?? [];
    $object = call_user_func_array($method, $arguments);
    if (!is_object($object)) {
        throw new InjectorNotFoundException("Method '{$method}' does not return an object");
    }
} else {
    $factory = isset($spec['factory']) ? $this->get($spec['factory']) : $this->getObjectCreator();
    $object = $factory->create($class, $constructorParams);
}   

At the same time though, I'm also not keen on this change because it's changing what %$ can represent. In the yml example provided in this pull request:

SilverStripe\Core\Injector\Injector:
  LogMiddleware:
    class: 'GuzzleHttp\Middleware::log'
    constructor: ['%$Psr\Log\LoggerInterface', '%$GuzzleHttp\MessageFormatter', 'info']
  GuzzleHttp\HandlerStack:
    class: 'GuzzleHttp\HandlerStack::create'
    calls:
      - [push, ['%$LogMiddleware']]
  GuzzleHttp\Client:
    constructor:
      -
        handler: '%$GuzzleHttp\HandlerStack'

GuzzleHttp\Middleware::log returns a callable, not a class

Therefore '%$LogMiddleware' is now not the usual service class / singleton, instead here it's a callable. This would not have been possible out-of-the-box before because InjectorCreator goes new $class(...$params);, so now we're changing what %$ can mean, which I'm not particularly keen to do.

I'm not really sure where to go with this one. @dhensby @maxime-rainville @GuySartorelli do you have any views on this one?

[emteknetnz]

Need to decide if this is something we want to proceed with at this stage

[GuySartorelli]

This seems like a really useful and flexible feature on the surface, but it does feel a bit too flexible. I would expect the result of Injector::inst()->get('some-service') to always be some object, but with this change it feels like the result could be effectively anything.

[PawelSuwinski]

I would expect the result of Injector::inst()->get('some-service') to always be some object, but with this change it feels like the result could be effectively anything.

Nope, object always:
https://github.com/PawelSuwinski/silverstripe-framework/blob/InjectionCreator_callableExtension/src/Core/Injector/InjectionCreator.php#L14

[PawelSuwinski]

It's kind of messy having create($class, because create($classOrMethod, and then having constructor act as methodParameters. The YML config for service classes is already confusing enough as it is, this will only make it more confusing.

Yes, a bit, I think that factory setting is a better place for it. I just didn't want to bring new things and concepts just wanted to use what I found here. IMHO a factory that can point only to Factory interface is unnecessary too restrictive. Any callable method that gives an object could be good as well as a factory, and constructor wouldn't be confusing then, as said in Factories docs it is arguments for factory method. I can change this PR this way.

GuzzleHttp\Middleware::log returns a callable, not a class. Therefore '%$LogMiddleware' is now not the usual service class / singleton, instead here it's a callable.

Not exactly, IMHO in this exactly case GuzzleHttp\Middleware went unnecessary too far generalizing return type here. In fact those helper methods always return a Closure instance. So from Injector point of view everything is correct, still right.

[emteknetnz]

OK, if we're validating that things are Objects I'm less concerned because it means that the meaning of %$ is unchanged.

There isn't currently any type checking within Injector.php that $spec['factory'] implements Factory, so it seems like what we might want then is the key factory_method added? Then perhaps we could do something like this:

Injector::instantiate()

$factory = isset($spec['factory']) ? $this->get($spec['factory']) : $this->getObjectCreator();
if (($factory instanceof Factory) || !isset($spec['factory_method']) || !method_exists($spec['factory'], $spec['factory_method'])) {
    // retain existing functionality
    $object = $factory->create($class, $constructorParams);
} else {
    // new functionality
    $method = $spec['factory_method'];
    $object = $factory->$method($class, $constructorParams);
    if (!is_object($object)) {
        $class = get_class($factory);
        throw new InjectorNotFoundException("Method '{$method}' on '{$class}' does not return an object");
    }
}

How does this sound?

[PawelSuwinski]

How does this sound?

Quite well. I added support for static factory method, not every factory class allows instantiation.

So here it is.

[emteknetnz]

It's looking good, a few more changes to make

[PawelSuwinski]

Done.

[maxime-rainville]

Just double checking my understanding of the use case here.

The idea is that occasionally you will use a library that expect you to call a "factory method" on class to create an instance rather than a regular constructor. If you want to instantiate that class with Injector, your only option right now is to create a proper Factory class.

The point of this PR is to update the Injector so that we can simply tell Injector to use the factory method and avoid creating a full factory class.

If my understanding, this seems like a useful approach and something I would support.

[PawelSuwinski]

The point of this PR is to update the Injector so that we can simply tell Injector to use the factory method and avoid creating a full factory class.

That's right. A lot of libs use factory design patterns having it's own ready to use factory methods (like a GuzzleHttp from the example) so factory (class) is already there just method name differs than the one imposed by Factory interface.

[emteknetnz]

Just noting for posterity that symfony simply allows setting the factory_method as the second argument when creating a factory via yml

https://symfony.com/doc/current/service_container/factories.html#static-factories

In a later version of framework, it might be good to align our approach with symfony's, remove the php Factory interface from Silverstripe where the create method is required to be the factory_method

It would also be good to rename constructor to arguments

[emteknetnz]

Great work.

Have tested locally to confirm everything works as expected