contributte / console

:boom: Best minimal console (symfony/console) to Nette Framework (@nette)
https://contributte.org/packages/contributte/console.html
MIT License
91 stars 32 forks source link

Console Helpers support #2

Closed enumag closed 7 years ago

enumag commented 7 years ago

I like how minimal this integration of Kdyby console is. However I still think it should support console helpers. See the relevant lines in Kdyby.

f3l1x commented 7 years ago

@enumag Thanks. We might support it. Good point. I'll implement it.

Any other thoughts/ideas? Please consider it as small as possible. ;-)

TomasVotruba commented 7 years ago

@enumag HelperSet is just another services locator. I'm strictly against it, because it leads you to write overcomplicated structures as in Kdyby.

What is the advantage over constructor injection?

enumag commented 7 years ago

@TomasVotruba Symfony commands are not really compatible with constructor injection because all of them are instantiated every time you use console - instead of only the one you need. If you know a solution, please share.

TomasVotruba commented 7 years ago

@enumag Why is that issue? I use is for years like that and never had a problem.

If so, it belongs to Symfony\Console repository, not anywhere else.

Do you prefer service locators then?

enumag commented 7 years ago

@TomasVotruba It is a problem because I don't want to create all the services any command might have a dependency on every time I run any of them. Sometimes I use commands as long-running processes and I want to reduce their memory usage - meaning it's better to instantiate only what is necessary for the given command. This is the same thing as with Presenters/Controllers - would you want to instantiate all of them for every http request? Me neither. But this of course means there is some sort of service locator in PresenterFactory/ControllerResolver - and that's ok with me.

The thing with service locator is that it is not necessarily bad nor fully replaceable by dependency injection. It just should be used carefully and mainly hidden in libraries, but not used in applications themselves. This is of course why you dislike console helper. And I agree with you, I don't like them either. The service locator would be far better to have in Symfony\Component\Console\Application to load commands, not in the commands themselves. The real issue here is that Symfony\Component\Console\Command\Command::configure() is not static and implement this would require hacks involving ReflectionClass::newInstanceWithoutConstructor().

TomasVotruba commented 7 years ago

@enumag Still, Symfony\Console issue. This is wrong level of abstraction to workaround this.

There are many similar issues: https://github.com/symfony/symfony/issues?utf8=%E2%9C%93&q=resolve%20is%3Aopen%20label%3AConsole%20

I recommend you focusing your energy there. It might actually change something and make situation better for all Symfony\Console integrations.

enumag commented 7 years ago

@TomasVotruba Precisly. But since there is no better solution at the moment it is still needed.

I'm well aware of those issues but since they have been sitting there for years I consider it a waste of time. Solving it is too much of a BC break for Symfony.

f3l1x commented 7 years ago

@enumag I've discovered that some default helpers are already setup in Symfony\Application (https://github.com/symfony/console/blob/v3.2.8/Application.php#L84).

I've merged #4 just few minutes ago. Do you need anything more? I saw some Container and Presenter helpers in Kdyby. Do you need them too?

enumag commented 7 years ago

I think #4 is enough. Please don't pollute this package with Kdyby garbage.

f3l1x commented 7 years ago

@enumag Great. But, what about ContainerHelper or PresenterHelper, do you use them? In case of you do, do you want to create them for each project?

enumag commented 7 years ago

ContainerHelper is about the only way to use services in commands. Or do you know any other? PresenterHelper - no thanks.

f3l1x commented 7 years ago

@enumag Could we consider this issue as done?

https://github.com/contributte/console/commit/9fe2639c3e06a8684b973153668d9ecc5adc6756


I experiment with some ProxyCommands for lazy-loading. We'll see. :-)

enumag commented 7 years ago

Yeah, thanks!