Flexible discovery #59

Nyholm · 2016-06-12T16:12:38Z

I was not too happy about #58. We treat Puli in a special manner which feels weird. Also, I was not too happy about the logic in the fallback strategies.

This PR continues the work and replaces #58. The user has now the possibility to select what strategies to use and also add some more custom if there is a need for it.

Changes

Introduced Stratiegies that do the discovery. We try the strategies one by one until we find what we're looking for
many public functions are moved from ClassDiscovery to Strategy\Puli.
I've also created some new exceptions.
Added a simple cache mechanism
When Puli is missing we will not throw a \RuntimeException anymore. There will be a NotFoundException which has previous exception so you can see that Puli was missing.
The NotFoundException is moved from Http\Discovery to Http\Discovery\Exception

TODO

This is just a proof of concept. Do we like to go this way?

Add tests
Update documentation

Nyholm · 2016-06-12T21:57:53Z

src/ClassDiscovery.php

+        $exceptions = [];
+        foreach (self::$strategies as $strategy) {
+            try {
+                $bindings = call_user_func($strategy.'::find', $type);


Is "binding" the best name here?

Puli terminology

Yeah, but Im hesitant over using "binding" since this is not in the Puli scope. I'll use "binding"

sagikazarmark · 2016-06-13T12:16:22Z

Huh, nice work. Haven't reviewed yet, but definitely will.

About strategies: not sure if we want discoveries to be made configurable. It's like creating a custom configuration layer which we don't want to.

I think the idea in the linked ticket was to use puli if available, fallback to our known resources if they are available. If this is what strategies intend to do (with a better abstraction) then it's fine, but we should keep it minimal: not too much extension points, etc

Nyholm · 2016-06-13T12:57:25Z

Thank you Mark.

I think the idea in the linked ticket was to use puli if available, fallback to our known resources if they are available.

Have a look at #58. It did that exactly. But it felt weird and it did not follow the Open/Close principle. But maybe you are correct, this is too many of extension points.

sagikazarmark · 2016-06-13T12:58:42Z

Discovery is for convenience, so I would rather follow KISS.

dbu · 2016-06-14T06:08:57Z

spec/ClassDiscoverySpec.php

@@ -47,7 +47,7 @@ function it_throws_an_exception_when_binding_not_found(Discovery $discovery)
    {
        $discovery->findBindings('InvalidBinding')->willReturn([]);

-        $this->shouldThrow('Http\Discovery\NotFoundException')->duringFindOneByType('InvalidBinding');
+        $this->shouldThrow('Http\Discovery\Exception\NotFoundException')->duringFindOneByType('InvalidBinding');


should we use NotFoundException::class here?

dbu · 2016-06-14T06:24:41Z

i like it. its indeed quite a thing you built here, but i think its clearly structured. i think there are too many public methods that are additional extension points we should not need. i think hard-setting the answer for a $type on the discovery itself should be all the configuration we allow. the whole discovery interface and implementations should have @internal annotations and never be interacted with. if you want to interact, have your application/library accept instantiated clients in addition to the discovery.

i also feel a bit unsure about the number of static methods. but as we don't want the discoveries to be configurable anyways, it should not matter.

Nyholm · 2016-06-14T07:34:56Z

FYI, If Puli is not installed AND we can't find a client:

For a client:

For a message factory:

dbu · 2016-06-15T06:42:46Z

i like this! the implementation looks good to me.

joelwurtz · 2016-06-15T10:48:05Z

src/Strategy/HttpClients.php

+            ['class' => Curl::class, 'condition' => Curl::class],
+            ['class' => Socket::class, 'condition' => Socket::class],
+            ['class' => Buzz::class, 'condition' => Buzz::class],
+            ['class' => React::class, 'condition' => React::class],


Not sure if this work when you have only one HttpClient and not the other since you will only have one class, the other will not be found and this will throw an autoloading exception.

That is the beauty of Foo::class, it doesn't throw an exception if the class not exists. =)

dbu · 2016-06-15T14:35:06Z

@Nyholm can you check the build failures please?

Nyholm · 2016-06-15T14:57:58Z

Yes of course. I started to fix the tests this morning but did not have time to finish.

dbu · 2016-06-16T11:14:38Z

sure, no hurry. just wanted to be sure we understand who is waiting on whom ;-)

Nyholm · 2016-06-16T14:53:14Z

Yooho. Tests are passing. I added a fix for the composer/puli thing.

Please feel free to make suggestions on the tests. PHPSpec is not my strongest suit.

sagikazarmark · 2016-06-24T16:32:33Z

spec/Helper/DiscoveryHelper.php

+    {
+        self::$classes = [];
+    }
+}


Missing newline

sagikazarmark · 2016-06-24T16:45:33Z

Let me see if I understand well: we have x strategies which all return some resources, which we cache and then use this cache to find the actual resource?

Nyholm · 2016-06-24T17:14:59Z

Thank you for your review.

We work with a strategy at the time, if a strategy return a "candidate" were its dependencies are met we have found a class and we will return without investigating further strategies. This candidate is saved to the cache. The cache is only read from the second time we ask for the same resource.

The great thing about the strategies is that we can easily support Puli2 or WhatveverDiscovery without editing any existing class.

sagikazarmark · 2016-06-24T19:58:45Z

src/ClassDiscovery.php

     */
-    private static $puliFactory;
+    public static $strategies = [


Do we need to configure the strategies in the abstract class? Can't we do it on a per discovery basis?

Yes, we are using strategies that we know would fail. Example; the HttpClients strategy will always fail for the FactoryDiscovery. I'll fix this by merging the strategies suggested bellow.

sagikazarmark · 2016-06-24T20:13:20Z

This is cool, I like it, great job @Nyholm.

I've added a few more comments besides the CS ones from earlier. Can you please take a look at them and at least answer them? If you have no time, I can do the improvements I suggested, but I need your comment to make sure I understood everything right. Thank you!

Nyholm · 2016-06-24T21:02:43Z

Thank you. I've address all your comments, either with a reply or a code change (or both).

sagikazarmark · 2016-06-24T21:04:30Z

This is still an open question from my POV: #59 (comment)

sagikazarmark · 2016-06-25T08:26:51Z

Great, I am happy with the current state. Can you please add some change log and squash?

Nyholm · 2016-06-25T09:13:09Z

Awesome. I'll must go afk now. Please feel free to make some changes or merge this when the tests passes.

Thank you for the reviews!

Nyholm force-pushed the flexible-discovery branch 2 times, most recently from a5b302c to 96a650b Compare June 12, 2016 17:05

This was referenced Jun 12, 2016

Make puli optional #55

Closed

Added fallback on common classes. Puli is now optional #58

Closed

Nyholm reviewed Jun 12, 2016
View reviewed changes

dbu reviewed Jun 14, 2016
View reviewed changes

joelwurtz reviewed Jun 15, 2016
View reviewed changes

sagikazarmark reviewed Jun 24, 2016
View reviewed changes

spec/Helper/DiscoveryHelper.php

{

self::$classes = [];

}

}

Copy link

Member

sagikazarmark Jun 24, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing newline

sagikazarmark reviewed Jun 24, 2016
View reviewed changes

Nyholm force-pushed the flexible-discovery branch from 7ca6695 to 1c37482 Compare June 25, 2016 09:07

Added strategies to make Puli optional

1c37482

sagikazarmark merged commit ff4c775 into master Jun 25, 2016

sagikazarmark deleted the flexible-discovery branch June 25, 2016 10:07

This was referenced Jun 25, 2016

Better exception message when Puli not found #54

Closed

Diactoros support #42

Closed

Nyholm mentioned this pull request Jul 16, 2016

Update discovery documentation php-http/documentation#105

Merged

Flexible discovery #59

Flexible discovery #59

Uh oh!

Conversation

Nyholm commented Jun 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

TODO

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Jun 13, 2016

Uh oh!

Nyholm commented Jun 13, 2016

Uh oh!

sagikazarmark commented Jun 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Jun 14, 2016

Uh oh!

Nyholm commented Jun 14, 2016

Uh oh!

dbu commented Jun 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbu commented Jun 15, 2016

Uh oh!

Nyholm commented Jun 15, 2016

Uh oh!

dbu commented Jun 16, 2016

Uh oh!

Nyholm commented Jun 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Jun 24, 2016

Uh oh!

Nyholm commented Jun 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagikazarmark commented Jun 24, 2016

Uh oh!

Nyholm commented Jun 24, 2016

Uh oh!

sagikazarmark commented Jun 24, 2016

Uh oh!

sagikazarmark commented Jun 25, 2016

Uh oh!

Nyholm commented Jun 25, 2016

Uh oh!

Uh oh!

Nyholm commented Jun 12, 2016 •

edited

Loading