[Console] Throw an exception if the Command __construct() method is not called #9186
Conversation
It can only happend if the constructor has been overridden
…ntain aliases (lyrixx) This PR was merged into the master branch. Discussion ---------- [Console] Throw an exception if the command does not contain aliases | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | license? | MIT It can only happend if the constructor has been overridden Commits ------- 7e5c901 [Console] Throw an exception if the command does not contain aliases
| @@ -407,6 +407,10 @@ public function add(Command $command) | |||
| return; | |||
| } | |||
|
|
|||
| if (null === $command->getAliases()) { | |||
| throw new \InvalidArgumentException(sprintf('You must call the parent constructor in "%s::__construct()"', get_class($command))); | |||
There was a problem hiding this comment.
InvalidArgumentException seems inappropriate here. UnexpectedValueException would be better.
|
I think adding this code is a bad idea. Do we want to add such code in all classes of symfony to see if the constructor has been called?! |
|
@Tobion it is very very hard, for the end user, to find this bug. So here, it is |
|
Exactly, it's a hack. This is why 👎 for me. getAliases returns an array according to the phpdoc. So it should not even be allowed to return null. |
|
If someone fixes that getAliases cannot return null, it will break suddenly... |
|
What feature ? |
|
It based on implementation detail not based on API. Cannot say more. |
|
Btw, it's also a BC break that should be documented. For example when I override the public methods in my command, I don't need to call the parent constructor. Now my console will be broken by this exception suddenly. |
|
@Tobion If you are not overriding the constructor, you will not get this exception as the inherited constructor is used. |
|
@stof wrong. I can override the constructor without calling the parent, when I also override the public methods. This is absolutly valid. After this change, it does not work anymore. |
|
@Tobion If you overwrite the method but respects its API, you should not reach this path either as it should return an array |
|
I will if I did not override getAliases() because I don't want any. |
|
@stof so you think this implementation is good? |
|
Well, it is probably not the best way to do this. Ideally, we should not need to warn users about forgetting to call the parent constructor, but many users don't think about it when overwriting a constructor |
…ion) This PR was merged into the master branch. Discussion ---------- [Console] make parent constructor test more reliable | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Fixes | #9186 , f2b60e9 | Tests pass? | yes | license? | MIT It also fixes the test since f2b60e9 and improves phpdoc The second commit improves regex performance to validate name (using possesive quantifier). I did some basic performance tests http://3v4l.org/PuvuL The new regex only takes 1/3 of the time compared to the old one! Commits ------- 5798029 [Console] improve regex performance to validate name 22b09ce [Console] make parent constructor test more reliable
It can only happend if the constructor has been overridden