Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect behavior of ConsistentImplodeRector #7903

Closed
nerones opened this issue Apr 27, 2023 · 1 comment · Fixed by rectorphp/rector-src#3702
Closed

Incorrect behavior of ConsistentImplodeRector #7903

nerones opened this issue Apr 27, 2023 · 1 comment · Fixed by rectorphp/rector-src#3702
Labels

Comments

@nerones
Copy link

nerones commented Apr 27, 2023

Bug Report

I'm not sure how the rule should handle this case. If you try to refactor implode('string a', 'string b' ) the rule will not change the order of the arguments, that is fine, the code will throw an error anyway.

But in my case I have a variable as second argument: implode(' ' . $separator, $anArray), $anArray is an array but due to some errors in annotations phpstan thinks that it is a string, so the rule is changing the order of the arguments and causing errors. I know that if I fix the problem with the annotations the rule work as expected.

So, I was thinking that the correct behavior for this case it's to not change the order of the arguments since it's something unexpected, but I'm not sure.

I will fix the annotations in my code, this will not be a problem for me. But I wanted to report this to see if it's a case that needs to be handled.

Subject Details
Rector version last dev-main
Installed as composer dependency

Minimal PHP Code Causing Issue

See https://getrector.com/demo/ec054f63-0f4e-489d-a3da-fa87581814f0

<?php

final class DemoFile
{
    public function run()
    {
        implode('|' . ' ', 'should not be changed?');
    }
}

Responsible rules

  • ConsistentImplodeRector

Expected Behavior

The order of the arguments should not change.

@nerones nerones added the bug label Apr 27, 2023
@TomasVotruba
Copy link
Member

Thank you for your report and demo link!

We'll need a failing test case in a pull-request, so we have it covered in Rector.
You can click "Create a Test" button at demo page.

nerones added a commit to nerones/rector-src that referenced this issue Apr 28, 2023
samsonasik pushed a commit to rectorphp/rector-src that referenced this issue May 2, 2023
)

* Add failing test fixture for ConsistentImplodeRector

# Failing Test for ConsistentImplodeRector

Based on https://getrector.com/demo/ec054f63-0f4e-489d-a3da-fa87581814f0

Reported in rectorphp/rector#7903

* Use the type to check for string in ConsistentImplodeRector

* Change fixture name
samsonasik pushed a commit to rectorphp/rector-src that referenced this issue May 8, 2023
)

* Add failing test fixture for ConsistentImplodeRector

# Failing Test for ConsistentImplodeRector

Based on https://getrector.com/demo/ec054f63-0f4e-489d-a3da-fa87581814f0

Reported in rectorphp/rector#7903

* Use the type to check for string in ConsistentImplodeRector

* Change fixture name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants