Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Factory vs Provider interfaces #60

Closed
mindplay-dk opened this issue Dec 2, 2023 · 23 comments
Closed

Factory vs Provider interfaces #60

mindplay-dk opened this issue Dec 2, 2023 · 23 comments

Comments

@mindplay-dk
Copy link
Collaborator

It's been a long time... does anyone recall?

Did we ever consider the "obvious" approach of simply standardizing on the factory facet itself?

The current proposal currently does two things: service registrations and extensions.

How come we didn't just propose a simple factory interface?

interface ContainerFactoryInterface
{
    /**
     * @param string $id
     * @param callable(ContainerInterface $container): mixed $create
     */
    public function register(string $id, callable $create): void;

    /**
     * @param string $id
     * @param callable(ContainerInterface $container, mixed $value): mixed $extend
     */
    public function extend(string $id, callable $extend): void;
}

This seems more direct and likely requires less explaining?

People are used to getting "the thing they want" from PSR interfaces - the current provider interfaces basically gives you arrays and lets you figure out the rest. It might be easier for end users to understand?

To my understanding, and as I recall, the current proposal was designed and intended for container implementors to use? But perhaps a proposal designed for end users would be easier to understand, and might be more successful?

Pimple gets 60K installs per month and still rising, and it's basically this.

Perhaps we shouldn't have been designing for container developers first, but rather for the everyday developer, who just wants to ship their package with generic bootstrapping?

Just something that occurred to me. There may have been another reason we went this direction, but I don't recall.

We might have had more success with something that feels more "frameworky" to the developer, as most of the PSRs tend to do?

@shochdoerfer
Copy link

Sounds reasonable to me and is similar to what we have done with PSR-11 where we don't care about how the container does the job but how you can interact with the container.

@mindplay-dk
Copy link
Collaborator Author

@shochdoerfer exactly my thinking. :-)

I'm also trying to second guess myself though - I would love to hear potential criticisms or challenges from @mnapoli and @moufmouf who were the main contributors behind the current proposal. :-)

@mnapoli
Copy link
Member

mnapoli commented Dec 9, 2023

It's been years, I'm too far from the original problem.

I think the main thing you want to do with any proposal is:

  • take the most popular DIC
  • try applying the suggestion to them

This is the only way to demonstrate that a standard can be implemented and has a chance of being implemented.

@mindplay-dk
Copy link
Collaborator Author

that makes sense, but so... Symfony? it's basically a programming language in itself, haha.

I'm definitely not going there, only a "Symfony developer" would have that kind of patience 😂

@mindplay-dk
Copy link
Collaborator Author

mindplay-dk commented Dec 11, 2023

Actually, after some further investigation, this wouldn't even work for my own latest DI container experiment - but the current proposal in fact would not work either.

My latest experiment is a DI container that can perform a basic dependency analysis - being able to verify the container configuration ahead of time, and (potentially) render a tree-view or diagram of dependencies.

This is only possible by using a declarative format for service lookups - the current proposal does not support that, and what I proposed here does not either, as both rely on a function (ContainerInterface $container) doing dynamic service lookups when the service is requested, i.e. too late.

I believe this may have been the same limitation @XedinUnknown pointed out in the past? as I recall, the current proposal did not work for his container either. (?) (this one).

In order for my container to be supported, I would need a format like the following - for a service A depending on B and C:

[
    "A" => [["B", "C"], fn ($B, $C) => new A(B, C)],
    "B" => ...
    "C" => ...
]

Or as an interface:

interface ContainerFactoryInterface
{
    /**
     * @param string $id
     * @param string[] $deps
     * @param callable(...): mixed $create
     */
    public function register(string $id, array $deps, callable $create): void;

    /**
     * @param string $id
     * @param string[] $deps
     * @param callable(...): mixed $extend
     */
    public function extend(string $id, array $deps, callable $extend): void;
}
EDIT: added missing string $id parameters

This still looks "foreign" to most DI container syntax though, and my own experimental container doesn't look like this - it uses parameter types/names, and an attribute for non-singletons.

I don't know if there's still any interest in developing any of these ideas here?

And part of me wonders (as I did from the beginning) if it's even really possible or worth while, as the main reason to choose a DI container in the first place, is about what configuration looks like - most containers are just ContainerInterface after registration anyway... 🤔

@mnapoli
Copy link
Member

mnapoli commented Dec 11, 2023

I don't know, the project kinda stopped 7 years ago, it's been a while 🤷‍♂️

I don't see any benefit in pursuing this unless there is proven interest from users and from the most popular DIC containers.

@moufmouf
Copy link
Contributor

Hey @mindplay-dk ,

I don't see any benefit in pursuing this unless there is proven interest from users and from the most popular DIC containers.

I still believe having such a standard would be immensely beneficial for the PHP community as it would allow a standard way to build bundles (if I use the Symfony terminology).

Regarding the proposed approach, it has been a really long time, but I remember one of the important considerations was to be able to "compile" the container (like Symfony does) easily. So the interface we design should be able to run "once" during the compiling phase instead of being called on each request, if the container we use can be compiled.

@mindplay-dk
Copy link
Collaborator Author

I'm working on a (side) project which could really benefit from being decoupled from the DI container - hence my renewed interest.

Not that I ever really lost interest - I've returned to this project many times over the years, as I also believe this could really benefit the PHP community.

Regarding the proposed approach, it has been a really long time, but I remember one of the important considerations was to be able to "compile" the container (like Symfony does) easily. So the interface we design should be able to run "once" during the compiling phase instead of being called on each request, if the container we use can be compiled.

What does that mean, "run once"? Can we use closures?

Does the current provider format work for Symfony and PHP-DI? (I think those are the compiled containers?)

@moufmouf
Copy link
Contributor

Does the current provider format work for Symfony and PHP-DI? (I think those are the compiled containers?)

Yes, it does.

For Symfony, the code of the compiler pass is here: https://github.com/thecodingmachine/service-provider-bridge-bundle/blob/1.0/src/ServiceProviderCompilationPass.php

Basically, it scans through all the items of the service provider and creates one "factory" service per item in the service provider.
The result is a compiled container that contains every single item that are supposed to be in the final container.

@bnf
Copy link

bnf commented Dec 12, 2023

FYI, we use a similar compiler pass like thecodingmachine/service-provider-bridge-bundle (together with an in-tree fork of container-interop/service-provider – we hoped this to become a PSR soon ;) ) in TYPO3 (a CMS used mainly in Germany, ~170k installations) since 2019: https://review.typo3.org/c/Packages/TYPO3.CMS/+/58255
This actually still works with Symfony DI v6: https://github.com/TYPO3/typo3/blob/v12.4.8/typo3/sysext/core/Classes/DependencyInjection/ServiceProviderCompilationPass.php

The current service provider architecture getFactories() and getExtensions() turned out to be very simple to implement and still flexible enough to cover all our usecases. Especially getExtensions() is important as it is basically a very simple abstraction for all sorts of plugin-usecases, e.g. collection routes or middleware stacks in registries.

For us it made sense to have this kind of a generic php-based format, as we use an uncached container for our recovery tool (usage without Symfony DI) and Symfony DI for the main application. The service definitions in our service providers could therefore properly be re-used for both usecases: recovery tool and main application.

So I agree with @moufmouf, there is still demand for service providers, especially when you use middlewares from third party packages, where it would be ideal to just instantiate their service provider in order to hook them up to whatever container is being used.

@mindplay-dk
Copy link
Collaborator Author

mindplay-dk commented Dec 12, 2023

@BFN thanks for chiming in :-)

your fork (this one presumably) what is the reason for that? it looks identical to what we have in the repo currently?

@moufmouf @bnf can either of your say if the interface I described above would work for you? (note that this was edited since I posted it)

I'm sure no one wants to convert everything they've built already - and I understand that the current interface is already somewhat established, but the fact is it does not work for at least two containers that I know of, and/or would break some of their most prominent features. But if it turns out no meaningful common interface is even possible, establishing that has value.

@moufmouf
Copy link
Contributor

I think your interface would be a challenge to implement with a Symfony compiler pass.

interface ContainerFactoryInterface
{
    /**
     * @param string $id
     * @param string[] $deps
     * @param callable(...): mixed $create
     */
    public function register(string $id, array $deps, callable $create): void;

    /**
     * @param string $id
     * @param string[] $deps
     * @param callable(...): mixed $extend
     */
    public function extend(string $id, array $deps, callable $extend): void;
}

If I write a "bundle", using this interface would look like this:

$containerBuilder->register("myService", ["dependency"], function() {
    return new Dependency();
});

The issue here is that Symfony needs to compile the container. It will try to create a PHP file containing the "myService" service and when this compiled container is called, it should be able to get a reference to the factory. The only way to do that would be to run the $containerBuilder->register call on each request.

Now, imagine, you have 1000 services in your bundles.
That is 1000 calls to register on each request.
The whole point of compiling a container is to avoid this kind of work on each request.

So in addition to your interface, you would need some kind of interface that allows you to make a lookup into the service provider for a given key (which is basically what the current proposal is doing).

I'm not sure I am explaining the issue correctly. It is definitely easier to understand if you have played previously with compiler passes. Is my message clear?

@moufmouf
Copy link
Contributor

Also, before starting container-interop/service-provider, we did try container-interop/definition-interop.

It was an attempt to have something 100% declarative (and therefore statically analysable). We failed to go any further in that direction because (if I remember correctly), that approach would be too slow for non compiled containers (and it was also way more complex to implement)

@mindplay-dk
Copy link
Collaborator Author

@moufmouf how about a declarative format with dependency IDs, would that work?

class MyServiceProvider implements ServiceProviderInterface
{
    public function getFactories()
    {
        return [
            "A" => [["B", "C"], fn ($B, $C) => new A(B, C)],
            "B" => ...
            "C" => ...
        ];
    }
    
    public function getExtensions()
    {
        return [];
    }
}

(I'm not sure writing this by hand would be a great experience, just trying to establish what's possible...)

@mindplay-dk
Copy link
Collaborator Author

Another recurring thought in my head: a fully declarative format (e.g. JSON or, heaven forbid, YAML) which would actually be used to code-generate containers in the individual formats required by your container of choice - you'd actually get readable code you can examine, written in your DI container's "native tongue". 🤔

@moufmouf
Copy link
Contributor

how about a declarative format with dependency IDs, would that work?

Yes, that would work!

a fully declarative format (e.g. JSON or, heaven forbid, YAML) which would actually be used to code-generate containers in the individual formats required by your container of choice

If I recall correctly, this was actually my first idea. It got busted on the FIG mailing list.

@mindplay-dk
Copy link
Collaborator Author

Just kicking around ideas, but what do you think about doing something more PHP 8 ish?

class MyServiceProvider implements ServiceProviderInterface
{
    public function getFactories()
    {
        return [
            "A" => fn (B $B, C $C) => new A($B, $C),
            "B" => fn (#[ID("pdo.read-replica")] PDO $db) => new B($db),
            "C" => ...
        ];
    }
    
    public function getExtensions()
    {
        return [];
    }
}

this is declarative as well - we simply assume singletons, based on the type-hints, unless an ID attribute is specified on the parameter. a bit less verbose - writing in this format would be a lot less repetitive and error prone.

question is whether that would work with compiled containers?

@moufmouf
Copy link
Contributor

Indeed, we can have something way better. Back in the days, function return types were not even a thing because we had to cope with PHP 5.6 compatibility.

That being said, I really think that having something easy to write is a non-objective.
What we need is something that is easy to implement.

After that, it can be the responsibility of library authors to provide libraries with opinionated ways of implementing a service provider.

For instance, I wrote the Funky library on top of container-interop/service-provider to make it easy to write a service provider.

Funky allows writing a service provider purely using annotations:

class WhoopsMiddlewareServiceProvider extends Funky\ServiceProvider
{
    /**
     * @Factory(
     *   tags={@Tag(name="middlewares.queue", priority=MiddlewareOrder::EXCEPTION_EARLY)}
     * )
     */
    public static function createMiddleware() : WhoopsMiddleware
    {
        return new WhoopsMiddleware();
    }
}

In your proposal, you assume singletons unless an ID attribute is specified. This is already pretty opinionated (although I have to agree most containers work this way nowadays). You could instead keep the interface ignorant of this singleton / ID attribute thing and build a lib on top of the interface to make life easier for users.

@mindplay-dk
Copy link
Collaborator Author

mindplay-dk commented Dec 13, 2023

So the idea is you write your container factory so that it implements ServiceProvider? that's interesting - I've never even considered that possibility. The question is whether most people would even think of that? Aren't most (if not all) of the existing libraries, that provide a ServiceProvider, hand-written? I guess most of them were written at a time when no other option was available though. 😌

But so, yeah, if the idea/goal is to provide interop among DI containers, then perhaps the end user experience isn't of any real importance - I can see that. The current proposal might be just fine - the shape of it ultimately isn't very important, and if anything, I suppose you just want something that is as simple, as lightweight, as generic and compatible as possible.

Maybe there is nothing to "fix" here. 🙂

Perhaps the real question is, how do we drive adoption. I suppose I could start by making my own DI container support provider-interop! I think, for my latest experimental container, this would actually be quite easy and natural. For my existing DI container, I'm fairly certain I'd have to make some breaking changes. Might be nothing major though.

I will definitely give this some more thought. I might go ahead and see if my experimental container can act as a ServiceProvider, as I believe that might be relatively "low hanging fruit".

I'll let you know how it goes. 🙂

@mindplay-dk
Copy link
Collaborator Author

Okay, so I tried implementing provider-interop in my experimental container and, while that works, the current provider format does in fact break the main feature of that container: the container-factory being able to verify itself before creating a container.

This is simply not possible with the current format, since it doesn't declare the dependencies of the closure in question.

So I'm back to the following idea:

class MyServiceProvider implements ServiceProviderInterface
{
    public function getFactories()
    {
        return [
            "A" => [["B", "C"], fn ($B, $C) => new A($B, $C)],
            // ...
        ];
    }
    
    public function getExtensions()
    {
        return [
            "X" => [["Y", "Z"], fn ($X, $Y, $Z) => new X1($X, $Y, $Z)],
            // ...
        ];
    }
}

This would obviously be a major breaking change, but is there any other reason you'd be opposed to trying this out? My project is actually stranded, so I'd be happy to put in some time.

What do you think would be required as proof of concept?

A fork of Pimple and/or another container, and porting an existing provider to demonstrate each of the requirements?

Note that factories/extensions could still ask for ContainerInterface if they need to do dynamic lookups for some reason - this change doesn't take away any existing features, as far as I can tell.

@moufmouf
Copy link
Contributor

What do you think would be required as proof of concept?

If you want to try something, in my opinion, you should test at least Pimple + Symfony. They are the 2 extremes in term of container. Pimple is ultra simple and does everything at runtime. Symfony is compiled and does a huge bunch of optimization in advance. If you manage to find something that works for both, you are on a good track.

I've been thinking a bit more about your proposal to use modern PHP features like attributes. My first reaction was that this was probably not needed. But the more I think about it, the more I wonder if you don't have a good idea here.

Maybe simply marking a method with a #[Factory] attribute to declare it has able to create a service could do it?

class MyServiceProvider {
    #[Factory]
    public function myService(MyDependency $a, MyOtherDependency $b, #[id="DB_HOST"] $dbHost): MyService {
        // Code to instantiate the service
    }
}

By default, the ID of the service could be the FQDN of the returned type of the factory.

We could have a bunch of properties associated to the Factory attribute:

#[Factory(["id" => "my_custom_id", aliases=["an_alias"], tags=["some_tag]]])]

The hard part for the current spec in my point of view is that we never managed to find a way to handle properly tagged services, or lists of services (like when you need to inject a PSR-15 middleware at a certain point in the list of middlewares provided by the container. Using attributes, we could add "optional attributes" that can be implemented by some containers if they support the feature (like tagging, etc...).

And if the Factory is a plain method, you can easily do static analysis on it.

I'm not sure what the others think about it? This has been in the back of my mind for a few days now... :)

@mindplay-dk
Copy link
Collaborator Author

Well, I took your point before, and I still think you were right - the idea you're describing here is, again, cool, but it's opinionated, and it's not actually required in order to achieve interoperability. If somebody wants that, this (and my idea) could simply be packages that give you this API and generate (or directly implement) the service provider interface.

I don't know what "tags" are in the context of a DI container, but I'm sure that's another concept you could implement? Inserting middleware into a list is certainly possible with extensions - if you need a safer or more convenient way to do that, it sounds like that's possible without affecting interoperability, just put another function in front of your function.

I'm completely sold on the idea that this particular standard should be as unopinionated as possible - it doesn't need to be practical or consider DX at all, because most developers will never need to work with it directly. I think that's how we should describe it in the spec eventually - hand-rolling a service provider might be okay for very small and simple providers, but it shouldn't be the first option, and definitely not the only option.

I know that's not the situation right now, and most service providers are probably hand written, but I think that's one of the things we need to work on, in order to demonstrate the viability of an eventual proposal.

I would love to hear your thoughts on the simple structure I've laid out here vs the current state of #54 which tries to do the same thing in a more roundabout way. #54 has four interfaces instead of one, as it is trying to be more formal - at this point, I'm not sure whether that really matters at all. Safety constraints and convenience can come from a provider factory.

At least, that's my opinion at the moment - I probably need to prove it by actually implementing something.

@mindplay-dk
Copy link
Collaborator Author

Closing this in favor of open discussion here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants