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

Adding support for every kind of callables and removed the need for static getServices method #404

Closed
wants to merge 4 commits into from

Conversation

@moufmouf
Copy link

commented Apr 22, 2016

Hey Matthieu!

I had a couple of hours to spare and was feeling in the mood for a coding frenzy :)

I've been doing some tests, forked your service-provider branch and finally added support for any kind of callables in PHP-DI.

I've kept some optimizations: public static function are still called directly, while the other methods must go through a Registry object that is in charge of registering / instantiating the service providers.

There are certainly a lot of things to be improved, but that's already a fairly good proof of concept.

I've updated the unit tests so you can have a look at what's going on easily.

Let me know what you think about this!

++
David.

…tatic getServices method
@moufmouf

This comment has been minimized.

Copy link
Author

commented Apr 22, 2016

Ouch! Broke the PHPUnit tests for PHP < 5.6 (because I added dependencies on a PHP 5.6+ package). Anyway, that can be fixed later. PHPUnit tests are working for PHP 5.6+

moufmouf added 3 commits May 7, 2016
… as an implementation detail)
@moufmouf

This comment has been minimized.

Copy link
Author

commented May 7, 2016

I've improved the implementation a bit by hiding the dependency on the Registry class.

Now, you can do:

$containerBuilder->addDefinitions($myServiceProviderInstance);
// or
$containerBuilder->addDefinitions(MyServiceProvider::class);

In case you use the second version, it is assumed that the service provider is not taking any argument for its constructor. The service provider is created only if needed (i.e. only if no cache or if you fetch an entry whose factory is not a public static method).

I guess it's as far as I can go for a proof of concept.

What do you think? If you are interested to go this way, I can change the service locator Registry to be compatible with PHP 5.4+. I've noticed it is useful in almost all implementations of service providers in advanced container (I'm using it in Symfony bridge and Yaco). Of course, you can also completely drop it and write your own "service locator registry" in order not to depend on too many external packages. Anyway, I'd be interested to know what you think about this. Convinced? Shall we move forward with container-interop/service-provider#20 ?

@mnapoli

This comment has been minimized.

Copy link
Member

commented May 29, 2016

Thanks! I've continued working on this to get up to date with container-interop/service-provider#20 I first started off your PR but I don't think it's worth having the "service provider registry" in this case. Most definitions will not be cacheable (closures, methods, etc.), so we'll need the service provider instances most of the time, and the cost of creating such instances will probably not be very high (compared to the cost of having the whole registry). Additionally I want to avoid adding new dependencies when it can be avoided. So I went a simpler way (allow adding ServiceProvider instances only, no class name), at least for now and we'll see. Now that 0.3.0 is supported in PHP-DI I'll try to update all the other packages.

@mnapoli mnapoli closed this May 29, 2016
@moufmouf

This comment has been minimized.

Copy link
Author

commented May 30, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.