I agree that the code looks weird and it doesn't make sense ... but if you remove that method, 1 test fails:

1) Symfony\Component\Console\Tests\ApplicationTest::testRun->run() runs the help command if --help is passedFailed asserting that two strings are equal.--- Expected+++ Actual@@ @@ 'Usage:-  list [options] [--] [<namespace>]+  list [options] [--] <command> [<namespace>]

Looks like a dx issue to me:

/** * @param bool $short Whether to show the short version of the synopsis (with options folded) or not */publicfunctiongetSynopsis($short =false){$key =$short ?'short' :'long';if (!isset($this->synopsis[$key])) {$this->synopsis[$key] =trim(sprintf('%s %s',$this->name,$this->definition->getSynopsis($short)));    }return$this->synopsis[$key];}

Could becomegetLongSynopsis andgetShortSynopsis

javiereguiluz your are right. Sorry that i did not dig deeper into it.

getSynopsis() avoids the single responsibility principle (SRP). Because:

• it is a getter method AND
• it initializes an internal array

Calling a getter method without using its return value is a:

• SRP violation indicator OR
• just waist of resources

Instead of calling:
$this->getSynopsis(true);
$this->getSynopsis(false);

we should call:
$this->initLongAndShortSynopsis() (needs to be implemented)
or
$this->initLongSynopsis() (needs to be implemented)
$this->initShortSynopsis() (needs to be implemented)

getSynopsis() should be refactored to use the init methods.

Status: needs work

@christian-weiss have you digged in why calling this method twice does not make senseexactly? The comment is more or less clear about it

// force the creation of the synopsis before the merge with the app definition

Ie. what are you trying to solve?getSynopsis follows SRP perfectly fine, it just lazy loads its value. Agree the long/short version could be separated in 2 methods.. however i dont think that's worth the effort / a real (SRP) issue.

@ro0NL

My understanding is@christian-weiss is trying to improve DX, and obviously calling a getter method twice and ignoring its return value isn't particularly straight-forward code to read.

In the context of this specific piece of code, I don't have a strong opinion as to whether it is beneficial for this method to remain exactly as is, or for it to be refactored intoinitializeSynopsis,getShortSynopsis, andgetLongSynopsis (or similar).

I simply want to point out that the SRP deficiency@christian-weiss asserted is absolutely valid. Saying (and I quote) "getSynopsis follows SRP perfectly fine" couldn't be farther from the truth: thegetSynopsis method obviously has three distinct responsibilities:

• Class member initialization.
• Getter method (for long synopsis).
• Getter method (for short synopsis).

Now, to be clear, this isn't meant to be an endorsement of refactoring the code: sometimes the most functional codedoes not perfectly model every rule, andthat's just fine.

But then, the conversation shouldnot be whether or not the method follows thesingle responsibility principle (it doesn't), but instead focus on where to draw the line betweeneffective SRP (and inherit DX clarity that is achieved through the application of this rule) andinefficient SRP (and the excessive verbosity that results).

Imo the problem is pre-initializing here because otherwisemergeApplicationDefinition wont work?

Im not fully aware of inner workings here... butjust removing a method call for cosmetic reasons dont seem right.

Hence i askedhave you digged in why calling this method twice does not make sense exactly?

"getSynopsis follows SRP perfectly fine"

Yes it's a somewhat pragmatic method (2in1 getter), but valid SRP imo. It'sonly responsibility is to return a short/ or long synopsis.

Ok. Initializing is needed because otherwisemergeApplicationDefinition will update the definition and we cant get the original/needed synopsis anymore (getSynopsis couples with$this->definition).

However it looks inconsistent when using descriptors,TextDescriptor does this as well, butXml only inits the long version. This could be intented, but im not sure.

At first sight i'd say move thegetSynopsis(true) andgetSynopsis(false) calls tomergeApplicationDefinition and cleanup. For better DX we can considerinitSynopsis and use it ingetSynopsis as well.

Passes tests for the console component.

diff --git a/src/Symfony/Component/Console/Command/Command.php b/src/Symfony/Component/Console/Command/Command.phpindex 0d5001b..2df1ec1 100644--- a/src/Symfony/Component/Console/Command/Command.php+++ b/src/Symfony/Component/Console/Command/Command.php@@ -209,10 +209,6 @@ class Command      */     public function run(InputInterface $input, OutputInterface $output)     {-        // force the creation of the synopsis before the merge with the app definition-        $this->getSynopsis(true);-        $this->getSynopsis(false);-         // add the application arguments and options         $this->mergeApplicationDefinition();@@ -300,6 +296,10 @@ class Command             return;         }+        // force the creation of the synopsis before the merge with the app definition+        $this->getSynopsis(true);+        $this->getSynopsis(false);+         $this->definition->addOptions($this->application->getDefinition()->getOptions());         if ($mergeArgs) {diff --git a/src/Symfony/Component/Console/Descriptor/JsonDescriptor.php b/src/Symfony/Component/Console/Descriptor/JsonDescriptor.phpindex 87e38fd..c3852a8 100644--- a/src/Symfony/Component/Console/Descriptor/JsonDescriptor.php+++ b/src/Symfony/Component/Console/Descriptor/JsonDescriptor.php@@ -152,7 +152,6 @@ class JsonDescriptor extends Descriptor      */     private function getCommandData(Command $command)     {-        $command->getSynopsis();         $command->mergeApplicationDefinition(false);         return array(diff --git a/src/Symfony/Component/Console/Descriptor/MarkdownDescriptor.php b/src/Symfony/Component/Console/Descriptor/MarkdownDescriptor.phpindex 2eb9944..3af4a15 100644--- a/src/Symfony/Component/Console/Descriptor/MarkdownDescriptor.php+++ b/src/Symfony/Component/Console/Descriptor/MarkdownDescriptor.php@@ -89,7 +89,6 @@ class MarkdownDescriptor extends Descriptor      */     protected function describeCommand(Command $command, array $options = array())     {-        $command->getSynopsis();         $command->mergeApplicationDefinition(false);         $this->write(diff --git a/src/Symfony/Component/Console/Descriptor/TextDescriptor.php b/src/Symfony/Component/Console/Descriptor/TextDescriptor.phpindex 719f238..cc12aac 100644--- a/src/Symfony/Component/Console/Descriptor/TextDescriptor.php+++ b/src/Symfony/Component/Console/Descriptor/TextDescriptor.php@@ -134,8 +134,6 @@ class TextDescriptor extends Descriptor      */     protected function describeCommand(Command $command, array $options = array())     {-        $command->getSynopsis(true);-        $command->getSynopsis(false);         $command->mergeApplicationDefinition(false);         $this->writeText('<comment>Usage:</comment>', $options);diff --git a/src/Symfony/Component/Console/Descriptor/XmlDescriptor.php b/src/Symfony/Component/Console/Descriptor/XmlDescriptor.phpindex b5676be..69e5a25 100644--- a/src/Symfony/Component/Console/Descriptor/XmlDescriptor.php+++ b/src/Symfony/Component/Console/Descriptor/XmlDescriptor.php@@ -59,7 +59,6 @@ class XmlDescriptor extends Descriptor         $dom = new \DOMDocument('1.0', 'UTF-8');         $dom->appendChild($commandXML = $dom->createElement('command'));-        $command->getSynopsis();         $command->mergeApplicationDefinition(false);         $commandXML->setAttribute('id', $command->getName());

It still looks very weird to call getters and not doing anything with them. Right now it's just getters with side effects it seems

Agree, but at least itlooks weird in 1 place only :)

If you have trouble with my SRP definition, just leave that argument as it is.

Premature optimization is the root of all evil (Donald Knuth).
There is no lazy loading. There is lazy initializing. Lazy initializing do not make sense for scalar values. It adds complexity while not inproving performance or memory footprint at a way that it payback a benefit.

The side effect of a getter is worse and avoids POLS / POSA.
https://en.wikipedia.org/wiki/Principle_of_least_astonishment.

To remove the DX issue just move the init stuff forward to a init method or to the constructor. Or give a valid reason to delay the initialization.

Agree this all is not designed perfectly and perhaps violates one or more patterns in one or more ways. But lets be pragmatic.. the intention of the code is clear by now?

Imo. the main problem ismergeApplicationDefinition violating SRP for being depended on callinggetSynopsis first. See my diff that fixes that.. we can argue if thisgetSynopsis call really needs to be renamed toinitSynopsises/whatever whereas the result will be the same.

edit: im not sure what the effect will be on

There is no lazy loading. There is lazy initializing. Lazy initializing do not make sense for scalar values. It adds complexity while not inproving performance or memory footprint at a way that it payback a benefit.

The goal is not to optimize memory, but to avoid computing the synopsis for all cases which don't need it (it is needed only when displaying the help, not when actually running the command).$this->definition->getSynopsis($short) involves quite some logic.

@stof wouldmergeApplicationDefinitionWithSynopsis make things more clear perhaps?

Closing in favor of#20054