Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Console] Add support for command lazy-loading#22734

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:3.4fromchalasr:lazy-console-commands
Jul 12, 2017

Conversation

@chalasr
Copy link
Member

@chalasrchalasr commentedMay 17, 2017
edited by nicolas-grekas
Loading

QA
Branch?3.4
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#12063#16438#13946#21781
LicenseMIT
Doc PRtodo

This PR adds command lazy-loading support to the console, based on PSR-11 and DI tags.
(#12063 (comment))

Commands registered as services which set thecommand attribute on theirconsole.command tag are now instantiated when callingApplication::get() instead of all instantiated atrun().

Usage

app.command.heavy:tags:        -{ name: console.command, command: app:heavy }

This way private command services can be inlined (no public aliases, remain really private).

With console+PSR11 implem only:

$application =newApplication();$container =newServiceLocator(['heavy' =>function () {returnnewHeavy(); }]);$application->setCommandLoader(newContainerCommandLoader($container, ['app:heavy' =>'heavy']);

Implementation is widely inspired from Twig runtime loaders (without the definition/runtime separation which is not needed here).

TomasVotruba, yceruto, Taluu, ogizanagi, Koc, theofidry, ro0NL, maidmaid, skalpa, tjaari, and 9 more reacted with thumbs up emojiyceruto, theofidry, jvasseur, gnugat, BPScott, robfrawley, apfelbox, efiku, mnapoli, supercid, and 2 more reacted with hooray emoji
publicfunction__construct(ContainerInterface$container,array$commandNames)
{
$this->container =$container;
$this->commandNames =$commandNames;
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the point of having an array of command names when you could just have all these in the container ? Just for theall ?

Copy link
MemberAuthor

@chalasrchalasrMay 17, 2017
edited
Loading

Choose a reason for hiding this comment

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

yes, forfind() and related helpers especially (resolvingreq asrequire)

*/
publicfunctionget($name)
{
if (!in_array($name,$this->commandNames,true)) {

Choose a reason for hiding this comment

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

Could we replace this byif(!$this->has($name)) to avoid duplicating code? (unless this function call makes a big difference in performance)

Copy link
MemberAuthor

@chalasrchalasrMay 18, 2017
edited
Loading

Choose a reason for hiding this comment

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

I have no strong opinion on this, I looked at the existing and it seems duplicating has been preferredhttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/ServiceLocator.php#L47.
However it means that if a sub class extendshas() for some reasons,get() would still use the "default" way. Not sure if it's a good thing, perhaps someone else has an hint?

Copy link
Contributor

Choose a reason for hiding this comment

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

i would use the api method indeed, or mark final :)

chalasr reacted with thumbs up emoji
@nicolas-grekasnicolas-grekas added this to the3.4 milestoneMay 18, 2017
@chalasrchalasrforce-pushed thelazy-console-commands branch 6 times, most recently from3539734 to10871a4CompareMay 18, 2017 14:15
Copy link
Contributor

@ro0NLro0NL left a comment

Choose a reason for hiding this comment

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

like this in general 👍

}elseif ($this->commandLoader &&$this->commandLoader->has($name)) {
$command =$this->commandLoader->get($name);
if (!$command->getName()) {
$command->setName($name);
Copy link
Contributor

Choose a reason for hiding this comment

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

wouldnt it be more safe to always set this?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

indeed

if (isset($this->commands[$name])) {
$command =$this->commands[$name];
}elseif ($this->commandLoader &&$this->commandLoader->has($name)) {
$command =$this->commandLoader->get($name);
Copy link
Contributor

Choose a reason for hiding this comment

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

$command = $this->commands[$name] = ...;?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

add() does more than$this->command[$name] = $command, hence calling it

Copy link
Contributor

Choose a reason for hiding this comment

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

omfg. my bad :)

chalasr reacted with laugh emoji
/**
* @return string[] All registered command names
*/
publicfunctiongetCommandNames();
Copy link
Contributor

Choose a reason for hiding this comment

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

getNames?

chalasr reacted with thumbs up emoji
*/
publicfunctionget($name)
{
if (!in_array($name,$this->commandNames,true)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i would use the api method indeed, or mark final :)

chalasr reacted with thumbs up emoji
$definition =$container->getDefinition($id);
$class =$container->getParameterBag()->resolveValue($definition->getClass());

if (!$r =$container->getReflectionClass($class)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this skip abstracts first?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

ro0NL reacted with thumbs up emoji
private$loaderServiceId;
private$commandTag;

publicfunction__construct($loaderServiceId ='console.command_loader',$commandTag ='console.command')
Copy link
Contributor

Choose a reason for hiding this comment

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

imo.$tag or$commandLoaderServiceId ;-)

chalasr reacted with thumbs up emoji
publicfunctionhas($name)
{
returnisset($this->commands[$name]);
returnisset($this->commands[$name]) ||$this->commandLoader &&$this->commandLoader->has($name);
Copy link
Contributor

@ro0NLro0NLMay 18, 2017
edited
Loading

Choose a reason for hiding this comment

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

personally i preferx || (y && z)

@chalasrchalasrforce-pushed thelazy-console-commands branch 4 times, most recently from6461a31 to3ec61bbCompareMay 18, 2017 18:13
@theofidry
Copy link
Contributor

Wouldn't an alternative be to make an abstract static functiongetName(): string which could be called without instantiating the command? This would be a simpler solution and avoid the need to sync the command name from the tag and the code, although it would require to change the way we currently give the names to the commands.

@chalasr
Copy link
MemberAuthor

@theofidry I thought about it.
First, makinggetName() static would forbid having several commands for the same class, that is a valid use case to me (2b82fcb).
It would also be hard to achieve from a BC pov (actually, I don't see any smooth upgrade path for making a method static).
Lastly, this is mostly about commands registered as services (with dependencies), the current implementation doesn't require any change for basic command which are still in majority.
So I think the static alternative would be a too big step for the need that this is trying to solve.

@chalasr
Copy link
MemberAuthor

chalasr commentedMay 18, 2017
edited
Loading

sync the command name from the tag and the code

Note that when using this feature, you don't need to set the name in your command. It is automatically set when the command is loaded. Same for aliases.

@chalasrchalasrforce-pushed thelazy-console-commands branch from3ec61bb to7e90c79CompareMay 18, 2017 20:43
@Koc
Copy link
Contributor

Koc commentedMay 19, 2017

This implementation looks better than previous

@sepehr
Copy link
Contributor

This is a very good news for a lot of people around the world! Thanks.

TomasVotruba, 1ma, and gnugat reacted with thumbs up emoji

@chalasrchalasrforce-pushed thelazy-console-commands branch 4 times, most recently from6cd19b0 to9fa9fc7CompareJuly 11, 2017 22:59
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment
edited
Loading

Choose a reason for hiding this comment

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

👍 with "for fine tuning" comments.

In a future PR, I thinkCommandLoaderInterface could be replaced by a more generic interface in the DI component, which would be an extension of PSR11 and would add a "getProvidedServiceIds" to it. But that's to be discussed in another PR :)

/**
* @return iterable All registered commands mapped by names
*/
publicfunctionall();
Copy link
Member

@nicolas-grekasnicolas-grekasJul 12, 2017
edited
Loading

Choose a reason for hiding this comment

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

unless I'm misread, this method is not used at all, better remove it then, esp. since getNames() already allows iterating when needed

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

agreed,all() removed

* @param ContainerInterface $container A container from which to load command services
* @param array $commandMap An array with command names as keys and service ids as values
*/
publicfunction__construct(ContainerInterface$container,array$commandMap)
Copy link
Member

@nicolas-grekasnicolas-grekasJul 12, 2017
edited
Loading

Choose a reason for hiding this comment

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

if container can be keyed by command names, then we won't need $commandMap (we'll need "$commandNames", which would be enough.)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I started with only command names, but@mnapoli noticed that it prevents mapping a command to a service that is not named the same as the command (#22734 (comment)), which can be useful especially outside of the fullstack I think.
A good side effect is that using acommandMap makes that we register only one closure for a command and its aliases (they're all mapped to the same service id in the commandMap array instead of in the locator factories, that gives a lighter container). Still not convinced?

Choose a reason for hiding this comment

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

Ok, fine for me, thanks for the details

@ogizanagi
Copy link
Contributor

@nicolas-grekas :

In a future PR, I think CommandLoaderInterface could be replaced by a more generic interface in the DI component, which would be an extension of PSR11 and would add a "getProvidedServiceIds" to it. But that's to be discussed in another PR :)

I think we should avoid replacingCommandLoaderInterface completely, which would mean tying this to the DI component. In a simple CLI application, you probably won't requiresymfony/dependency-injection but simply create your own implementation instead.
BTW, I'd like to suggest a simple factory based implementation in another PR (similar to DI component'sServiceLocator).

nicolas-grekas and chalasr reacted with thumbs up emoji

@chalasrchalasrforce-pushed thelazy-console-commands branch from9fa9fc7 to7f97519CompareJuly 12, 2017 09:59
@nicolas-grekas
Copy link
Member

Thank you@chalasr.

ro0NL, chalasr, maidmaid, skalpa, TomasVotruba, medinae, yceruto, and sepehr reacted with hooray emoji

@nicolas-grekasnicolas-grekas merged commit7f97519 intosymfony:3.4Jul 12, 2017
nicolas-grekas added a commit that referenced this pull requestJul 12, 2017
This PR was merged into the 3.4 branch.Discussion----------[Console] Add support for command lazy-loading| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#12063#16438#13946#21781| License       | MIT| Doc PR        | todoThis PR adds command lazy-loading support to the console, based on PSR-11 and DI tags.(#12063 (comment))Commands registered as services which set the `command` attribute on their `console.command` tag are now instantiated when calling `Application::get()` instead of all instantiated at `run()`.__Usage__```yamlapp.command.heavy:    tags:        - { name: console.command, command: app:heavy }```This way private command services can be inlined (no public aliases, remain really private).With console+PSR11 implem only:```php$application = new Application();$container = new ServiceLocator(['heavy' => function () { return new Heavy(); }]);$application->setCommandLoader(new ContainerCommandLoader($container, ['app:heavy' => 'heavy']);```Implementation is widely inspired from Twig runtime loaders (without the definition/runtime separation which is not needed here).Commits-------7f97519 Add support for command lazy-loading
@chalasrchalasr deleted the lazy-console-commands branchJuly 12, 2017 12:27
thrownewInvalidArgumentException(sprintf('Missing "command" attribute on tag "%s" for service "%s".',$this->commandTag,$id));
}
if ($commandName !==$tag['command']) {
thrownewInvalidArgumentException(sprintf('The "command" attribute must be the same on each "%s" tag for service "%s".',$this->commandTag,$id));
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be more straightforward if aliases are just the additional tags using thecommand attribute as well?
Then there is no need for an alias property at all and this strange condition doesn't apply either.

tags:        - { name: console.command, command: app:my-command }        - { name: console.command, command: app:my-alias }

apfelbox, mnapoli, ro0NL, ogizanagi, DavidBadura, jvasseur, and chalasr reacted with thumbs up emoji
chalasr pushed a commit that referenced this pull requestJul 19, 2017
…application with lazy-loading needs (ogizanagi)This PR was merged into the 3.4 branch.Discussion----------[Console] Add a factory command loader for standalone application with lazy-loading needs| Q             | A| ------------- | ---| Branch?       | 3.4 <!-- see comment below -->| Bug fix?      | no| New feature?  | yes <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass?   | yes (failure unrelated)| Fixed tickets |#22734 (comment) <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | todo (withsymfony/symfony-docs#8147)So standalone applications can also benefit from the lazy loading feature without requiring a PSR-11 implementation specifically for this need.The loader does not memoize any resolved command from factories, as it's the `Application` responsibility and the `ContainerCommandLoader` does not either (the PSR-11 does not enforce two successive calls to return the same value).Commits-------9b40b4a [Console] Add a factory command loader for standalone application with lazy-loading needs
symfony-splitter pushed a commit to symfony/console that referenced this pull requestJul 19, 2017
…application with lazy-loading needs (ogizanagi)This PR was merged into the 3.4 branch.Discussion----------[Console] Add a factory command loader for standalone application with lazy-loading needs| Q             | A| ------------- | ---| Branch?       | 3.4 <!-- see comment below -->| Bug fix?      | no| New feature?  | yes <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks?    | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass?   | yes (failure unrelated)| Fixed tickets |symfony/symfony#22734 (comment) <!-- #-prefixed issue number(s), if any -->| License       | MIT| Doc PR        | todo (withsymfony/symfony-docs#8147)So standalone applications can also benefit from the lazy loading feature without requiring a PSR-11 implementation specifically for this need.The loader does not memoize any resolved command from factories, as it's the `Application` responsibility and the `ContainerCommandLoader` does not either (the PSR-11 does not enforce two successive calls to return the same value).Commits-------9b40b4a [Console] Add a factory command loader for standalone application with lazy-loading needs
fabpot added a commit that referenced this pull requestJul 20, 2017
…oconfigure enabled (chalasr)This PR was merged into the 3.4 branch.Discussion----------[Console] Fix registering lazy command services with autoconfigure enabled| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aFor```yaml_defaults:    autoconfigure: trueApp\:    resource: '../../src/*'App\Command\FooCommand:    tags:        - { name: console.command, command: foo }```Before you get the following error:> Missing "command" attribute on tag "console.command" for service "App\Command\FooCommand"Now the command is lazy.----Btw,@Tobion's#22734 (comment)> Wouldn't it be more straightforward if aliases are just the additional tags using the command attribute as well?Then there is no need for an alias property at all and this strange condition doesn't apply either.Partially addressed here by removing the need for repeating the `command` attribute on each `console.command` tag```yaml# beforetags:    - { name: console.command, command: foo }    - { name: console.command, command: foo, alias: foobar }# aftertags:    - { name: console.command, command: foo }    - { name: console.command, alias: foobar }```Tobias proposal:```yamltags:    - { name: console.command, command: app:my-command }    - { name: console.command, command: app:my-alias }```I wanted to propose exactly the same at first, but finally found more clear to add a specific attribute for aliases, especially because relying on the order on which tags are defined sounds less good to me. Please tell me about your preference.(And sorry for the noise around this feature, I want to polish it for 3.4)Commits-------8a71aa3 Fix registering lazy command services with autoconfigure enabled
symfony-splitter pushed a commit to symfony/console that referenced this pull requestJul 20, 2017
…oconfigure enabled (chalasr)This PR was merged into the 3.4 branch.Discussion----------[Console] Fix registering lazy command services with autoconfigure enabled| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aFor```yaml_defaults:    autoconfigure: trueApp\:    resource: '../../src/*'App\Command\FooCommand:    tags:        - { name: console.command, command: foo }```Before you get the following error:> Missing "command" attribute on tag "console.command" for service "App\Command\FooCommand"Now the command is lazy.----Btw,@Tobion'ssymfony/symfony#22734 (comment)> Wouldn't it be more straightforward if aliases are just the additional tags using the command attribute as well?Then there is no need for an alias property at all and this strange condition doesn't apply either.Partially addressed here by removing the need for repeating the `command` attribute on each `console.command` tag```yaml# beforetags:    - { name: console.command, command: foo }    - { name: console.command, command: foo, alias: foobar }# aftertags:    - { name: console.command, command: foo }    - { name: console.command, alias: foobar }```Tobias proposal:```yamltags:    - { name: console.command, command: app:my-command }    - { name: console.command, command: app:my-alias }```I wanted to propose exactly the same at first, but finally found more clear to add a specific attribute for aliases, especially because relying on the order on which tags are defined sounds less good to me. Please tell me about your preference.(And sorry for the noise around this feature, I want to polish it for 3.4)Commits-------8a71aa31bb Fix registering lazy command services with autoconfigure enabled
@Danack
Copy link

Hi everyone,

The solution that has been raised in this ticket seems complex and a workaround for the fundamental problem that combining the 'routing' of commands with the dispatching of commands is a 'bad idea', which is why most modern frameworks separate those two concerns in HTTP contexts; not separating them in CLI contexts seems just the wrong thing to do.

I've was hoping when Iraised this issue a while ago that it could be improved in the next version of Symfony. I still think that would be a better approach than using lazy loading, which always makes it harder to reason about what is actually happening inside an application.

I realise github comments are never the best place to discuss philosophical differences in software architecture - do you guys have a more appropriate place to discuss this?

Otherwise I think I'm going to invite people to an 'official', maintained fork of the Symfony/console library, which would be along the lines of the version I've been using for a couple of yearshttps://github.com/danack/console

@nicolas-grekas
Copy link
Member

I don't see what's complex with implemetingCommandLoaderInterface. Basically, it's what you say: a intermediary "router" between names and commands.

@Ocramius
Copy link
Contributor

Ocramius commentedAug 13, 2017 via email

The complexity comes from adding a workaround rather than tackling theissue at core (which might not be possible).
On 13 Aug 2017 5:34 PM, "Nicolas Grekas" ***@***.***> wrote: I don't see what's complex with implemeting CommandLoaderInterface. Basically, it's what you say: a intermediary "router" between names and commands. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22734 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJakC9e_jZVDSHlnhnE8ycZrLhLgI5tks5sXxeHgaJpZM4Neeqc> .

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedAug 13, 2017
edited
Loading

(sorry, bad issue number, reposting on#11974)
(hum, ok, no, the comment has been double-posted, I've been confused :) )

This was referencedOct 18, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@javiereguiluzjaviereguiluzjaviereguiluz left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@fabpotfabpotfabpot approved these changes

+4 more reviewers

@TaluuTaluuTaluu left review comments

@TobionTobionTobion left review comments

@mnapolimnapolimnapoli left review comments

@ro0NLro0NLro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

19 participants

@chalasr@theofidry@Koc@weaverryan@stof@Ocramius@skalpa@gnugat@mnapoli@sepehr@ogizanagi@nicolas-grekas@Danack@fabpot@javiereguiluz@Taluu@Tobion@ro0NL@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp