container-interop / service-provider

[EXPERIMENTAL] Promoting container/framework interoperability through standard service providers
72 stars 10 forks source link

Advocating for a ->getRequirements() method #44

Closed pmall closed 10 months ago

pmall commented 6 years ago

Im a huge supporter of the service provider autoloading idea. But no matter how hard I'm trying to implement it in my own personal projects, there is some issues which cant be avoided. Lets face it: it will allways be up to the end developer to choose which implementation to use for some dependencies. What if we specify a way for service provider to declare those dependancies?

Let's say I want my session, cache and queue management based on redis. Obviously I can require a package providing a session handler, one providing a Psr-6 cache and another one providing a queue management service, all three requiring a Predis client instance. Some autoloader can automatically provide those three packages service providers to my project container but there is absolutely no way in the current specification for them to know what concrete instance of Predis client they should get from the container. My assumption here is there will never be a way because it is up to the end developer to choose how many redis servers he wants to use and which one go with one service.

My wish is to have the container to fail nicely when a requirement is not met. By nicely I mean throwing an exception containing all the info a clever framework would need to catch it and indicate to the end developer what he should do in order to make everything works.

Also - this must be noted but I didn't made my mind about it - based on this assumption, this is a bad practice to use \Predis\Client::class as a factory id (following the previous example), because there is no such thing as universal redis connection, only dependencies to higer order services. Unless you want to play around with a single Predis client without injecting it. This is wried.

I think service providers should have the ability to tell they need a specific class instance to be mapped to a package vendor specific id. Then it is up to the end developper application service provider to associate a factory to this id. This is what I learnt from using the service provider interface, I'm very open to hear suggestions which would prove me wrong.

Also I read somewhere about havig one scoped container per service provider/package (@mindplay-dk). As I think the concept is elegant, I have a hard time figuring out how it would work. I guess it would be hard to implement. Also who need a scoped container if we have scoped ids? Not sure about this but I throw the idea.

My proposal here is a simple ->getRequirements(): array method can solve a lot of issues regarding its simplicity. Lets go back to the above example. I want session, cache and queues to be handled by Predis clients. Obviously I don't want to code session handler, Psr-6 caches and queue manager so I autoload the three packages. I is up to me to specify which Predis instance should go with each service:

<?php

class RedisSessionHandlerServiceProvider implements ServiceProviderInterface
{
    public function getRequirements()
    {
        return [
            \Predis\Client::class => '{session.vendor.namespace}.client',
        ];
    }

    public function getFactories()
    {
        return [
            // As we say so we expect a redis connection in '{vendor.session.handler}.client' id.
            RedisSessionHandler::class => function ($container) {

                $client = $container->get('{session.vendor.namespace}.client');

                return new RedisSessionHandler($client);

            },
        ];
    }
}

'{vendor.session.handler}' is a placeholder for example. Would be replaced with a concrete string. Same for the cache implementation and the queue management service provider.

Then in your app, you map factories with the vendor ids:

<?php

class AppServiceProvider implements ServiceProviderInterface
{
    public function getRequirements()
    {
        return [];
    }

    public function getFactories()
    {
        // Here we map the session and cache to the same Predis client but the
        // queue one to a different one. This is what the developer wants there
        // is no way an autoloaded package can figure this out.
        return [
            '{session.vendor.namespace}.client' => function ($container) {

                return $container->get('app.redis.default');

            },

            '{cache.vendor.namespace}.client' => function ($container) {

                return $container->get('app.redis.default');

            },

            '{queue.vendor.namespace}.client' => function ($container) {

                return $container->get('app.redis.queue');

            },

            'app.redis.default' => function () {

                return new \Predis\Client(...);

            },

            'app.redis.queue' => function () {

                return new \Predis\Client(...);

            },
        ];
    }
}

This is the simplest approach and yet we can take benefits from it. It is so simple to implements for containers they could register all the factories then inspect all the requirements. When one is missing it can fail nicely by telling which service provider requires an instance of what class under what id. What do you think about this?

mindplay-dk commented 6 years ago

From my perspective, these problems have all been solved already, or they're not really problems. In my opinion, by even starting down the road of service provider auto-loading, you're choosing an elaborate solution to a simple (or even non-existent) problem.

I mean, look:

$container->addProvider(new ConnectionProvider("root", "passw0rd"));
$container->addProvider(new UserProvider());

It's pretty straight-forward - any hard dependencies on things for which there are no defaults, such as the username and password for a database connection, are clear visible right there, with IDE auto-completion and what all. If you need to temporarily disable one of these providers, you can just comment it out. Simple.

What you're proposing as an alternative to these two lines of code, is far more complex, more error-prone, less transparent, and more clumsy both to set up and work with.

Hard dependencies, such as the DB username and password, have to come from somewhere - so now you need a configuration facility of some sort. You're trying to avoid writing code, so you're likely going to go for a data format like JSON or something, which is much less flexible than code, and hides your dependencies by removing them from the context in which they're needed - your configuration file loader/bootstrapper, for all intents and purposes, is a locator, which is what you were trying to avoid by using a DI container in the first place.

If I want to disable one of the providers temporarily, now I have to either do it from a configuration file somewhere, or if you go all the way with autoloading, worse, I'll have to uninstall the entire package just to disable the provider. You've also lost the ability to disable only one of the providers of a package that comes with more than one.

You're building all of these facilities and taking on a bunch more complexity, and losing a lot of flexibility and transparency, just so you can avoid writing (typically) one line of code for each package.

In my opinion, you're favorizing ease of use over simplicity, and in this case, it's a really ugly trade-off with a whole bunch of drawbacks.

I don't mean to be harsh, but no one should be that averse to writing one line of code.

I don't at all understand what it is you're trying to accomplish with this idea. Bootstrapping a DI container is already dead easy and really simple - why would you trade that away for a tiny amount more perceived ease of use, when clearly it's going to complicate things and provide no real benefits?

My favorite Einstein quote of all time:

image

He was being facetious, you see - he uses the word "simple" first implying it's true meaning, which is the oppisite of "complex"; but then uses it again poignantly to imply "easy", which is where a lot of people get mixed-up - "easy" can actually be the opposite of "simple", which I believe is the case here.

I mean, do what you want, but ideas like this one will never have my support. I would rather teach people knowledge than try to remove minor obstacles such as bootstrapping service providers, which is an easy and meaningful exercise that helps you understand the components you're working with, rather than trying to hide them from you.

Just my $.02 :smile: :heart:

pmall commented 6 years ago

I totally understand you don't like the idea of autoloading and your points against it are interesting.

Also I can't agree more with you about code over configuration, that's why I love this interface proposal: it let me code my service provider factories instead of going through some bizarre config files wrote in another language, annotations or god know what.

Yet I still believe the service providers should have a way to delare their requirements. If this problem is already solved well I'm missing the point and I don't know where to catch up on this.

You'd tell me it is solved by using constructor injection. I think this idea is appealing and your example make sense but I don't get how it would work in a more complex scenario. Lets say I have two packages service providers requiring a Psr6 cache implementation. Well I need to build those cache objects and inject them in the service provider of my packages.

<?php

$predis_cache = new Psr6PredisCache(...);
$file_cache = new Psr6FileCache(...);

$container->register(new SomePackageServiceProvider($predis_cache));
$container->register(new SomeOtherPackageServiceProvider($file_cache));

So the cache instances are built even when the objects requiring them are never retrieved from the container. How would you handle such a case?

Is your point is we should do something like this:

<?php

$container1->register(new PredisServiceProvider($host, ...)); // Provides a \Predis\Client factory
$container1->register(new PredisPsr6ServiceProvider()); // Ok because container1 has a factory for \Predis\Client
$container1->register(new SomePackageServiceProvider()); // Ok because container1 has a factory for \Psr\Cache\CacheItemPool

$container2->register(new LocalFlysystemServiceProvider($root, ...)); // Provides a \League\Flysystem\FilesystemInterface
$container2->register(new FlysystemPsr6ServiceProvider());  // Ok because container2 has a factory for \League\Flysystem\FilesystemInterface
$container2->register(new SomeOtherPackageServiceProvider());  // Ok because container2 has a factory for \Psr\Cache\CacheItemPool

// somehow use a composite container to combine exposed parts of $container1 & $container2
$container = new CompositeContainer([$container1, $container2]);

?

If so, well, I need to meditate about this :)

mindplay-dk commented 6 years ago

Lets say I have two packages service providers requiring a Psr6 cache implementation.

I usually solve this by injecting the provider's dependency - the component ID - rather than injecting the component itself. I use this approach for database connections, caches, etc.

mindplay-dk commented 6 years ago

I do find myself missing a way to declare provider dependencies, and we have been debating this on the team - but I'd prefer to find a simple solution, one that has more uses, and builds on what we already have, as opposed to inventing an entirely new facility just for that.

One way would be to simply register a boolean true as a "component" - a flag indicating that a given provider has been bootstrapped... a provider would e.g. $container->register("myapp.db", true), and a provider that depends on the database connection would do something like $container->depends("myapp.db") - the container-factory, when you ask it to build a container, would check any listed dependencies, and would throw if anything is missing.

One advantage of this approach, is you can declare dependencies on anything - using flags to signal that a provide has been bootstrapped is just one option. So, for example, $container->depends(PDO::class) would work for projects that use a single bootstrapped database connection, etc. - or any other case where you depend on a single component and don't care which provider it came from. Using this for something like a PSR-7 factory would make a lot of sense.

This approach also lets you substitute any third-party provider for a custom one, as long as you know the name it uses to indicate that it has been bootstrapped.

One drawback to this approach, is these indicator components aren't very formal. One way to address this, would be adding an interface that providers can implement, something like getProviderName(), enabling the container-factory's addProvider() method to query the provider's name and register it automatically. (Perhaps even defaulting to registering the provider's class-name internally, for providers that don't implement the interface - so that e.g. $container->depends(SomeProvider::class) would work by default.)

We're still unsure about any of these ideas, which is why we haven't attempted anything yet, and haven't come forward with the idea. Until just now ;-)

moufmouf commented 6 years ago

Hey guys,

There are really 2 discussions in this thread. One about autoloading and one about service providers declaring their requirements. These should really be 2 separate topics.

About "autoloading":

First, maybe we should call it auto-discovery of service providers since autoloading is usually used for classes (PSR-4...).

I'm a huge fan of auto-discovery. I know @mindplay-dk thinks this is favoring "easy" over "simple", but if you look at the trend in current frameworks, 2017 is clearly the year of auto-discovery. This year alone, Zend-expressive, Symfony 4 (via Flex) and Laravel 5.5 all added auto-discovery.

I do think the standard should be compatible with the notion of auto-discovery.

I have nothing with the fact that @mindplay-dk prefers passing parameters in the service-provider constructor arguments like this:

$container->addProvider(new ConnectionProvider("root", "passw0rd"));

but personally, I don't like this approach. I prefer fetching the configuration from the container itself:

$container->get('DB_HOST'); // containers can ALSO contain configuration values

If we can make a standard that supports both options, that's all the best.

Regarding declaring their requirements

@pmall : 2 weeks ago, at Paris ForumPHP, I've had a chat with @nicolas-grekas (from Symfony) regarding service providers. Nicolas has an idea quite similar to yours. I'n not 100% sold to his concept but I promised I would write a blog article about his idea (blog post coming in the next weeks, haven't had enough time yet...)

Nicolas idea was wider than what I propose below. Regarding declaring the requirements of the service providers, we was thinking about something like this:

    public static function getSubscribedServices()
    {
        return [
            'debug' => '?bool', // The service provider CAN use an entry named "debug" that MUST be a bool
            'twig_extensions' => '?'.TwigExtentionInterface::class.'[]', // The service provider CAN use an entry named "twig_extensions" that MUST be a an array of TwigExtentionInterface instances
        ];
    }

I don't know yet if this is the perfect solution, but I do agree with @pmall and @nicolas-grekas when they say that we need some way for service providers to expose their requirements. That would allow frameworks to do some static checks which is always good.

Another idea that came to my mind would be to offer a "verify" method that could return an array of unmet expectations:

    /**
      * @return UnmetExpectationInterface[]
      */
    public function verify(ContainerInterface $container) : array
    {
        $expectations = [];
        if (!$container->has('redis.session.client')) {
            $expectations[] = new UnmetExpectation('This service provider expects the container to contain an entry named "redis.session.client"');
        }
        // ....
        return $expectations;
    }

The verify method can check for advanced problems that your getRequirements method cannot check (for instance checking that a container contains a cache entry that can be either PSR-6 or PSR-16 compliant)

There are probably lots of ways to solve this problem and we are certainly only getting started, but I'm 100% :+1: to go in that direction.

mindplay-dk commented 6 years ago

I prefer fetching the configuration from the container itself

I would use that approach for optional dependencies - never for hard requirements. Your provider has dependencies - using the container as a service-locator for the provider's dependencies is (no offense, but) a bit ironic, since you're using a DI container to avoid exactly this sort of thing.

Using your approach would be more robust as well though, if we had some means of validating that providers/dependencies have been bootstrapped at the time when the container is created. (although that seems like a work-around to me - when you could have just used constructor arguments to accurately indicate you have hard dependencies, which is both simpler and adheres better to the principle of dependency inversion, but, I digress... each to his own.)

shochdoerfer commented 6 years ago

What is the point of a has() check when the get() check potentially can still fail? Just wondering if that idea really makes sense or if it is not sufficient to throw exceptions in the callables when dependencies cannot be retrieved?

mindplay-dk commented 6 years ago

What is the point of a has() check when the get() check potentially can still fail?

That's not what I'm proposing - a has() check is an immediate check, a call to depends() is you ordering the container-factory to validate itself before creating a container, e.g. when you're done bootstrapping everything.

Just wondering if that idea really makes sense or if it is not sufficient to throw exceptions in the callables when dependencies cannot be retrieved?

That's already what happens automatically in almost every container implementation.

It's not sufficient, because missing dependencies will not surface before you hit a feature in your app that depends on one of the missing dependencies - you won't know about missing dependencies before it's too late. Knowing at container instanciation that something is missing would be much more meaningful.

moufmouf commented 6 years ago

@shochdoerfer I think the idea is to offer some ways to check the dependencies "statically" (so a CLI tool could validate your configuration and you could run that tool in your continuous integration builds).

For instance:

$ vendor/bin/console di:validate
> ERROR: Service provider Foo expects a service to be called "baz" of type Bar. None found.

Do you say that the interest is limited because you could also iterate over all the entries of the service provider, try to instantiate them and then simply catch the NotFoundExceptions? That would indeed be a simpler way to find misconfigurations and that does not require an additional method.

One thing you can do with an additional method is tools to help configure your container. Something like:

$ vendor/bin/console di:validate
> ERROR: Service provider Foo expects a service to be called "baz" of type Bar. Your container does not contain "baz" but contains "foobar" of type Bar. Do you want to create an alias "baz" of "foobar"? (y/n)

In the example above, I'm using the knowledge that "baz" is expected to be of type "Bar" to propose some form of help to the user.

I don't know if it makes much sense (I'm thinking out loud here)

pmall commented 6 years ago

There are probably lots of ways to solve this problem and we are certainly only getting started, but I'm 100% :+1: to go in that direction.

Glad to hear that! As far as I'm concerned the only thing I would disagree is the specific notation you used in the examples. As I said earlier what makes this specification promising is to use plain php code to configure containers, stating to add '?' and '[]' around scares me a bit. I'd be more comfortable with something like this:

<?php

public static function getSubscribedServices()
{
    return [
        // Using interface constants. Last two defaults to required and single.
        'debug' => ['bool', self::OPTIONAL, self::SINGLE],
        'twig_extensions' => [TwigExtentionInterface::class, self::REQUIRED, self::MULTIPLE];
    ];
}

Maybe the ->verify() method is more suited. Didn't think about it, this is a nice perspective, as well as cli tool to run the verifications.

shochdoerfer commented 6 years ago

@moufmouf I am referring to the sample code that uses the has() check in the verify method. Doing that seems useless because the has() call can return true but the get() call can potentially throw an exception due to some other problems. That means verify is a useless check - at least to me - as the service can't be built even though "the configuration" might be valid. Sure we can check if some requirements are met upfront what what gain does bring when we still can't be sure if the component can be correctly created or not. That means yes, the only valid approach would be to pull each and every service out of the container.

It kind of feels like we are going the PSR-6 route and make things more complicated than needed. The beauty of PSR-11 is that it is very, very simple. We should try hard to find a similar way for service-provider ;)

Of course I am biased from my work on Disco where thanks to code the configuration is always valid - or if not, you would instantly see it. Ideally we would find a similar approach for service-provider.

mindplay-dk commented 6 years ago

If it's relevant, here's my current proposal for provider dependencies to be added to mindplay/unbox.

mindplay-dk commented 5 years ago

One year later - our project has grown a lot, we have a typical 25-30 providers bootstrapped on a project, and the pressure to solve this problem is growing.

Our providers use constructor-arguments for their dependencies - e.g. values passed directly from a configuration file (simple values like username/password) and this has been very clear and effective.

For costly dependencies (such as a PDO instance) we pass a service-name to the provider's constructor, which has been much less clear, and has proven less reliable at scale - some people try to pass the actual connection, others pass a service-name but forget to actually register the service, or at times these things end up broken with iterations of refactoring etc.

Since this proposal appears to have stalled (with no activity in over a year) I will most likely move ahead with the mentioned proposal and I guess we'll see how this pans out in practice.

Thinking about this again, I am somewhat of the opinion that this feature isn't necessarily essential to interoperability of providers. We might should consider closing this issue and moving ahead with the rest of the proposal? I think the proposal is still meaningful without this feature?

In other words, it would be nice to have something like, as it would make for more reliable providers - but it is functional without this feature, is it not?

pmall commented 5 years ago

@mindplay-dk I agree with you. I still work a lot with providers and now I think this feature is not essential.

In fact I'm not far from thinking the current interface is perfectly fine for the purpose of service provider interoperability. A library can expose factories and extensions through a provider, done. Most of the issues we discussed in this github repo are related to project configuration, and can be solved by exploiting them as they currently are.

For this specific issue, it think it can just be solved by throwing meaningful exceptions. "The container must provide a PDO instance for the 'some.pdo.service' id in order to build X". Then there could be a command line script executing all the factories and reporting errors. But it would be just a tool exploiting service providers, it is not much related to service provider interoperability, and you are free to use it or not.

There could be a list of "best practices" for provider interoperability, for example here we want the provider to require a specific id and not PDO::class, to let the developer free to "bind" any PDO instance to this provider. I don't see how we can enforce this with code.

mindplay-dk commented 5 years ago

For this specific issue, it think it can just be solved by throwing meaningful exceptions. "The container must provide a PDO instance for the 'some.pdo.service' id in order to build X".

When would you implement that check?

Getting this exception at the time when you hit the factory-function that tries to look up the component isn't anymore useful than what your container would throw? Likely the NotFoundException message thrown by most containers is more or less exactly what you would type out by hand.

More important to me than the error-message is the timing - do you find the missing database connection the moment you attempt to bootstrap your DI container? Or does it show up later, when you hit a controller that actually uses the database connection?

Being able to catch incomplete bootstrapping right away is definitely preferable to me.

There could be a list of "best practices" for provider interoperability, for example here we want the provider to require a specific id and not PDO::class, to let the developer free to "bind" any PDO instance to this provider. I don't see how we can enforce this with code.

If it's relevant, you might want to take a look at this feature, which we're currently testing:

https://github.com/mindplay-dk/unbox/pull/18

We're attempting to do exactly that: enforce provider requirements with code.

We're about to test it on a larger project to see if it's worth while.

I'm not sure if the idea is very interesting in terms of interop or not? But figured I'd post it here, as it's a long-standing problem and something that has turned out to be increasingly costly as we grow to many providers, with many inter-dependencies, implemented by different teams...

pmall commented 5 years ago

Likely the NotFoundException message thrown by most containers is more or less exactly what you would type out by hand.

Yes but there is no info about what this container entry is expected to be. What if the entry is defined but it is not the expected type? My point was meaningful exceptions could be thrown from factories and it might be enough. But I agree with you it would be way better to have checks at bootstrap.

But what are we able to check at bootstrap except the presence of required container entries? If that's enough we could just have this:

<?php

public function getRequirements() {
    return [
        'some.dependency' => 'some error message to show when not defined',
    ];
}

Then if a dependency has a bad type the factory requiring it could throw a meaningful exception. There is no way to check the type without retrieving the dependency from the container anyway.

We're attempting to do exactly that: enforce provider requirements with code.

I'm not sure I understand how this is related. My point was I can't see a way to enforce good practices for interoperability, not factory requirements.

An example of good practice would be: you should require a service named 'my.awesome.package.pdo' from the container instead of PDO::class, so the developer can bind the pdo instance he wants to your factory when he uses multiple PDO instances in his application.

Another one could be you should always provide a service with is fully qualified class name and use aliases for interfaces it implements. This way the developer has one implementation bound to the interface name but can also tag together all implementations using their class names.

Well i'm not sure enforcing good practices is needed, I guess packages with well designed service providers will have more success than poorly designed ones.


I feel most of the confusions we have comes from the fact we didn't make a clear separation between what is container configuration and service provider interoperability.

To me container configuration is defining factories and registering them into the container. It is everything which is specific to an application. Also service providers is not the only way to configure a container: for example with a library i'm developing I can collect factories from php files for easy configuration.

On the other hand interoperability is providing factories creating services provided by a package. It may or may not be used during container configuration. It frees up from the pain of having a package per framework for your library.

I think a way to provide factories, extensions and maybe the ablity to specify the id of some required container entries at bootstrap is enough for interoperability. What do you think about this?

mindplay-dk commented 3 years ago

Rereading this thread today after some recent explorations in this space again.

Two things.

One, I disagree with my past self on some of the points above. My thinking back then was, I wanted to track dependencies between providers - but I've since come to realize, providers are really just implementation details that define dependencies, and they shouldn't become dependencies themselves. So I'm no longer liking that idea.

And two, I'm uncertain about the idea of manually specifying "dependencies I'd like to verify", which now seems to me like an unfortunate workaround. Whether it's done declaratively like @pmall suggested, or procedurally as in the PR I posted above, the question I run into is, why would I need to designate specific dependencies for validation? Why can't the container just validate everything?

Manually having to specify dependencies to validate seems redundant, but also quite error prone - I could forget to specify an important dependency, or I could forget to remove an old dependency, forcing somebody to bootstrap a component that doesn't even get used. Either way it's not good.

In a compiled language, you would probably just validate all the dependencies when creating the first container instance. In PHP, loading all classes to check their dependencies isn't acceptable, so not an option here.

So I got to thinking, what if the component registrations themselves used a format that could be validated without loading the classes? And I came up with this little experiment, which seems interesting: basically, every registration is a closure, with singletons getting resolved automatically, and a PHP 8 attribute you can apply to specify the component names of non-singletons. It precludes any sort of magic or auto-wiring - and it's a bit verbose, but not too verbose with PHP 7.4 fn expressions. And it has other interesting properties besides full validation - IDE support and static analysis in particular.

I haven't decided yet if I want this feature in my own IOC container, but it's an idea with interesting qualities, I think, so just floating it for inspiration or discussion. 🙂

One last thought related to this thread: I argued in the past for just using constructor arguments to your providers - that way, no one can bootstrap a provider with missing requirements. That was the route we ended up taking in my last job, and it started out great, but became quite error prone in the long run, as some of these argument lists grew to 30-40 arguments - making the constructor call sites quite unreadable and very risky to change.

But today we have named arguments in PHP 8. Constructor call sites with dozens of arguments aren't really a problem anymore - as long as everyone agrees to call them with named rather than positional arguments, they remain quite readable and safe to change. (I only wish there'd been a way for constructors/functions to require named arguments. Perhaps this is solvable with static analysis tools, which could, for example, require any function with more than 3-5 arguments be called with names.)

Anyhow, I think my conclusion on the original topic, is that all requirements ought to be knowable, somehow - specifying them is redundant and not really safe, so I'm probably "thumbs down" on this feature, at this time.

The PHP world has changed a bit since the start of this discussion, and perhaps there are better approaches to explore now? 🤷‍♂️

mindplay-dk commented 1 year ago

I've had to revise my position on this yet again. 😅

Today, I added support for provider requirements to my container - for a few reasons:

Mainly, the realization that providing all requirements via constructors isn't always possible - in particular, if you have circular requirements, say, for example, your admin provider needs some sort of plugin, and the plugin needs something from the admin provider. Doing that with constructors isn't impossible - you can pass service names rather than actual services, but I know from experience that this can get pretty confusing.

Also, the notion of "abstract" requirements - as in, you have a dependency on something that isn't actually a service or value, something abstract, like "remove this file from your public directory", "cycle the private key" or "pour the tea", who knows. You just need an indication that it has been done.

Another small detail is, this feature enables you to add human-readable context when you require or provide something - so you can give a reason, which will be displayed if the requirement fails.

Five years on - never thought I'd close that PR, haha. 😄

XedinUnknown commented 1 year ago

@mindplay-dk, heya! Good to hear from you!

I'm a little lost in the current status of this proposal. What exactly are we trying to achieve/solve here, that isn't already achievable?

Is it that a provider needs to declare dependency on other providers? I disagree with that notion, and I guess agree on this with your more recent self: it's services that depend on other services, not providers. I had made the mistake in the past of giving my modules names, and having modules depend on other modules is an extremely inflexible approach, compared to just services that depend on other services, and any service provider can satisfy those dependencies.

Is it having a way to know what the dependencies of a service are, without actually resolving the service definition? I agree, and that's why I created https://github.com/Dhii/services: a utility package that defines an OOP callable Service representation, and makes that have dependencies that you can retrieve with Service::getDependencies(). This allows for a dependency graph to be built before any service resolution. It also allows a more declarative approach to service definitions, because you can just declare them as deps when defining the service, and the definition callback will get resolved deps that you can typehint against. You can find many examples of usage here.

Is it that service providers should receive a getRequirements() method? I don't believe so, as I've established that service providers don't have deps; only services do. When you run an application where a service is being resolved that depends on another service that is not defined, you get a NotFoundException, as intended: the chained exceptions from all the get() calls will show you a (often too verbose and hard to read) "path" of all the keys that couldn't be resolved, including the first one - that's how you know which dependencies you now need to satisfy. Also, with a convention (or, we can try and bump that into a BCP or a standard, if you'd like that) such as dhii/services, it's now possible to do static analysis on service dependencies; just a matter of putting that idea into code, but so far I haven't had any pressing need for that. So, I do not support adding getRequirements() to the service provider interface.

XedinUnknown commented 1 year ago

I'd like to add a couple of things.

First is that I've been using dhii/services approach for a few years now in many enterprise applications, and this works really well. I really enjoy the separation of a simple rule for service definition e.g. callable(ContainerInterface): mixed - from ways to add convenient ways of defining services in different ways, whether it's by invoking a constructor with a list of resolved deps, or just passing that list to a factory callback, or just returning a simple scalar value. This has a lot of value to my mind (no pun intended), and I belive that this separation should persist. If there's a need to standardize further on service definition, we can take our examples and do so; however, I believe that we should do so without adding anything to the simple and clear service provider. It's even possible to add own DSL syntax by using NEON entities, and viola - you can write most of your service definitions in a declarative way, with type information and other intellisense features.

Secondly, but perhaps more importantly: I believe that the service provider facet is much more important than ContainerInterface for modularity, and if anything - it should be simplified. I don't see a way to make a generic compiler for containers; but I see many amazing ways to do that with a service provider, because in contrast to the container, a service provider enumerates its services, and even provides a distinction between two types of definitions: factories and extensions. If you can enumerate definitions, you can direct a generic compiler to serialize them - perhaps after resolving all their extensions into a single callable function. At the moment, I cannot think of a better way to compile an abstract container with abstract services. Of course, specifically for scalars, it's possible to just cache them all together in a more compact way. And maybe even some other serializable services. Regardless, while I acknowlege the critical function of ContainerInterface in modularity, I believe that it can be done in a much more flexible way if stacking/chaining/combining service providers rather than containers. If modules expose or are represented by service providers, very generic mechanisms are accessible, and many optional feature implementations are possible.

In my applications, modules mainly represent slices of application configuration. App configuration can be sliced in different ways, and this is what defines the eventual config. Since I use a separator convention to create path-like keys e.g. vendor/module/service_name, that eventual config is a tree. I use different implementations of ServiceProviderInterface and ContainerInterface to attach, detach, or refine nodes/subtrees in that configuration. For example, the DeprefixingContainer allows consumers to know a single e.g. setting key without knowing that it corresponds to an app config node, and CompositeCachingServiceProvider predictably combines service providers in different modules, such that the factories and extensions that come later override or get applied on top of those that came before. Check out this article for a more in depth description, if you're interested.

That said, the service provider standard doesn't appear to be moving anywhere, because there doesn't seem to be much usage of it, apart from projects of the select few individuals like myself and perhaps some others in this repo. That' is unfortunate, and it is pushing me to de-couple my modularity standard from this repo, and instead incorporate my own service provider standard. Of course, I'd love to avoid that, and to have a PSR-20: Service Provider or something like that, which would enable generic tools to work with our code. The pressure I'm experiencing is the fact that this repo is not stable, has very little traction, and nobody knows who even has control of releases anymore. It's hard and increasingly unwise to depend on such packages in my enterprize applications, and I'd like to be able to serve my users better.

I'm open to discussing all and any of this, share more about how I use this, and learn more about how others do. I'd be happy to do a call with the other members of the workgroup and maybe just contributors to decide the way forward. If there's sufficient traction, I'm happy to work on a plan and push this thing forward. If there isn't, I guess we go our separate ways, and use whatever standards we can agree on.

mindplay-dk commented 1 year ago

it's services that depend on other services, not providers. I had made the mistake in the past of giving my modules names, and having modules depend on other modules is an extremely inflexible approach, compared to just services that depend on other services, and any service provider can satisfy those dependencies.

well said! 🙂👍

Is it having a way to know what the dependencies of a service are, without actually resolving the service definition?

Yep, that is why I added this feature.

Your approach with dhii/services is something I've studied many times, I find it quite interesting - and as you explained, it approaches the same problem, just in a very different way. I definitely like how this approach self-insures against missing dependencies, and everything being verifiable.

However, this approach is also verbose - in Unbox (my container) I've always tried to balance "correctness" against ease of use, and personally, I find that the ease and speed of just typing register(MyController::class) and having everything just work (because most controllers only depend on singleton class/interface registrations) is just too good to miss out on.

As said, I do see the value in what you're doing and have reviewed your code many times - at one point, I came up with this:

https://bitbucket.org/mindplaydk/mindplay-funbox/src/master/

With this, I was trying to enable that static analysis goodness, but via PHP type-hints rather than strings - and with the option to use an attribute to name a service instance, so yeah, strings for non-singleton dependencies. I wondered for a long time if I could reconcile this with Unbox, but I'm not sure it would make sense - it would just require too much boilerplate for anyone to upgrade from the current version. But it is an approach I'd like to explore further - it's not unthinkable I could work some of the shortcuts from Unbox into this "funbox" prototype - it just seems less practical to work this into Unbox, as it's fundamentally designed to be "dynamic" rather than "static" in nature. Changing that would break a lot of things.

Is it that service providers should receive a getRequirements() method?

No, I'm not here to advocate that either. I ended up in this thread after doing some updates on Unbox, found myself disagreeing with my former self, and just wanted to share, correct myself and explain why. 🙂

That said, the service provider standard doesn't appear to be moving anywhere, because there doesn't seem to be much usage of it, apart from projects of the select few individuals like myself and perhaps some others in this repo. That' is unfortunate, and it is pushing me to de-couple my modularity standard from this repo, and instead incorporate my own service provider standard.

...and that's probably what happens everywhere. 🙂

I think that's always going to be the hurdle for any conception of a shared provider format - it's just never going to be as convenient or capable as either yours or mine or any of the other containers developed by the good folks who travel this repo.

There are just so many concerns to balance, and everyone seems to balance them in a slightly different way - and I think, with that in mind, the people behind this proposal took the right (the only?) approach by keeping it extremely simple. But that's also the problem - what you get is a kind of "lowest common denominator", a thing that every DI container can handle, but not really a thing that anyone wants to directly use unless they have to.

So I don't know. It's hard to say where this proposal should go from here, if anywhere. I am definitely here for any new ideas you'd like to pitch though! If you decide to toss up a prototype, I'd always be interested in taking a look. 👍

btw, Unbox is also backing a large family of news portals - although you couldn't tell by the download numbers on Packagist, because apparently last year my former employer took many of my open-source projects inhouse and made them closed-source. 🤷‍♂️

mindplay-dk commented 10 months ago

Since this discussion isn't really focused on one specific topic, and doesn't appear to be going anywhere, I am closing this issue.

If you think further discussion on any of these topics is needed, please open one or more threads in Discussions with specific topics. 🙂