Skip to content

[Symfony] Add ConsoleExecuteReturnIntRector#2132

Merged
TomasVotruba merged 5 commits intorectorphp:masterfrom
keulinho:add-console-execute-return-int-rector
Oct 14, 2019
Merged

[Symfony] Add ConsoleExecuteReturnIntRector#2132
TomasVotruba merged 5 commits intorectorphp:masterfrom
keulinho:add-console-execute-return-int-rector

Conversation

@keulinho
Copy link
Copy Markdown
Contributor

Fixes: #2119

Comment thread packages/Symfony/src/Rector/Console/ConsoleExecuteReturnIntRector.php Outdated
@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented Oct 11, 2019

Do you plan to handle no return in execute() as well?

E.g.

 public function execute($input, $output)
 {
     $output->write('hi');
+    return 0;
 }

@keulinho
Copy link
Copy Markdown
Contributor Author

That should already be covered.

Copy link
Copy Markdown
Member

@TomasVotruba TomasVotruba left a comment

Choose a reason for hiding this comment

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

I mean this process:

if ($staticType instanceof MixedType) {
    // no idea → skip
}

if ($staticType instanceof IntegerType) {
   // already done → skip
}

// change the rest propperly

Comment thread packages/Symfony/src/Rector/Console/ConsoleExecuteReturnIntRector.php Outdated
Comment thread packages/Symfony/src/Rector/Console/ConsoleExecuteReturnIntRector.php Outdated
Comment thread packages/Symfony/src/Rector/Console/ConsoleExecuteReturnIntRector.php Outdated
Comment thread packages/Symfony/src/Rector/Console/ConsoleExecuteReturnIntRector.php Outdated
@TomasVotruba TomasVotruba changed the title Add ConsoleExecuteReturnIntRector [Symfony] Add ConsoleExecuteReturnIntRector Oct 13, 2019
@TomasVotruba
Copy link
Copy Markdown
Member

Related Symfony PR: symfony/symfony#33775

@TomasVotruba
Copy link
Copy Markdown
Member

TomasVotruba commented Oct 13, 2019

Btw, only numeric values are converted to int:
https://github.com/symfony/symfony/pull/33775/files#diff-8e80595663798837db1b033187835535R262

It seems like anything else will return 0:

-return 'hey';
+return 0;
-return '13';
+return 13;

@keulinho
Copy link
Copy Markdown
Contributor Author

So what do you suggest?
instead of always casting the return value we could use something like:

- return $this->doSomething();
+ $result = $this->doSomething();
+ is_numeric($result) ? return (int) $result : return 0;

@TomasVotruba
Copy link
Copy Markdown
Member

I suggest to change the value only if know what is returned, but that's just an detail.

But moreover, we should merge this PR as it is now, as there is lot of opened work. So next step is just small iteration

@TomasVotruba
Copy link
Copy Markdown
Member

Could you just make CI pass? It's ready then

@keulinho
Copy link
Copy Markdown
Contributor Author

👍
CI did pass, merging is blocked because you've requested changes ;)

@TomasVotruba
Copy link
Copy Markdown
Member

Few moments I saw if fail, probably miss click.
All right then :)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Symfony 4.4] Command must return int

2 participants