Skip to content

Add only_direct_assign config for SimplifyUselessVariableRector#6034

Merged
TomasVotruba merged 1 commit intorectorphp:mainfrom
ruudk:SkipMultiConcatString
Jun 26, 2024
Merged

Add only_direct_assign config for SimplifyUselessVariableRector#6034
TomasVotruba merged 1 commit intorectorphp:mainfrom
ruudk:SkipMultiConcatString

Conversation

@ruudk
Copy link
Copy Markdown
Contributor

@ruudk ruudk commented Jun 25, 2024

When a variable is assigned using concatenation it should not simplify the variable.

This looks weird:

 $content = 'Hello, ';
-$content .= 'World!';

-return $content;
+return $content . 'World!';

It would be better to keep the original:

$content = 'Hello, ';
$content .= 'World!';

return $content;

@TomasVotruba
Copy link
Copy Markdown
Member

Sounds good 👍 Could you send a feature as well together?

@samsonasik
Copy link
Copy Markdown
Member

@ruudk @TomasVotruba I think this should be configurable, eg: only_direct_assign with default to false so original opiniated functionality still works

@ruudk ruudk force-pushed the SkipMultiConcatString branch from 78d3962 to 1e0af59 Compare June 26, 2024 07:45
@ruudk ruudk changed the title Add failing test for SimplifyUselessVariableRector Add SKIP_CONCAT config for SimplifyUselessVariableRector Jun 26, 2024
@ruudk
Copy link
Copy Markdown
Contributor Author

ruudk commented Jun 26, 2024

Great, I just updated the PR with an implementation. Let me know if you prefer other naming or have any feedback. Thanks!

When a variable is assigned using concatenation it should not simplify the variable.

This looks weird:
```diff
 $content = 'Hello, ';
-$content .= 'World!';

-return $content;
+return $content . 'World!';
```

It would be better to keep the original:
```php
$content = 'Hello, ';
$content .= 'World!';

return $content;
```
@ruudk ruudk force-pushed the SkipMultiConcatString branch from 1e0af59 to 2669c07 Compare June 26, 2024 08:06
@ruudk ruudk changed the title Add SKIP_CONCAT config for SimplifyUselessVariableRector Add only_direct_assign config for SimplifyUselessVariableRector Jun 26, 2024
@ruudk ruudk requested a review from samsonasik June 26, 2024 08:11
@TomasVotruba TomasVotruba merged commit 3058379 into rectorphp:main Jun 26, 2024
@TomasVotruba
Copy link
Copy Markdown
Member

Thank you @ruudk 👍

@ruudk ruudk deleted the SkipMultiConcatString branch June 26, 2024 08:32
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.

3 participants