Injector factories should be passed the full service definition

[sminnee]

Injector factories could be more flexible if they were passed the full service definition as a 3rd argument.

For example, we could use a custom factory to do some special processing on selected properties. This relates to my work on #4430

• Advantage: Factories can be used to tidy up confusing aspects of Injector service definition
• Disadvantage: Factories could do anything, risking that we get a more divergent & confusing set of configuration approaches. We may end up with service definitions that have options other than class, constructor, properties, and dependencies.

One way to mitigate that disadvantage would be to set some guidelines for writing good factories.

The change would be trivial and I'll post a PR if I get buy-in, but I want to run it past people first.

[sminnee]

Note: the specific use-case that I want is that I want to grab the properties.Handler argument, do some processing, and pass its contents as repeated calls to Logger::pushHandler(). In doing this, my hope is that same configuration approach can be maintained even if Monolog improves things in the future to mitigate the need for a custom factory.

An alternative approach to this would be to shift the handling of the properties and dependencies argument to become part of the factory. Since it all relates to instantiation & set-up of the object, I don't see why that would be wrong, but others may disagree.

@nyeholt since you wrote the original class, it would be good to get your feedback.

[sminnee]

@stevie-mayhew you may have views on this.

[sminnee]

A related issue that came up discussing logger was the ability to inline service definitions within properties.

Right now, to create a handler with a custom formatter, two service definitions are necessary:

Injector:
  DisplayErrorHandler:
    class: SilverStripe\Framework\Logging\HTTPOutputHandler
    constructor:
      - "notice"
    properties:
      Formatter: %$DisplayErrorFormatter
  DisplayErrorFormatter:
    class: SilverStripe\Framework\Logging\DetailedErrorFormatter

It would be nicer if a service definition could be inlined:

Injector:
  DisplayErrorHandler:
    class: SilverStripe\Framework\Logging\HTTPOutputHandler
    constructor:
      - "notice"
    properties:
      Formatter:
        class: SilverStripe\Framework\Logging\DetailedErrorFormatter

However, this gets a little bit "magic": you have to know that an map containing a class key should be interpreted as a service definition. It would be nicer if there was a mechanism for indicating an inlined service definition. I'm not sure what that should be, though.

To be flexible, I would say that service definitions should be able to be inlined wherever a service definition reference can currently be used:

• constructor arguments
• properties
• array elements within either other those

In the absence of core changes, it will be possible to create custom factories that implement this in specific cases. It would also be possible to create an InlinedServiceDefinitionFactory that would scan any constructor and properties references and process the inlined service definition.

It would be easier to do this nicely if the processing of constructor and properties was shifted to be a responsibility of the factory: that is, the factory becomes fully responsible for processing a service configuration.

You could have this, which strictly speaking, would be a factory-factory, which I can't quite believe I am advocating.

$factory = isset($spec['factory']) ? $this->get($spec['factory']) : $this->getObjectCreator();
$factory->createFactoryForService($spec)->create();

Or we could have this, which is closer to what we currently have. It's a little arbitrary that class & params are broken out separately.

$factory = isset($spec['factory']) ? $this->get($spec['factory']) : $this->getObjectCreator();
$factory->create($class, $params, $spec);

The mixing in of constructor arguments is also a little arbitrary: strictly speaking, constructor arguments should be arguments to the factory, rather than being bound directly to the constructor arguments of the underlying object. The factory-factory makes it easier to do something like this:

$factory = isset($spec['factory']) ? $this->get($spec['factory']) : $this->getObjectCreator();
$factory->createFactoryForService($spec)->create($constructorParams);

We could also cache the result of $factory->createFactoryForService($spec).

[nyeholt]

(looks like you got your post in a few seconds before I could get my reply in!)

Just bringing across a couple of points from #4430 -

• I would like to improve some of the way properties are resolved so that, for example, a property could be another service definition that the injector would subsequently bootstrap, or retrieved from the environment, or x or y or z rather than the hardcoded scalar value or %$ lookup as current
• Sam disliked having to create a separate Formatter service definition just to map it as a property of the handler service definition (global namespace pollution, additional entities for a developer to know about)

Generally speaking I agree with your points regarding the need to define additional global entities just for them to be referenced in the one place. My concern around it is related to not being able to subsequently redefine the configuration of those internally defined objects, but at that point you can always override the definition of the top level object. I guess the test of what would be appropriate is whether the inner object would be constantly overridden. What I mean is (pseudo config only)

Injector
  EmailLogWriter:
    class: \Some\Namespace\EmailLogWriter
    properties:
      to: me@example.com
      subject: There was an error on your test site
  FileLogWriter:
    class: \Some\Namespace\FileLogWriter
    properties:
      filename: /file/system/path/output.log
  Logger:
    class: \Some\Other\Namespace\Logger
    properties:
      handlers:
        - %$EmailLogWriter
        - %$FileLogWriter

versus

Injector:
  Logger:
    class: \Some\Other\Namespace\Logger
    properties:
      handlers:
        0:
          resolve: true
          id: EmailLogWriter
          class: \Some\Namespace\EmailLogWriter
          properties:
            to: me@example.com
            subject: There was an error on your test site
        1: 
          resolve: true
          id: FileLogWriter
          class: \Some\Namespace\FileLogWriter
          properties:
            filename: /file/system/path/output.log

In this scenario both log writers could/would be re-configured by end users - which provides more clarity?

Either way - I think the concept of adding a proper property resolver layer has merit - I'm not sure yet what the performance impact would be though.

Also - just because FactoryFactory :+1: x :100:

[sminnee]

OK, so without needing to agree on core changes to the Injector, my factory-factory (or "Service Configuration Processor", if you prefer) approach would let us do this, constraining the semantics of nested service definitions to the implementation of NestedServiceConfigurationProcessor.

Injector:
  Logger:
    factory: NestedServiceConfigurationProcessor
    class: \Some\Other\Namespace\Logger
    properties:
      handlers:
        -
          class: \Some\Namespace\EmailLogWriter
          properties:
            to: me@example.com
            subject: There was an error on your test site
        - 
          class: \Some\Namespace\FileLogWriter
          properties:
            filename: /file/system/path/output.log

This would mean a few refactorings:

• Shifting the inject() call/code to become part of the ServiceConfigurationProcessor; Injector would mainly just be a service registry. I would be inclined to break inject() into 3 or 4 methods, each of which does 1 thing.
• Passing $constructorParams as an argument to instantiate (and then passed to create), rather than modifying the spec.

Perhaps ServiceConfigurationProcessor can be set up as an alternative interface, so that existing Factorys can keep working. Note that I would deprecate Factory rather than maintain the two separate abstractions.

[nyeholt]

As an interim solution that looks like the most reasonable approach; improved property resolution would probably be the better core change to make longer term, but that is more disruptive than the proposed mechanism in the short term.

[sminnee]

The nice thing about this approach is that we can experiment with different property resolution approaches in individual ServiceConfigurationProcessors, to get some real-world use before baking an approach into core.

[camspiers]

For inspiration:

http://php-di.org/ http://symfony.com/doc/current/components/dependency_injection/introduction.html https://github.com/auraphp/Aura.Di https://github.com/thephpleague/container http://laravel.com/docs/5.0/container https://github.com/silexphp/Pimple

[sminnee]

Thanks for the list of existing implementations, @camspiers :-)

It obviously begs the question "should we replace the container with another implementation?" At a minimum I'm keen to keep an eye on PSR-11 :-)

[sminnee]

What are the other implementations doing?

Pimple:

• Pimple let you bind lambas to generate a new service. Because it's just PHP code, you get a lot of things like looking up other services for free.
• Pimple uses singletons by default, and has a wrapper if you want to create a factory. Each service is either a singleton or a factory—the caller can't choose.
• Pimple recommends that if you want parameters, chuck them in the global container config. There's no other place to put them. This seems like it would be ugly, but I guess it lets you expose high-level configuration.

Laravel Service Container:

• Again, we register lambdas as factories or singletons that pull configuration out of the $app global state.
• Laravel calls these lambdas "resolvers"
• A lot of work here to automatically populate properties based on type-hinting to interfaces. This is cool, but beyond the scope of the changes I'm suggesting.

League Container:

• Although it has a fluent syntax to define services, it seems that lambda are the recommended way, like pimple and laravel. It calls these "factory closures".
• The factory closures don't take arguments.
• Rather than setters, it lets you configure method calls (which would help with, e.g., pushHandler in Monolog)
• "Service provider" is used to describe a sub-set of the overall container config: it registers a set of services.

Symfony:

• Again, rather than setters it lets you configure method calls.
• Symfony explicitly distinguishes parameters from registered services. This does have the effect of letting you expose "normal user config" in more obviously named areas.
• I also noticed here that nested Yaml arrays can be a bit more concise. :-)

PHP-DI:

• I didn't look too much into this, but it seems more or less more of the same.

What does it mean for us?

My proposal suggests a that rather than a lambda, we provide a class implementing an interface. Given we're using Yaml, that seems like a reasonable difference. I'll keep moving-away-from-yaml as out-of-scope for now. While providing lambda-specific config might be overkill, when we're instantiating a class, it's probably useful.

No-one has really done with the inlined-resolver, although you don't need this if you work with lambdas—you just add a line to your resolver.

It's interesting that no-one else has added support for including the equivalent of a constructor-with-arguments in their DI container. This is an existing feature of Injector and its seems to be used a fair bit - e.g. to pass the $_SESSION content into the Session object retrieved from the container.

There's some quite cool stuff in other libraries using reflection on type hints to auto-mapping dependencies; Laravel is probably the most advanced here. I'm not sure that re-implement this stuff ourselves is a good idea; at that point it's probably worth swapping over to a different library.

Allowing arbitrary method calls would be good, rather than the only allowing method calls that start with 'set'. It would need some way of calling the same method multiple times; the Symfony Yaml syntax for this looks reasonable as long as you use [ and ] to construct the nested arrays.

Changes to my previous suggestion

Not too much, really, but a few tweaks:

• Rather than ServiceConfigurationProcessor, I might follow the league's lead and call these internal methods ServiceDefinitions.
• Implement the Callable interface on them rather than expose a create() method.
• Consider supporting a methods: section in addition to constructor: and properties: (probably a separate PR).

Unrelated to the review of existing implementations but other ideas I've had:

• Pass the injector to the service definition constructor - ServiceDefinition::__construct(Injector $injector, array $definitionConfig) - so that it can be used to fetch additional services.
• Create a public method Injector::getServiceDefinition(array $definitionConfig) that will instantiate the right ServiceDefinition implementation; this can be used to support service definition inlining.

[nyeholt]

Wayyy back when I first wrote the injector, I think only the Symfony injector was around and was heavily code based for wiring things. I wanted something that was text based (or at least, text wrapped in arrays...) rather than explicit object creation, and that mapped nicely to the config system at the time in 3.0.

There's some quite cool stuff in other libraries using reflection on type hints to auto-mapping dependencies

You can set Injector's autoScanProperties field to true, and it'll attempt to automatically resolve any publicly declared variables to same-named services. It doesn't do type checking at all though - at the time of implementation I think there were issues with PHP opcode caches causing reflection to return empty information for some bits and pieces, and the alternatives were a pain. Probably wouldn't be hard to include, but it works pretty well without.

[camspiers]

I find Symfony's DI system with compiler passes to be incredibly powerful. So much so that we integrated Symfony's DI system into SilverStripe and created an interop layer (when building an ecommerce system).

In Symfony you can tag services with additional information, and then write a compiler pass that uses that information to do something specific with the service. For a canonical example, say you are building a command line interface, and you want a way for module creators to add commands to your command line tool. To achieve this, you can write a compiler pass that finds all services with the "command" tag added to them, and add those services to the command app service (which would effectively result in something like $commandLineApp->addCommand($someCommand)).

https://github.com/heyday/heystack/blob/master/src/DependencyInjection/CompilerPass/Command.php#L30 https://github.com/heyday/heystack https://github.com/heyday/heystack/blob/master/src/DependencyInjection/SilverStripe/HeystackInjectionCreator.php https://github.com/heyday/heystack/blob/master/src/DependencyInjection/SilverStripe/HeystackSilverStripeContainer.php

[camspiers]

Also, I really love the way PHP-DI's service definition works. Leveraging the power of PHP in service definition is really appealing to me:

See the example under "Expressive configuration": http://php-di.org/

YAML, JSON and XML are turned into DSLs for service definition in many DI systems. I find this to be a significant disadvantage (I struggled a lot with the expressiveness of SilverStripe's DI system), which means that almost any additional features to the DI system needs to be baked into the DSL (Symfony gets around this with tags + compiler passes). Using the power of an actual programming language but with a preference for declarative style definitions unlocks a lot of options for extension beyond what DSLs style approaches give.

I really don't like the combination of the Config system with the Injector system, and found numerous cases when I had to resort to service definition via _config.php files using Injector. This was because the config system just wasn't expressive enough.

[sminnee]

I can see the benefits of using Lambdas too, but at this stage, I'm less interested in the specific merits of individual DI container and service provider systems, as I am frustrated at the amount of monolithic tight-coupling we have. It shouldn't be the case that SilverStripe comes down from the mountain to decide whether Yaml or Lambdas are better—project developers should have some choice.

There are two main areas of interconnection:

• Retrieving service object from the container: This is what PSR-11 is trying to address; however, we'll see how long it takes to. It's basically an interface defining ->get($id) and ->has($id) methods. If we bound to an interface rather than the Injector class we could probably let people swap this out, but right now there's no clear 3rd party interface to bind to. In the short term we could just make InjectorInterface and let people build adapters.
• Defining services within the container: Injector does this with configuration sourced from yaml. Other systems do this in other ways. Ideally, we would want to be able to compose a number of service providers into a single DI container. This would let us pull in SilverStripe's default services (configured in the yaml, presumably) and use them alongside. The league's container's ServiceProvider would allow this; not sure about other systems.

One approach would be to build a silverstripe-league-container package that bridged league/container into Injector in these two places. The league is an example; presumably we could do the same thing for other DI containers.

Once PSR-11 comes out I'd like to see us use the interfaces provided by that, and then we can hopefully tie things together without needing adapter packages.

[nyeholt]

Agree that the PSR should dictate the specifics of how SS interacts with an injector of any description. In the interim I think your previous suggestions are probably the most efficient step to Get Things Done.

[sminnee]

Yeah, I better stop talking and start coding. ;-)

[camspiers]

:+1:

[dhensby]

So.....