Remove container dependency #13

jaapio · 2017-01-23T20:56:40Z

Client was leaking the applications container. When needed the
container can inject itself into handlers. There is no need to have this
in the client.

Beside that this pr removes the hard connection with php-di, which allows the user of the package to choose there own DIC. Without having multiple DIC's in your application.

jaapio · 2017-01-23T20:57:58Z

src/Factory.php

@@ -19,12 +19,13 @@ public static function create(
        LoopInterface $loop,
        array $options = []
    ): Client {
+        $container = self::createContainer($loop, $options);
        return new Client(


Do we actually need this method? This factory should return the commandbus, not the client?

Actually returning the command bus instead of the client makes sense. Clients should using it building their client and abstract all operations into it 👍 .

Ignore this comment for now. It would be something for a follow up PR, like to explore the idea though.

WyriHaximus · 2017-01-23T21:44:42Z

This makes a lot of sense. I needed the leak at some point, will look it up, test this PR with that client before I respond. But while skimming over it this looks good 👍

WyriHaximus

This is a great step in the right direction, but this PR as of right now has some contradictions.

WyriHaximus · 2017-01-24T07:00:02Z

src/Factory.php

        );
    }

-    private static function createContainer(LoopInterface $loop, array $options): ContainerInterface
+    public static function createContainer(LoopInterface $loop, array $options): ContainerInterface
    {
        $container = new ContainerBuilder();


While you removed PHP-DI from dependencies this ContainerBuilder is still a concrete class from PHP-DI.

WyriHaximus · 2017-01-24T07:03:19Z

composer.json

@@ -15,13 +15,14 @@
    "api-clients/hydrator": "dev-master",
    "api-clients/service": "dev-master",
    "api-clients/transport": "dev-master",
-    "league/event": "^2.1",
-    "php-di/php-di": "^5.4"


Not 100% sure if we should do this. The foundation providers a container for the application/client, and patching the leakage should hide it from the application. If we want to give applications access to adding definitions we could use the same kind of construction as we do with the command bus, some extra configuration in composer.json: https://github.com/php-api-clients/github/blob/master/composer.json#L38-L45

(No need to do that in this PR though 😄 .)

jaapio · 2017-01-25T14:03:43Z

What I intended to do was removing PHP-DI as dependency. Since that was only needed in this factory. Most applications will use there own DIC, so removing this dependency was needed to not have multiple DICs in one application.

To accomplish this I removed the getters from the Client that where never used by any of the projects in this organisation. So I thought it was safe to remove them. But I didn't want to break your Factory. So I left the dependency as is here.

The ContainerInterface only specifies the get and has methods and no way to configure containers. So when you want to use an other DI package, you will have to write your own factory. The whole createContainer method is not usable in this case.

Maybe we should do a step back to have a look at this implementation and why it is needed. When developing a api-client library that's meant to be used in an application. So from my point of view it should only have the dependencies that are required to get the client working.
All libraries will implement an AsyncClient, that needs an implementation of ApiClients\Foundation\ClientInterface. Basically that's just a wrapper around the CommandBus. But to hide that from the implementation it feels good to have this implementation.

The CommandBus itself doesn't need the ContainerInterface but the middelware (CommandHandlerMiddleware) that's injected does.
Same thing for Transport and Hydrators.

There is a lot of magic going on in the implementations of Transport and Hydrators to fetch their dependencies. I would like to make this more explicit. I tried to do that with this first step. And when I was writing this comment I saw why I feld it this way. It looks like the ContainerInterface is everywhere, And there is a hard binding to PHP-DI or an equal DIC. There is no way to use a ServiceLocator like Zend-ServiceManager for example because to much magic is involved.

WyriHaximus · 2017-01-25T22:02:19Z

[...]

There is a lot of magic going on in the implementations of Transport and Hydrators to fetch their dependencies. I would like to make this more explicit.

Absolutely 👍 ! Things have come a long way since I've started working on this project 6+ months ago and some of it still holds legacy from that long lost past. I'm glad to see you're picking these issues up and looking at it with a fresh pair of eyes 😄 .

Would the application be injecting it's DIC/Service Locator into a client or would the client hold it's own private internal DIC/Service Locator? We could do both where we look into this PHP-DI feature combining a passed in container for extra entries and our own for hydrator, transport, etc. We still ensure all components type hint against the container interop interface so they don't care.

Could you make issues/PR's on the different components describing your ideas or work them out in a PR and reference between them? Might be easier discussing changes that way.

P.S. As far a breaking things goes, we're still in dev-master if a break makes sense lets break it. If we're in 0.x we go to 0.y. If we're in stable releases we break and tag another major release. Especially since non of the clients have been tagged yet 😄 . The only thing to keep in mind is to coordinate breaking PR's with updating all other packages relying and influenced but that change.

jaapio · 2018-01-09T20:32:24Z

Anything I can do to have this merged?

WyriHaximus · 2018-01-09T20:41:01Z

Just skimmed over it and for some reason it made a lot of sense now. Not sure what I was missing last year 🤐 . Can you resolve the conflicts with master?

Client was leaking the applications container. When needed the container can inject itself into handlers. There is no need to have this in the client.

jaapio · 2018-01-09T21:05:03Z

I rebased it, but it was way to long ago to remember what I was doing that the moment of this pr :-( so I hope that this fits your needs

WyriHaximus · 2018-01-09T21:21:49Z

Yeah there are a few different things happening. But the main thing for me, is to remove the container from injecting it self into the Client. Will request a few changes to make that the main goal for the PR if you agree?

jaapio · 2018-01-09T21:34:30Z

👍 will do the rework after your review.

WyriHaximus · 2018-01-09T21:41:52Z

composer.json

@@ -14,7 +14,6 @@
    "api-clients/hydrator": "^1.0.1",
    "api-clients/middleware": "^4.0 || ^3.0 || ^2.0 || ^1.1",
    "api-clients/transport": "^3.0 || ^2.0 || ^1.0",
-    "php-di/php-di": "^5.4.3",


Can you revert this, PHP-DI was specifically picked for the autowiring of dependencies for services and handlers used throughout the entire @php-api-clients project. External definitions are supported through https://github.com/php-api-clients/foundation/blob/master/src/Factory.php#L53 but I would like to support wrappoing an external PSR-11 container in the future.

WyriHaximus · 2018-01-09T21:43:18Z

src/ClientInterface.php

-    /**
-     * @return mixed
-     */
-    public function getFromContainer(string $id);


This is only used in two clients and from I've gathered it can easily updated after this fix 👍

WyriHaximus · 2018-01-09T21:44:19Z

src/Factory.php

@@ -19,12 +19,13 @@ public static function create(
        LoopInterface $loop,
        array $options = []
    ): Client {
+        $container = self::createContainer($loop, $options);
        return new Client(


Ignore this comment for now. It would be something for a follow up PR, like to explore the idea though.

WyriHaximus · 2018-01-09T21:45:46Z

tests/FactoryTest.php

    {
        $loop = LoopFactory::create();

        $stdClass = new \stdClass();
        $stdClass->foo = 'bar';

-        $client = Factory::create(
+        $container = Factory::createContainer(


Doesn't this blow up since Factory::createContainer is private?

jaapio commented Jan 23, 2017

View reviewed changes

WyriHaximus requested changes Jan 24, 2017

View reviewed changes

jaapio added 2 commits January 9, 2018 21:51

Remove php-di as hard dependency

3012b77

jaapio force-pushed the remove-container-dependency branch from 6765bad to 3012b77 Compare January 9, 2018 21:02

WyriHaximus requested changes Jan 9, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove container dependency #13

Remove container dependency #13

jaapio commented Jan 23, 2017

jaapio Jan 23, 2017

WyriHaximus Jan 23, 2017

WyriHaximus Jan 9, 2018

WyriHaximus commented Jan 23, 2017

WyriHaximus left a comment

WyriHaximus Jan 24, 2017

WyriHaximus Jan 24, 2017

jaapio commented Jan 25, 2017

WyriHaximus commented Jan 25, 2017

jaapio commented Jan 9, 2018

WyriHaximus commented Jan 9, 2018

jaapio commented Jan 9, 2018

WyriHaximus commented Jan 9, 2018

jaapio commented Jan 9, 2018

WyriHaximus Jan 9, 2018

WyriHaximus Jan 9, 2018

WyriHaximus Jan 9, 2018

WyriHaximus Jan 9, 2018

Remove container dependency #13

Are you sure you want to change the base?

Remove container dependency #13

Conversation

jaapio commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WyriHaximus commented Jan 23, 2017

WyriHaximus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaapio commented Jan 25, 2017

WyriHaximus commented Jan 25, 2017

jaapio commented Jan 9, 2018

WyriHaximus commented Jan 9, 2018

jaapio commented Jan 9, 2018

WyriHaximus commented Jan 9, 2018

jaapio commented Jan 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment