Skip to content

[TypeDeclaration] Handle crash on concat param append returned used on Arg on StrictStringParamConcatRector#4774

Merged
samsonasik merged 6 commits intomainfrom
concat-return
Aug 11, 2023
Merged

[TypeDeclaration] Handle crash on concat param append returned used on Arg on StrictStringParamConcatRector#4774
samsonasik merged 6 commits intomainfrom
concat-return

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@samsonasik
Copy link
Copy Markdown
Member Author

Fixed 🎉 /cc @smortexa

function formatSizeUnits($bytes): string
{
if ($bytes >= 1_073_741_824) {
$bytes = number_format($bytes / 1_073_741_824, 1).' GB';
Copy link
Copy Markdown
Member Author

@samsonasik samsonasik Aug 11, 2023

Choose a reason for hiding this comment

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

here $bytes re-assign and used as Arg need to be float, that's why it need to be skipped as possible non string type.

@samsonasik samsonasik changed the title [TypeDeclaration] Handle crash on concat param append returned on StrictStringParamConcatRector [TypeDeclaration] Handle crash on concat param append returned used on Arg on StrictStringParamConcatRector Aug 11, 2023
@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 @TomasVotruba I am merging it ;)

@samsonasik samsonasik merged commit 3f863a7 into main Aug 11, 2023
@samsonasik samsonasik deleted the concat-return branch August 11, 2023 17:49
@smortexa
Copy link
Copy Markdown

@samsonasik Thanks. The issue is fixed.

Comment on lines +162 to +168
private function isVariableInArg(Arg $arg, string $paramName): bool
{
return (bool) $this->betterNodeFinder->findFirst(
$arg->value,
fn (Node $subNode): bool => $subNode instanceof Variable && $this->isName($subNode, $paramName)
);
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@TomasVotruba I think we can move this handling to a guard service to be reused when parameter used as Arg value in diferent rules, another rule that has same issue is:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Variable Param reassign check probably better instead of Arg check, I will check tomorrow.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

Return value must be of type ?PhpParser\Node\Expr\Variable, PhpParser\Node\Expr\AssignOp\Concat returned"

2 participants