Add URI factory (fix #3) #4

ddeboer · 2015-06-05T22:00:12Z

Fix #3.

Also refactored a bit to reduce code duplication.

sagikazarmark · 2015-06-05T22:03:12Z

src/ClassDiscovery.php

+ *
+ * @author David de Boer <david@ddeboer.nl>
+ */
+abstract class ClassDiscovery implements Discovery


I would rather call it FactoryDiscover, it tells more about it's purpose IMO.

True, but I thought we could make HttpAdapterDiscovery extend the abstract class too.

This isn't too much code that complexity worths avoiding duplication. The guess condition logic is a bit different there.

I already took that into account in the abstract class: with or without factory. Not much complexity, no, but it’s nice to not duplicate the caching/resetting.

Fine, but in that case the parameters should always be the same. See comment bellow.

ddeboer · 2015-06-05T22:24:15Z

src/HttpAdapterDiscovery.php

+    protected static $classes = [
+        'guzzle6' => [
+            'condition' => 'Http\Adapter\Guzzle6HttpAdapter',
+            'class' => 'Http\Adapter\Guzzle6HttpAdapter'


The class name is duplicated, but I like that it is explicit about that it’s going to return the same class as it finds.

No objections.

ddeboer · 2015-06-05T22:24:49Z

Updated.

sagikazarmark · 2015-06-05T22:25:08Z

src/ClassDiscovery.php

+     */
+    protected static $classes = [];
+
+    /**


They are not factories anymore, are they? 😃

sagikazarmark · 2015-06-05T22:27:18Z

src/ClassDiscovery.php

+     * @param string $condition
+     * @param string $class
+     */
+    public static function register($name, $condition, $class = null)


I would change the order of arguments: the first is the class, which is always required. The second is the condition. Even in factory guessers, it is acceptable to pass the condition as a second argument. Again, just for the sake of consistency.

So $class, $condition = null? How would that work for

'condition'=> 'GuzzleHttp\Psr7\Request', 'class' => 'Http\Discovery\MessageFactory\GuzzleFactory',

Yup, that seems more logical to me.

By the way, $name doesn’t really add much. A priority would be better. But that’s for another PR. 😉

Not sure if priority matters either. It is very unlikely you have two adapters/message implementations installed.

To answer your question: two possible way I can see: make condition required argument or check it in cases when required.

For the long term, i would convert the condition to something that can be evaluated as boolean. So we could accept a callable or boolean as well. In that case, null is obviously false.

sagikazarmark · 2015-06-05T22:35:40Z

src/ClassDiscovery.php

+     * Cached class
+     *
+     * @var object
+     */


Be aware, that in this case tha cache property will be the same for all child classes. You either have to add a cache property to all discovery objects or handle this case in the parent, like saving the cache in an array based on get_called_class.

To be clear: when a static property is declared in the parent, but not the child then setting it in the child will set it in the parent, since there is no context.

Yeah, I overlooked that. Would be a nice bug. 😊 Let’s add it to the children.

sagikazarmark · 2015-06-05T22:47:15Z

src/HttpAdapterDiscovery.php

-        }
-
-        throw new NotFoundException('No HTTP Adapter found');
+        return parent::find();


Are these methods necessary? For the sake of return type?

Necessary, no. But (as you may have noticed elsewhere) I value IDE auto-completion very much. Previously they had a @return object which stops your auto-complete chain.

Fine. 😃 But in that case, add a comment so that I dont remove it as auto optimalization. 😃

ddeboer · 2015-06-05T23:04:10Z

Okay, should be ready now.

sagikazarmark · 2015-06-05T23:08:07Z

Thanks

Add URI factory (fix #3)

ddeboer · 2015-06-05T23:16:29Z

You too, for the thorough review. 😄