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.

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

[RFC][Console][DI] Simplify adding DI container to CLI apps #27479

Open
PabloKowalczyk opened this Issue Jun 3, 2018 · 33 comments

Comments

Projects
None yet
9 participants
@PabloKowalczyk
Contributor

PabloKowalczyk commented Jun 3, 2018

Description
Hello, recently i've became the Crunz maintainer, after some review i've realized that Crunz doesn't use DI container - this means static calls and no way to write sensible test. In the meantime @TomasVotruba published some blog posts about adding DI container to CLI apps, as you can see there is no standard/recommended way to add DI to CLI app.
One approach is to copy some code from HttpKernel as i do in Crunz , but maintaining it is not nice nor easy. Nobody wants to reinvent the wheel.
Another approach, as suggested in above blog post, is to use Kernel provded by http-kernel package but i do not like this idea because it is weird to have full http support in CLI app :)

IMO there are three ways to increase DX and easy development of CLI apps with DI container:

  1. Introduce something like "cli-kernel" - like http-kernel (and framework-bundle) but for CLI. IMO this is the best way because developers can also have other features, out-of-the-box, like autoconfigure Symfony commands, easy access to container in test env and many others.
  2. Extend Symfony\Component\Console\Application to support DI natively. For example by decorator.
  3. Improve docs :) If you think that above options aren't good it would be very nice to at least add recommended way to glue DI and Console.

What do you think?
Thanks.

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Jun 3, 2018

Thanks for opening this 👍
CliKernel would be great.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 5, 2018

By using symfony/skeleton, you're already able to trivially create a DI-enabled console app. What's the issue this would solve that isn't solved using the skeleton?

@stof

This comment has been minimized.

Member

stof commented Jun 5, 2018

The issue by creating a cli-kernel parallel to http-kernel is that this would create a parallel bundle system to register third-party extension points. That would be much worse for the ecosystem than having the HTTP layer around in your CLI project IMO.
So I don't think creating a separate kernel component is a good idea.

@stof

This comment has been minimized.

Member

stof commented Jun 5, 2018

the article from @TomasVotruba is actually using HttpKernel (but then not reusing the built-in tag-based configuration for commands).

@symfony/deciders a solution might be to move \Symfony\Bundle\FrameworkBundle\Console\Application to HttpKernel, as it only integrates the HttpKernel and the Console component (the compiler pass registering commands as services is already part of the Console component). That would allow reusing the logic without FrameworkBundle (users would then only have to register the compiler pass in their ContainerBuilder and use the Application).

@PabloKowalczyk

This comment has been minimized.

Contributor

PabloKowalczyk commented Jun 5, 2018

@nicolas-grekas You are right but symfony/skeleton also comes with symfony/http-kernel, symfony/http-foundation or symfony/routing - i definitely won't use them in CLI-only app. Maybe there should be something like symfony/cli-skeleton to fit "CLI-only app" needs?

@stof You are also right, but i was thinking about something that will just glue DI container and Console component.

As i said, in this RFC i'm only interested in something that will just glue DI container and Console components. Maybe new component, maybe new class in DI/Console component - i don't know but i'm open for suggestions. IMO there is not sense in writing this integration in every CLI app because this integration is already written, but only for http-kernel.

BTW having skeleton that will bootstrap CLI app, with Symfony components of course, would be very nice :)

@stof

This comment has been minimized.

Member

stof commented Jun 5, 2018

@PabloKowalczyk but if you don't have FrameworkBundle, you don't have any of the autoconfiguration rules for core classes (as FrameworkBundle is the one shipping them). So you would have to reconfigure it all.
And you would also not be compatible with third-party commands (as the extension point to include third-party services in the container is the bundle system, and you said you don't want http-kernel which contains this system)

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Jun 5, 2018

@stof > but if you don't have FrameworkBundle, you don't have any of the autoconfiguration rules for core classes (as FrameworkBundle is the one shipping them). So you would have to reconfigure it all.

That's right. Framework bundle should be split to more decoupled parts. It seems like storage for "all other features".

I don't want to be forced to use whole FrameworkBundle, just to be able to load Command services. So I rather load them in own Compiler in 5 lines.

@chalasr

This comment has been minimized.

Member

chalasr commented Jun 5, 2018

👍 for @stof proposal about moving the frameworkbundle application to the kernel.

it seems like storage for "all other features".

@TomasVotruba we already moved compiler passes to their respective component, we try hard to break that statement as time goes by.
For the autoconfiguration part, perhaps we could find a way to make each component able to register their own interfaces for autoconfiguration? Something similar to compiler passes e.g. $container->addAutoconfigurationProvider(new ValidatorAutoconfigurationProvider()).

@PabloKowalczyk

This comment has been minimized.

Contributor

PabloKowalczyk commented Jun 23, 2018

So, any ideas how we can simplify adding DI container to symfony/console component?

@Korbeil

This comment has been minimized.

Contributor

Korbeil commented Jun 28, 2018

Hi, quite new around, so if I'm doing this bad, just tell me, I'll be glad to improve myself ;) I'm doing some console apps and would love to see such functionality ! So here I'm trying to help :)

So, from what I understood, we have a lot of configuration stuff coming from FrameworkBundle, problem is: you take all of them or none.

Another problem here, on my opinion, is that we should not mix framework-related configuration and components (console or any other component), since components are standalone and adding this configuration stuff will add dependencies.

Instead of moving this configuration stuff, could we make it more flexible ?
Like, for example, having "groups" of classes loaded from a config file:

framework:
  use_compiler_pass:
    - console
    - annotations

That will filter what we have to use in FrameworkBundle::build() (indeed, this will load everything if nothing is specified).

This is probably not the best way to do it, but it will fulfill our purpose without having to change many stuff in DI and all configuration there is in FrameworkBundle.

On another hand, we could do like for AddConsoleCommandPass and handpick all CompilerPass we need to move. But that won't resolve the way to load them.
Where some CliKernel could fill that and load all CompilerPass we need.

By the way, I'll be glad to help for both options.
Any opinions?

@DanieleGBX

This comment has been minimized.

DanieleGBX commented Jun 28, 2018

There's also a partial issue of perception, the fact that Symfony can't genuinely tout a fat-free setup for CLI-only DI-enabled apps will hinder adoption rate.

@theofidry

This comment has been minimized.

Contributor

theofidry commented Jun 28, 2018

After a fresh Symfony skeleton install:

vendor
├── composer
├── psr
│   ├── cache
│   ├── container
│   ├── log
│   └── simple-cache
└── symfony
    ├── cache
    ├── config
    ├── console
    ├── debug
    ├── dependency-injection
    ├── dotenv
    ├── event-dispatcher
    ├── filesystem
    ├── finder
    ├── flex
    ├── framework-bundle
    ├── http-foundation
    ├── http-kernel
    ├── polyfill-mbstring
    ├── routing
    └── yaml

What looks useless at first sight for a backbone CLI app is:

  • cache
  • dotenv
  • event-dispatcher
  • filesystem
  • finder
  • http-foundation
  • http-kernel
  • routing
  • yaml
@Korbeil

This comment has been minimized.

Contributor

Korbeil commented Jun 28, 2018

event-dispatcher, cache, filesystem, finder and yaml can be usefull in some case.
(For example: my last cli app is using event-dispatcher)
The best we can do is allow to load any component and like @PabloKowalczyk said, having some autoconfigure() command to manage all CompilerPass within a given component.

@theofidry

This comment has been minimized.

Contributor

theofidry commented Jun 28, 2018

I'm not saying they are not, I'm questioning if they are really necessary and can seem overkill for a backbone app. But in the end the skeleton is 15M and ~3K files, it's really not much to begin with. I feel like if you CLI app is big enough to justify a DI container like the Symfony one, those extra dependencies shouldn't be a blocker. I just irk a bit at the idea of depending on non-necessary dependencies but it's really nitpicking at this point

@PabloKowalczyk

This comment has been minimized.

Contributor

PabloKowalczyk commented Jun 28, 2018

What about extending \Symfony\Component\Console\Application constructor with third (optional of course), ContainerBuilder, parameter like public function __construct($name = 'UNKNOWN', $version = 'UNKNOWN', ContainerBuilder $containerBuilder = null)?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 28, 2018

There is a big issue that has been described by @stof above: this is seeking to create an alternative configuration system than the existing one based on bundles. Technically, you can create one for sure, but this also means that this would create an incompatible way of doing things. What I'd call a dead end for projects that take this direction - or a niche at least.

A better vision for me is to standardize on the skeleton to start small, and have all of us extend it in the same way. This would allow composing/sharing features more easily in the community.

Honestly, I don't see the issue with the frameworkbundle as a glue system. Of course, if we can move some logic to components - and if can further shrink the skeleton, why not. But I would really like we work on something that doesn't create an alternative configuration system for the sake of it and instead creates a standard way to wire features in the Symfony world, like SF4+Flex already started to do.

@DanieleGBX

This comment has been minimized.

DanieleGBX commented Jun 28, 2018

Tbh, as long as http-foundation, http-kernel, and routing are out of the structure of a CLI only app, I'm happy, anything that gives us that is welcome.

@TomasVotruba

This comment has been minimized.

Contributor

TomasVotruba commented Jun 28, 2018

Size nor file count nor number components is the main issue.

The main point from my side is teaching programmers bad programming habbits by overcoupling to the framework.

This naturally leads to legacy code and "use everything we can because we can" instead of "is there a reason to do this" thinking.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 28, 2018

And light coupling to the framework doesn't mean don't use a framework. I feel like that's what you're implying here, which is wrong of course. You can build clean architecture on top of Symfony. And it's harder without actually.

@PabloKowalczyk

This comment has been minimized.

Contributor

PabloKowalczyk commented Jun 28, 2018

@nicolas-grekas did you saw my comment from an hour ago?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 28, 2018

I just did thanks for the ping. I don't know actually...

@Korbeil

This comment has been minimized.

Contributor

Korbeil commented Jun 29, 2018

Just got some idea, let's share, the idea is to create a Framework directory inside all component, that directory will manage all autoconfiguration behavior for each component.

For example, the Console component:

Console
├── ...
├── Framework
    ├── DependencyInjection/
    │   └── Compiler/
    ├── Descriptor/
    ├── Helper/
    ├── Application.php
    └── Autoconfigure.php
  • DependencyInjection/ will contains Component extension loading.
  • Compiler/ for all CompilerPass
  • Descriptor/, Helper/, Application.php corresponds to classes that were in Symfony\Bundle\FrameworkBundle\Console
  • Autoconfigure.php will manage all CompilerPass loading as FrameworkBundle does today.

Like that, we can still manage every configuration from FrameworkBundle, but it would be just loading each component Framework configuration and not assuming what you are using, or not.

It will also contain all configuration related class that we actually store in FrameworkBundle, like Application for Console. At the end, FrameworkBundle would be only a loader and checking if dependencies are ok.

@Korbeil

This comment has been minimized.

Contributor

Korbeil commented Aug 17, 2018

Quick update. After some thinking, I think we should focus on component loading on FrameworkBundle side. Theses "Framework-related" configuration hasn't to be out of the FrameworkBundle but they should be loaded only if needed.

Autoconfiguration being decoupled from FrameworkBundle is just a mather of time but nothing to rush on that side I think.

(I still rebased my last PR since it's still a good thing to move theses and will try to take a look at how we could load components only when they're here)

symfony-splitter pushed a commit to symfony/framework-bundle that referenced this issue Oct 10, 2018

feature #27770 [FrameworkBundle] Moving Cache-related CompilerPass to…
… Cache component (Korbeil)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[FrameworkBundle] Moving Cache-related CompilerPass to Cache component

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Hi, first PR here 🎉

This is related to #27479 and a first work to move Cache-related CompilerPass out of `FrameworkBundle`, it allows to decouple part of the FrameworkBundle configuration classes.

Since we didn't choosed a fixed directory organization to manage theses, I actually did same as in Bundles and used DependencyInjection folder. If we do choose to follow my [last comment directory organization proposal](symfony/symfony#27479 (comment)), I'll move theses CompilerPass to `Framework/DependencyInjection/Compiler` directory (nothing hard here).

Thanks to @DanieleGBX that helped me checking this PR and gave me some good advices !

Here is a list of all CompilerPass I moved, with related component (I also moved related tests when they were present):
- **Cache** - CacheCollectorPass
- **Cache** - CachePoolClearerPass
- **Cache** - CachePoolPass
- **Cache** - CachePoolPrunerPass

Commits
-------

53e7040829 moving Cache-related compiler pass from FrameworkBundle to Cache component

symfony-splitter pushed a commit to symfony/cache that referenced this issue Oct 10, 2018

feature #27770 [FrameworkBundle] Moving Cache-related CompilerPass to…
… Cache component (Korbeil)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[FrameworkBundle] Moving Cache-related CompilerPass to Cache component

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Hi, first PR here 🎉

This is related to #27479 and a first work to move Cache-related CompilerPass out of `FrameworkBundle`, it allows to decouple part of the FrameworkBundle configuration classes.

Since we didn't choosed a fixed directory organization to manage theses, I actually did same as in Bundles and used DependencyInjection folder. If we do choose to follow my [last comment directory organization proposal](symfony/symfony#27479 (comment)), I'll move theses CompilerPass to `Framework/DependencyInjection/Compiler` directory (nothing hard here).

Thanks to @DanieleGBX that helped me checking this PR and gave me some good advices !

Here is a list of all CompilerPass I moved, with related component (I also moved related tests when they were present):
- **Cache** - CacheCollectorPass
- **Cache** - CachePoolClearerPass
- **Cache** - CachePoolPass
- **Cache** - CachePoolPrunerPass

Commits
-------

53e7040829 moving Cache-related compiler pass from FrameworkBundle to Cache component

fabpot added a commit that referenced this issue Oct 10, 2018

feature #27770 [FrameworkBundle] Moving Cache-related CompilerPass to…
… Cache component (Korbeil)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[FrameworkBundle] Moving Cache-related CompilerPass to Cache component

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Hi, first PR here 🎉

This is related to #27479 and a first work to move Cache-related CompilerPass out of `FrameworkBundle`, it allows to decouple part of the FrameworkBundle configuration classes.

Since we didn't choosed a fixed directory organization to manage theses, I actually did same as in Bundles and used DependencyInjection folder. If we do choose to follow my [last comment directory organization proposal](#27479 (comment)), I'll move theses CompilerPass to `Framework/DependencyInjection/Compiler` directory (nothing hard here).

Thanks to @DanieleGBX that helped me checking this PR and gave me some good advices !

Here is a list of all CompilerPass I moved, with related component (I also moved related tests when they were present):
- **Cache** - CacheCollectorPass
- **Cache** - CachePoolClearerPass
- **Cache** - CachePoolPass
- **Cache** - CachePoolPrunerPass

Commits
-------

53e7040 moving Cache-related compiler pass from FrameworkBundle to Cache component
@DanieleGBX

This comment has been minimized.

DanieleGBX commented Oct 11, 2018

Okay, so step one happened, what's next on the line?

@Korbeil

This comment has been minimized.

Contributor

Korbeil commented Oct 11, 2018

Thanks for keeping that issue updated ;)

Like I said in last comment, it wasn't really a first step, I started thinking it was but we don't really need to move theses to make FrameworkBundle load only components we required (and not all he defined).

I'll try to make a draft on how we could make FrameworkBundle load only what he have. But the idea is to rely only on existent class and not force users to load everything FrameworkBundle wants.

@chalasr

This comment has been minimized.

Member

chalasr commented Oct 11, 2018

I have the feeling that the discussion is deviating from the original issue.
Arguments against a CliKernel have been given and I agree with them.
About making symfony/console's Application aware of ContainerBuilder and able to compile it, I don't think it's worth it.
Moving the FrameworkBundle's Application to HttpKernel is stil possible, PR welcomed I guess.
I think we should close here and eventually open one or more dedicated issues about moving component-related things from FrameworkBundle to components.

@Korbeil

This comment has been minimized.

Contributor

Korbeil commented Oct 12, 2018

@chalasr I started moving stuff out of FrameworkBundle but this wasn't the goal of this issue.
We more talked about a way to make FrameworkBundle loads only components that are loaded out of his scope but should not have to load components that may not be needed (including HttpKernel)

If you prefer I can open 2 other issues about decoupling passes from FrameworkBundle & about that loading issue.

@DanieleGBX

This comment has been minimized.

DanieleGBX commented Oct 12, 2018

@chalasr I strongly disagree, I don't see any consensus established against allowing cli-only applications to be able to run using cached container without requiring the http layer.

@PabloKowalczyk

This comment has been minimized.

Contributor

PabloKowalczyk commented Oct 13, 2018

@Korbeil do you also plan to move code related to container building/caching out of \Symfony\Component\HttpKernel\Kernel? IMO this is the point of the issue, but IDK is it even possible to move the code?

@chalasr

This comment has been minimized.

Member

chalasr commented Oct 13, 2018

I don't see how to provide more reusable code regarding container caching in a relevant way.
Just extracting the code from Kernel to a class/trait would not help, even your Crunz example does not use the exact same code (hence my suggestion to close, nobody proposed a viable/worthwhile implementation yet).

@alquerci

This comment has been minimized.

Contributor

alquerci commented Oct 29, 2018

Hello @PabloKowalczyk,

@Korbeil do you also plan to move code related to container building/caching out of \Symfony\Component\HttpKernel\Kernel? IMO this is the point of the issue, but IDK is it even possible to move the code?

Yes, the idea is this one.

Currently the HttpKernel component depends on a lot of packages. But as soon as we remove all HTTP features dependencies becomes fewer.

We can see that the console Application only required the Kernel without HTTP features.

I propose a solution to this issue.

The plan can be the following

1. Create Kernel component extracted from HttpKernel without HTTP features

Excluding Tests it becomes very small:

Without HTTP related methods on the Kernel class

  • terminate
  • handle
  • getHttpKernel
Bundle/
├── BundleInterface.php
└── Bundle.php
DependencyInjection/
├── AddAnnotatedClassesToCachePass.php
├── ConfigurableExtension.php
├── Extension.php
└── MergeExtensionConfigurationPass.php
KernelInterface.php
Kernel.php
RebootableInterface.php
Config/
└── FileLocator.php

Note: Cache warmup and clearer can be added.

Dependencies becomes tiny as well:
  • symfony/config
  • symfony/dependency-injection

2. The HttpKernel depends (and extends Kernel to provide a BC layer)

3. The Kernel can be extended and/or used for any usage like for a CliKernel

This provides a very tiny application with the power of the DIC.

I am able to provide a PR.

cc @stof @nicolas-grekas @chalasr

@alquerci

This comment has been minimized.

Contributor

alquerci commented Nov 5, 2018

As the version 4.2 is currently on stabilization process then if the idea is approved it will target the version 4.3 anyway in December.

However, for experimentation purpose can I create a repository to support from 4.1 to current version (v4.1.7, 4.1, master).

I have a project that initiated the migration to Symfony4 and need this extraction to take profit of wonderful features provided by the DIC.

cc @thePanz

@alquerci

This comment has been minimized.

Contributor

alquerci commented Nov 8, 2018

The experimental repository created there https://github.com/alquerci/kernel.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment