Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DriftPHP goes Symfony Flex #9

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

DriftPHP goes Symfony Flex #9

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8fb7d11
merged symfony-flex and driftphp project structure
Basster Jul 4, 2020
cf16879
#9: make controller tagging optional.
Basster Jul 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,14 @@
vendor
.php_cs.cache
composer.lock
var
var

###> symfony/framework-bundle ###
/.env.local
/.env.local.php
/.env.*.local
/Drift/config/secrets/prod/prod.decrypt.private.php
/public/bundles/
/var/
/vendor/
###< symfony/framework-bundle ###
14 changes: 12 additions & 2 deletions Drift/Kernel.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ class Kernel extends AsyncKernel
{
use MicroKernelTrait;

private const CONFIG_EXTS = '.{php,xml,yaml,yml}';
Copy link
Member

Choose a reason for hiding this comment

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

  • 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.

Copy link
Author

Choose a reason for hiding this comment

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

  • 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.

Copy link
Member

Choose a reason for hiding this comment

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

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


/**
* @return iterable
*/
Expand Down Expand Up @@ -68,7 +70,12 @@ protected function configureContainer(ContainerBuilder $container, LoaderInterfa
{
$confDir = $this->getApplicationLayerDir().'/config';
$container->setParameter('container.dumper.inline_class_loader', true);
$loader->load($confDir.'/services.yml');
$container->setParameter('container.dumper.inline_factories', true);

$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');
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

}

/**
Expand All @@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.


$routes->import($confDir.'/{routes}/'.$this->environment.'/*'.self::CONFIG_EXTS, '/', 'glob');
$routes->import($confDir.'/{routes}/*'.self::CONFIG_EXTS, '/', 'glob');
$routes->import($confDir.'/{routes}'.self::CONFIG_EXTS, '/', 'glob');
}
}
2 changes: 1 addition & 1 deletion Drift/config/bootstrap.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

use Symfony\Component\Dotenv\Dotenv;

$appDir = dirname(__FILE__).'/../..';
$appDir = __DIR__ .'/../..';
Basster marked this conversation as resolved.
Show resolved Hide resolved
require $appDir.'/vendor/autoload.php';

if (is_file($appDir.'/.env')) {
Expand Down
14 changes: 14 additions & 0 deletions Drift/config/packages/driftphp.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
parameters:

services:
# default configuration for services in *this* file
_defaults:
autowire: true # Automatically injects dependencies in your services.
autoconfigure: true # Automatically registers your services as commands, event subscribers, etc.

Domain\:
resource: "%kernel.project_dir%/src/Domain/*"

Infrastructure\:
resource: "%kernel.project_dir%/src/Infrastructure/*"

8 changes: 8 additions & 0 deletions Drift/config/packages/framework.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
framework:
secret: '%env(APP_SECRET)%'
form: false
assets: false
session: false
translator: false
php_errors:
log: false
3 changes: 3 additions & 0 deletions Drift/config/packages/prod/routing.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
framework:
router:
strict_requirements: null
3 changes: 3 additions & 0 deletions Drift/config/packages/routing.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
framework:
router:
utf8: true
2 changes: 2 additions & 0 deletions Drift/config/packages/test/framework.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
framework:
test: true
0 Drift/config/routes.yml → Drift/config/routes.yaml
View file
Open in desktop
File renamed without changes.
Empty file added Drift/config/routes/.gitignore
View file
Open in desktop
Empty file.
20 changes: 20 additions & 0 deletions Drift/config/services.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
parameters:
app_path: "../.."

# config/services.yaml
services:
_defaults:
autowire: true
autoconfigure: true

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

@Basster Basster Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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.

#
# If you want to provide HTTP Endpoints, include the following part to enable service argument injection
# and make controllers public.
#
# App\Controller\:
# resource : "%kernel.project_dir%/src/Controller/*"
# tags:
# - {name: controller.service_arguments}
26 changes: 0 additions & 26 deletions Drift/config/services.yml
View file
Open in desktop

This file was deleted.

70 changes: 39 additions & 31 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,41 @@
{
"name": "drift/skeleton",
"description": "Skeleton repository ofr DriftPHP",
"type": "project",
"license": "MIT",
"authors": [
{
"name": "Marc Morera",
"email": "[email protected]"
}
],
"require": {
"php": "^7.3",
"symfony/console": "^5.0.0",
"symfony/framework-bundle": "^5.0.0",
"symfony/dotenv": "^5.0.0",
"symfony/stopwatch": "^5.0.0",
"symfony/debug": "^4.4.0",

"drift/http-kernel": "0.1.*, >=0.1.7",
"drift/server": "0.1.*, >=0.1.7",
"drift/react-functions": "0.1.*",
"drift/event-loop-utils": "0.1.*"
},
"autoload": {
"psr-4": {
"App\\": "src/",
"Domain\\": "src/Domain",
"Infrastructure\\": "src",
"Drift\\": "Drift/"
}
"name": "drift/skeleton",
"description": "Skeleton repository ofr DriftPHP",
"type": "project",
"license": "MIT",
"authors": [
{
"name": "Marc Morera",
"email": "[email protected]"
}
}
],
"require": {
"php": "^7.3",
"symfony/console": "^5.0.0",
"symfony/framework-bundle": "^5.0.0",
"symfony/dotenv": "^5.0.0",
"symfony/stopwatch": "^5.0.0",
"symfony/debug": "^4.4.0",
"drift/http-kernel": "0.1.*, >=0.1.7",
"drift/server": "0.1.*, >=0.1.7",
"drift/react-functions": "0.1.*",
"drift/event-loop-utils": "0.1.*",
"symfony/flex": "^1.8"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

},
"autoload": {
"psr-4": {
"App\\": "src/",
"Domain\\": "src/Domain",
"Infrastructure\\": "src/Infrastructure",
"Drift\\": "Drift/"
}
},
"scripts": {
"server": "server run 0.0.0.0:8000",
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

"watcher": "server watch 0.0.0.0:8000 --debug --env=dev"
},
"extra": {
"src-dir": "Drift",
"config-dir": "Drift/config"
}
}
51 changes: 50 additions & 1 deletion composer.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Empty file added src/Infrastructure/.gitignore
View file
Open in desktop
Empty file.
Loading