DriftPHP goes Symfony Flex #9

Basster · 2020-07-04T08:51:16Z

Since Symfony-Flex is the defacto standard for new projects since 4.something, I tried to adopt the structure with the DriftPHP Skeleton. I think it merges fine.

it also adds two conveniece composer scripts, to start the server/watcher.

What do you think?

mmoreram · 2020-07-04T10:13:29Z

Hello @Basster

First of all, thanks so much for that PR.
The means that you've been playing for a while with DriftPHP, and this means a lot :)

I've added many points in this PR. You've been so brave with this PR, and opens so many (philosophical) discussion points, what makes sense and seems the right moment to do that.

Remember that DriftPHP aims to use Symfony components and some parts of the application structure, but aims as well to be an independent framework on top of simplicity and architecture decisions.

What do you think?
Thanks!

Drift/config/bootstrap.php

mmoreram · 2020-07-04T10:01:36Z

composer.json

+    "drift/react-functions": "0.1.*",
+    "drift/event-loop-utils": "0.1.*",
+    "symfony/flex": "^1.8"


TBH, I don't like flex. It introduces an insane dependency to Symfony environment in terms of the project, and DriftPHP should only use some Symfony components, but be completely detached from the project itself.

I've been working without Flex for the last years, and TBH, I didn't even notice it at all.

Flex follows the SoC concept on the config layer and is the standard way of creating Symfony apps since 5.x, so you should adapt it.

symfony.lock

mmoreram · 2020-07-04T10:02:48Z

composer.json

+  },
+  "scripts": {
+    "server": "server run 0.0.0.0:8000",


Did this work in your local? I mean. How could this work? server run is not a kernel-loaded command, but an isolated one. Same for watcher.

Quote [https://getcomposer.org/doc/articles/scripts.md#writing-custom-commands](composer docs):

Before executing scripts, Composer's bin-dir is temporarily pushed on top of the PATH environment variable so that binaries of dependencies are easily accessible. In this example no matter if the phpunit binary is actually in vendor/bin/phpunit or bin/phpunit it will be found and executed.

So this works perfectly on each environment, since its only a convenient way of executing scripts in vendor/bin

Let me try again. The last time it didn't work properly for me.

mmoreram · 2020-07-04T10:04:22Z

Drift/config/services.yaml

+
+    App\:
+        resource: '%kernel.project_dir%/src/*'
+        exclude: '%kernel.project_dir%/src/{DependencyInjection,Entity,Migrations,Tests}'


This block doesn't make sense, as including "everything" is never a good decision. I turned autowiring friendly as I discovered that it decreases so much the code and the configuration spent time, but I'm still against the initial skeleton preset configuration.

Its (again) taken from the official Symfony Skeleton.

The way the dependency injection container compiler works it is perfectly fine to include everything, because each service which is not injected via DI and is not public will be deleted from the container either way. This costs only some milliseconds during container warmup and doesn't effect runtime at all, but it adds a lot of DX, because using DI requires no additional configuration, most of the time.

I'm still against the initial skeleton preset configuration

You will end up with github issues like "why is my service not injected", because devs are so used to it.

Drift/config/services.yaml

mmoreram · 2020-07-04T10:06:48Z

Drift/Kernel.php

+        $loader->load($confDir.'/{packages}/*'.self::CONFIG_EXTS, 'glob');
+        $loader->load($confDir.'/{packages}/'.$this->environment.'/*'.self::CONFIG_EXTS, 'glob');
+        $loader->load($confDir.'/{services}'.self::CONFIG_EXTS, 'glob');
+        $loader->load($confDir.'/{services}_'.$this->environment.self::CONFIG_EXTS, 'glob');


Let me start a discussion here.

Symfony turned too magical in this part, and this code block opens the door to many different ways to work in the same project with the same result. This is something I'd like to avoid.

Hmmm... I like the way Symfony supports Separation of Concerns even on the configuration layer. It gives a lot of DX and makes things more clear. I hated the times with 1k+ LoC in services.yml.

Okay, no ReactPHP/DriftPHP app should grow that much, but it still helps giving a better overview.

mmoreram · 2020-07-04T10:09:35Z

Drift/Kernel.php

@@ -29,6 +29,8 @@ class Kernel extends AsyncKernel
 {
    use MicroKernelTrait;

+    private const CONFIG_EXTS = '.{php,xml,yaml,yml}';


yaml === yml, so I'd go for only yml.

Almost no one configure services using PHP.

I'm an XML hater, and as a user and a programmer, I think that almost everyone understands YML at first sight, while in XML you must make an extra effort to split between the "struicture definition" part and the content part.

Developers are used to go with both, yml and yaml (e.g. k8s strictly uses yaml, while gitlab only supported yml for a long time), so I'd support both of them.

I'm also not a friend of xml config in userland (other than while developing libraries), but some devs prefer xml, because of its validation possibilities, so I'd stick with it.

Me personally uses .php config for some runtime configuration in a bigger project, so I'd also stick with it.

Generally these are the config formats each Symfony Dev expects to be supported from a framework using/inheriting Symfony, so I'd follow this part.

Looks like Symfony is going to use PHP configs in the future. 🤔
symfony/symfony#37186

mmoreram · 2020-07-04T10:11:09Z

Drift/Kernel.php

@@ -79,6 +86,9 @@ protected function configureContainer(ContainerBuilder $container, LoaderInterfa
    protected function configureRoutes(RouteCollectionBuilder $routes): void
    {
        $confDir = $this->getApplicationLayerDir().'/config';
-        $routes->import($confDir.'/routes.yml');


Another discussion here.

Multiple routes depending on the environment can be achieved in a less magical way by using different bundles (contexts). From an architectural perspective, including testing and extension, this makes more sense.

Same as with the services/config SoC: Still unsure whether a reactive app should grow that much, but I would follow the parents framework path here.

Basster · 2020-07-06T10:43:40Z

Maybe before going into detail of your review, I want to tell my side of the story:

You started developing a framework on top of another framework, which not only uses its components, but (literally) inherits from its core/kernel. Therefore I'd follow the principles of the parent framework, either way I dislike some of its concepts personally.

Symfony Framework gives a lot of freedom to solve things in different ways and that is one of its main strengths comparing it with some of its competitors.

As you require symfony/framework-bundle: ^5.0 DriftPHP should follow the same principles as its realtive. So mostly I've picked stuff from the official symfony skeleton and placed it here. I'm going to give some examples on each of your comments now.

mmoreram · 2020-07-06T11:00:15Z

@Basster

You started developing a framework on top of another framework, which not only uses its components, but (literally) inherits from its core/kernel.

Yes. The core/kernel is a Symfony component, not the framework itself. In this case, the kernel is extending the Symfony one for back compatibility purposes, but the new (and only used) handleAsync method rewrite the whole kernel.

Symfony Framework gives a lot of freedom to solve things in different ways and that is one of its main strengths comparing it with some of its competitors.

Sometimes freedom means caos, and this is one of the things I would like to "change" in this project. I'd like to have one and only one way of solving one problem. And about the competitors thing. The Symfony problem is precisely this. Everything is about competitors. Not my style.

As you require symfony/framework-bundle: ^5.0 DriftPHP should follow the same principles as its realtive.

Why? I'm proposing a different way to solve exactly the same.

mmoreram · 2020-07-06T11:13:33Z

To be clear, I'd like to take a decision on top of people feedback.
My opinion will be another opinion.

Basster · 2020-07-06T11:17:02Z

Another personal note:

I found DriftPHP and gave it a try, because it tries to merge concepts from Symfony and ReactPHP and beacuse I found familiar patterns to have an easy start.

Why? I'm proposing a different way to solve exactly the same.

Proposing is fine but forcing devs to follow one single path mostly ends up in rejection, especially if they are still used to the concepts.

Maybe to explain it with the liskov substitution principle, I would expect DriftPHP to behave like Symfony. DriftPHP on the other hand provides more/other values than its ancestor.

That's just my 2 cents.

seregazhuk · 2020-07-07T13:23:13Z

I think that we misunderstand each other.

Let me explain my position. As I understood the idea behind Drift, that it is not like "async Symfony" but a stand-alone framework on top of ReactPHP and Symfony components. Like Laravel for example. Yes, it seems like it repeats the structure of Symfony (configs, routes) and thus it feels like it should follow Symfony in everything. But I think it was done just for convenience. To show people that they can easily migrate from Symfony to Drift without "rewriting everything".

Talking about OOP: DriftPHP doesn't inherit Symfony and ReactPHP. It is more about composition here.

Basster · 2020-07-16T19:55:59Z

Hi everyone.

after @seregazhuk comment I see a little clearer and I didn't want to sound so harsh. Sorry about that. I really, really like the idea of DriftPHP and maybe I'm into Symfony for so long, so maybe I just saw something like "async Symfony" in it.

I think we can close this for now. Let me know if I can help you out with anything else.

seregazhuk · 2020-07-17T07:41:26Z

@Basster Thank you for understanding! I'm glad that now we are on the same page. We really appreciate the work you did! 💪

mmoreram · 2020-07-17T09:38:39Z

@Basster to be clear, I don't think you were harsh at all. Open source doesn't mean we have to add the word "please", "thanks" and "IMHO" in every sentence, so if your arguments were for the only for good purposes in terms of the project, the is a good thing :)

Said that, I think that this discussion is not empty at all. I've been the (lonely) lord of the project during months and every single step in the framework has been done because of my perspective. I defend what is my point of view, but each time I need to consider it because of having another point of view in front of me, that enriches me so much.

What I mean is that even I think that @seregazhuk is right, I don't think you're not.

I've always defended that in a framework where software, architecture, and performance are the 3 principal blocks to consider in each PR. For me, the easiest way is not always the best, no matter how many people will just go away from this project because their laziness. Basically because with this philosophy, people that already make an effort to understand a new way is completely abandoned.

Please, consider reopening this PR again. We have time for discussion, and I'm sure more people will have their 5 cents.

Thanks for your work.

Basster · 2020-07-17T13:06:14Z

Thank you for sharing your perspectives, therefore I'll /reopen it to keep the discussion alive.

merged symfony-flex and driftphp project structure

8fb7d11

mmoreram requested changes Jul 4, 2020

View reviewed changes

driftphp#9: make controller tagging optional.

cf16879

Basster closed this Jul 16, 2020

Basster reopened this Jul 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DriftPHP goes Symfony Flex #9

DriftPHP goes Symfony Flex #9

Basster commented Jul 4, 2020

mmoreram commented Jul 4, 2020

mmoreram Jul 4, 2020

Basster Jul 6, 2020

mmoreram Jul 4, 2020

Basster Jul 6, 2020

mmoreram Jul 6, 2020

mmoreram Jul 4, 2020

Basster Jul 6, 2020 •

edited

Loading

mmoreram Jul 4, 2020

Basster Jul 6, 2020

mmoreram Jul 4, 2020

Basster Jul 6, 2020

seregazhuk Jul 7, 2020

mmoreram Jul 4, 2020

Basster Jul 6, 2020

Basster commented Jul 6, 2020

mmoreram commented Jul 6, 2020

mmoreram commented Jul 6, 2020

Basster commented Jul 6, 2020

seregazhuk commented Jul 7, 2020

Basster commented Jul 16, 2020

seregazhuk commented Jul 17, 2020

mmoreram commented Jul 17, 2020

Basster commented Jul 17, 2020

DriftPHP goes Symfony Flex #9

Are you sure you want to change the base?

DriftPHP goes Symfony Flex #9

Conversation

Basster commented Jul 4, 2020

mmoreram commented Jul 4, 2020

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

Basster Jul 6, 2020 • edited Loading

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

Basster commented Jul 6, 2020

mmoreram commented Jul 6, 2020

mmoreram commented Jul 6, 2020

Basster commented Jul 6, 2020

seregazhuk commented Jul 7, 2020

Basster commented Jul 16, 2020

seregazhuk commented Jul 17, 2020

mmoreram commented Jul 17, 2020

Basster commented Jul 17, 2020

Basster Jul 6, 2020 •

edited

Loading