Improvements/#1 #32

lucianoqueiroz · 2017-03-04T21:38:37Z

related with #31 .

malukenho

Good job 👏

malukenho · 2017-03-05T01:07:19Z

config/services.php

 use Prooph\EventStore\EventStore;
 use Prooph\ServiceBus\CommandBus;
 use Zend\Expressive\Application;
 use Zend\Expressive\Router\FastRouteRouter;

+


remove this new line

malukenho · 2017-03-05T01:09:17Z

config/services.php

-            },
-
+            Application::class     => ApplicationFactory::class,
+            FastRouteRouter::class => RouterFactory::class,


FastRouter doesn't need a specialised class for factory, I guess you can use the InvokableFactory::class in this situations.

malukenho · 2017-03-05T01:09:52Z

config/services.php

@@ -45,7 +31,7 @@
                return 'root';
            },
            'db_password' => function () {
-                return 'root';
+                return null;


It need to be changed, this values should come from environment variables

malukenho · 2017-03-05T01:10:24Z

src/Conference/Domain/Command/CreateConference.php

+        \DateTimeImmutable $date
+    ): self {
+        return new self(
+            $conferenceId,


Inline arguments

malukenho · 2017-03-05T01:10:39Z

config/commands.php

@@ -0,0 +1,14 @@
+<?php
+


Missing docheader

malukenho · 2017-03-05T01:11:47Z

src/Conference/Infrastructure/Service/RouterFactory.php

+/**
+ * @author Luciano Queiroz <luciiano.queiroz@gmail.com>
+ */
+final class RouterFactory


Kill this class as mentioned above

malukenho · 2017-03-05T01:11:54Z

src/Conference/Infrastructure/Service/RouterFactory.php

+    {
+        return new FastRouteRouter();
+    }
+}


Missing EOL EOF

malukenho · 2017-03-05T01:12:09Z

src/Conference/Infrastructure/Service/RouterFactory.php

+
+use Interop\Container\ContainerInterface;
+use Zend\Expressive\Router\FastRouteRouter;
+


Too much empty lines

malukenho · 2017-03-05T01:13:34Z

config/services.php

@@ -45,7 +31,7 @@
                return 'root';


This values should come from environment variables,

malukenho · 2017-03-05T01:13:44Z

config/services.php

-
-            // @todo move repository to another file
-            ConferenceRepositoryInterface::class => ConferenceRepositoryFactory::class,
-
            // @todo move db info to a class to get ENV vars
            'db_dsn' => function () {
                return 'mysql:host=localhost;dbname=conticket';


This values should come from environment variables

malukenho · 2017-03-05T01:19:41Z

src/Conference/Infrastructure/Service/ApplicationFactory.php

+/**
+ * @author Luciano Queiroz <luciiano.queiroz@gmail.com>
+ */
+final class ApplicationFactory


implements FactoryInterface

Which FactoryInterface should be implemented? I didn't find one that matches our factories implementation

This one: https://github.com/zendframework/zend-servicemanager/blob/master/src/Factory/FactoryInterface.php

I just want to avoid the maximum of duck type on code.

perfect! 👍

Also, I'm thinking about drop container-interop/container-interop and use just PSR-11 instead. Not sure yet, seems like it'll be possible only when using SM4.0

I think that's a good call, since https://github.com/container-interop/container-interop#deprecation-warning

@lucianoqueiroz we can do that in subsequent PR, It's getting huge man. Is it done on your side? can I merge it?

yes, you can

…dotenv package

malukenho · 2017-03-07T04:15:20Z

composer.json

@@ -35,7 +35,8 @@
    "ramsey/uuid": "^2.8",
    "zendframework/zend-expressive": "^1.0",
    "zendframework/zend-expressive-fastroute": "^1.0",
-    "zendframework/zend-servicemanager": "^3.2"
+    "zendframework/zend-servicemanager": "^3.2",
+    "vlucas/phpdotenv": "^2.4"


--sort-packages

malukenho · 2017-03-07T04:16:59Z

config/service-manager.php

+            require __DIR__ . '/commands.php',
+            require __DIR__ . '/middlewares.php',
+            require __DIR__ . '/repositories.php',
+            require __DIR__ . '/services.php'


Seems like you created a new line and deleted services.php here.
Stay with services.php at the top so the diff will be not affected by this.

malukenho · 2017-03-07T04:17:29Z

config/services.php

-            },
-
+            Application::class     => ApplicationFactory::class,
+            FastRouteRouter::class => InvokableFactory::class,


align the rest of the file to =>

malukenho · 2017-03-07T04:18:03Z

public/index.php

@@ -7,6 +7,9 @@
 (function () {
    require __DIR__ . '/../vendor/autoload.php';

+    /* loading .env variables */


Unneeded comment.

malukenho · 2017-03-07T04:18:17Z

public/index.php

@@ -7,6 +7,9 @@
 (function () {
    require __DIR__ . '/../vendor/autoload.php';

+    /* loading .env variables */
+    (new \Dotenv\Dotenv(__DIR__ . '/..'))->load();


Can't we use $app->pipe()?

I didn't get it... Are you saying to load the .env variables via a middleware and pipe it to run every request?

malukenho · 2017-03-07T04:21:25Z

src/Conference/Infrastructure/Service/ConnectionFactory.php

-                    $container->get('db_dsn'),
-                    $container->get('db_user'),
-                    $container->get('db_password')
+                    getenv('DB_DSN'),


It SHALL still get this info from containers IMHO.

Register it under services

malukenho · 2017-03-08T02:20:16Z

config/services.php

-                return 'root';
-            },
+            Application::class      => ApplicationFactory::class,
+            FastRouteRouter::class  => InvokableFactory::class,


too much space

malukenho · 2017-03-08T16:03:02Z

Thank you @lucianoqueiroz 🍻

lucianoqueiroz added 4 commits March 4, 2017 18:34

moving logic from named constructor to private constructor

9c88345

fixing cs

e26062a

creating new config files and application factory

c2038cc

creating router factory

52b48a6

malukenho requested changes Mar 5, 2017

View reviewed changes

malukenho reviewed Mar 5, 2017

View reviewed changes

lucianoqueiroz added 4 commits March 5, 2017 09:35

removing router factory, fixing cs in application factory and adding …

cbb39f0

…dotenv package

adding docheader

3da5028

adding eof

edb6437

fixing middlewares config file

63d110a

malukenho requested changes Mar 7, 2017

View reviewed changes

malukenho assigned lucianoqueiroz Mar 7, 2017

lucianoqueiroz added 2 commits March 7, 2017 22:10

adding pdo factory

a4ffc9d

sorting packages

d2fb3cf

malukenho reviewed Mar 8, 2017

View reviewed changes

implementing factory interface in all service factories

5e382fe

malukenho merged commit b2e51bd into PHPeste:inhancement/#31 Mar 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements/#1 #32

Improvements/#1 #32

lucianoqueiroz commented Mar 4, 2017

malukenho left a comment

malukenho Mar 5, 2017

malukenho Mar 5, 2017

malukenho Mar 5, 2017

malukenho Mar 5, 2017

malukenho Mar 5, 2017

malukenho Mar 5, 2017

malukenho Mar 5, 2017

malukenho Mar 5, 2017

malukenho Mar 5, 2017

malukenho Mar 5, 2017

malukenho Mar 5, 2017

lucianoqueiroz Mar 5, 2017

malukenho Mar 8, 2017

lucianoqueiroz Mar 8, 2017

malukenho Mar 8, 2017

lucianoqueiroz Mar 8, 2017

malukenho Mar 8, 2017

lucianoqueiroz Mar 8, 2017

malukenho Mar 7, 2017

malukenho Mar 7, 2017

malukenho Mar 7, 2017

malukenho Mar 7, 2017

malukenho Mar 7, 2017

lucianoqueiroz Mar 8, 2017

malukenho Mar 7, 2017

malukenho Mar 8, 2017

malukenho commented Mar 8, 2017


		use Interop\Container\ContainerInterface;
		use Zend\Expressive\Router\FastRouteRouter;

Improvements/#1 #32

Improvements/#1 #32

Conversation

lucianoqueiroz commented Mar 4, 2017

malukenho left a comment

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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malukenho commented Mar 8, 2017