Clarification: "The service created by a factory should only depend on the input parameters of the factory"

[mindplay-dk]

@moufmouf can you provide some clarification on the following section of the README, please?

service-provider/README.md

Lines 344 to 370 in a55e717

	### Managing configuration
	The service created by a factory should only depend on the input parameters of the factory (`$container` and `$getPrevious`).
	If the factory needs to fetch parameters, those should be fetched from the container directly.
	```php
	class MyServiceProvider implements ServiceProviderInterface
	{
	public function getFactories()
	{
	return [
	'logger' => [ MyServiceProvider::class, 'createLogger' ],
	];
	}
	public function getExtensions()
	{
	return [];
	}
	public static function createLogger(ContainerInterface $container)
	{
	// The path to the log file is fetched from the container, not from the service provider state.
	return new FileLogger($this->container->get('logFilePath'));
	}
	}
	```

This section appears to stipulate that service provider implementations must essentially be stateless? But it does not explain why.

I frequently use service providers with constructor arguments - I don't believe most existing DI containers mandate or enforce any such restriction, so my first impulse is to remove this clause, as it appears to potentially clash with common patterns and practices used by existing DI containers, possibly creating interoperability problems.

Do you recall why this was a requirement? My best guess is, you intended for providers to be automatically bootstrapped via some sort of service discovery facility?

If that was the reason, I'm not sure this the right approach. While implementing service discovery might be made easier by this stipulation, it is not made impossible by permitting service providers to have constructors with arguments.

For example, auto-discovered service providers with constructor arguments could load these from configuration files, obtain them from the system environment, or load them from a key vault, and so on.

Things like API keys or other access credentials need to come from somewhere, and baking them into a service provider isn't typically going to make sense - if you want a working service configuration, some things are going to be non-optional, and non-optional constructor arguments are the natural way for any other class to require those things.

It's an unusual requirement, and there is no way to enforce it, so as said, I'm inclined to remove it.

Any objections?

[moufmouf]

Do you recall why this was a requirement? My best guess is, you intended for providers to be automatically bootstrapped via some sort of service discovery facility?

Gee, I forgot this part too!

I think your assumption is correct.

I think we also wanted to stress out that the configuration should be stored in the container / come from the container.

I have no strong objection removing this.

One important thing to keep in mind: the getFactories and getExtensions methods cannot rely on the container at all (because for compiled container, you cannot access the container before it is fully built and it won't be built until after the service providers are called).

This section appears to stipulate that service provider implementations must essentially be stateless?

This is not something I view as bad. Having service providers behave in a stateless way makes things easier to manage. As soon as you introduce state, you are at risk of having different results based on the fact the container is runtime or compiled.

What happens if a configuration value changes AFTER the container is compiled? If you know all parameters come from the container, you don't have any issue. If parameters come from somewhere else, you start having issues: you'll need to recompile the container when one of the parameters changes, so you need to find a way to track parameters and purge the compiled container accordingly.

Viewing factories as strictly pure functions could also allow for a number of crazy optimizations the Symfony people are used to (like inlining the service, etc...)

[mindplay-dk]

This section appears to stipulate that service provider implementations must essentially be stateless?

This is not something I view as bad. Having service providers behave in a stateless way makes things easier to manage. As soon as you introduce state, you are at risk of having different results based on the fact the container is runtime or compiled.

Are talking about the same thing though?

I would think it extremely strange if someone wrote a provider where the individual factories maintain state, or where the provider itself has state that changes.

But as far as having providers with dependencies, that sounds like an extremely normal everyday OOP thing to me? I wouldn't like to ship a provider that requires you to bootstrap a bunch of configuration entries based solely on following directions in a README or something - providers giving you half your bootstrapping and expecting you to sort out the rest by hand, that sounds fragile to me?

I would definitely prefer this:

$container->add(new MailProvider(host: "...", username: "....", password: "..."));

Over this:

$container->add(new MailProvider);

I mean, it looks nice, but it doesn't work, does it?? 😅

You need to manually register your credentials separately:

$container->set("MailProvider.host", "....");
$container->set("MailProvider.username", "....");
$container->set("MailProvider.password", "....");

This creates so many problems with versioning, readability, etc. - you can only actually make the connection by reading the manual of every single third-party provider you've plugged in, then 🤞 fingers crossed, hopefully those dependencies don't change between versions, hopefully types stay the same, hopefully you registered any optional configuration values under the correct name 😬 and so on, basically everything a DI container was supposed to help with.

I wouldn't find MailProvider very helpful if it's not going to register all the dependencies and give me a working email service - I would likely even prefer to just write the bootstrapping myself.

I don't generally like "rules of hooks" type situations, where things don't actually work by default, where you have to warn people not to use regular everyday language mechanics and principles, or (heaven forbid) ask them to use a linter, which can tell you when perfectly normal everyday code for some reason isn't allowed by your API.

Is that what you meant?

If so, this might be a longer discussion. 😅

[mindplay-dk]

My current PSR draft includes this section:

4.2. Idempotency

Calling a factory or extension multiple times with the same container instance SHOULD result in the same service being created each time. Factories and extensions SHOULD NOT maintain internal state that modifies the returned service.

I do agree that service providers should be idempotent - this doesn't mean they're not allowed to have constructors or accept/hold configuration values required for bootstrapping.

You can have two provider instances that don't produce the same result - however, if you have identical provider instances, they must produce the same result, at all times.

@moufmouf is this sufficient to address the issues you're concerned with?

[moufmouf]

👍 on the Idempotency chapter, I wouldn't have stated it in better terms.

Regarding your previous comment, mmm..... well... I did indeed meant I prefer

$container->add(new MailProvider);

instead of:

$container->add(new MailProvider(host: "...", username: "....", password: "..."));

I prefer this because it makes autodiscovery / autoregistration possible.

It indeed means you need to manually register your credentials separately.

I remember @nicolas-grekas (from Symfony) told me each service provider should therefore expose a list of required services / parameters. We started working on an interface to expose the requirements of a service provider but never finished. Somehow, your new proposal could fill that void since you are making the dependencies of a service provider explicit.

function getFactories() {
    return [
        MailerInterface::class => function(string $mailer_host, string $mailer_username, string $mailer_password) {
            return new MailProvider($mailer_host, $mailer_username, $mailer_password);
        }
    ]
}

Here, it is obvious to the container, that 3 parameters are needed: $mailer_host, $mailer_username, $mailer_password.

Now, the issue with what I'm proposing here is that it is harder to build services providers that provide several mailers (unless you pass in parameter an array of "host/username/password", but this is not ideal....)

[mindplay-dk]

I prefer this because it makes autodiscovery / autoregistration possible.

But I don't believe providers with constructor arguments make autodiscovery / autoregistration not possible?

For example, let's say you have:

class MailServiceProvider implements ServiceProviderInterface
{
    public function __construct(
        private string $host,
        private string $username,
        private string $password
    ) {}

    // ...
}

Now, as explained, these dependencies are not optional - if the service provider didn't register this configuration itself, you would just have a service provider that doesn't work unless you do it yourself.

So the configuration needs to come from somewhere, anywhere, let's say (God forbid) a YAML file:

MailServiceProvider:
  host: xyz
  username: root
  password: root

Now, your auto-discovery/configuration framework loads up the configuration values from YAML/JSON/INI files, from an external configuration store, from your composer.json or the system environment, and in PHP 8 (having named parameters) you can literally just do:

$provider = new $provider_type(...$loaded_config[$provider_type]);

And (shazaam!) working providers ready to bootstrap, n'est-ce pas? 😄

Are you sure this gets in the way of what you want to do? Or does it actually make it better, safer and simpler?

[mindplay-dk]

For the record, the Idempotency section is in the draft now.

@moufmouf I'd still like to hear your thoughts on my last post? I remain unconvinced we'd need to stipulate any restrictions on normal OOP to allow for auto-discovery - if you don't object, I will likely close this issue in favor of keeping the PSR simpler.

I am not closing it just yet though, as I am not 100% certain. 🙂