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

Fix UnusedImportRemovingPostRector for different letter case #6350

Merged

Conversation

jorgsowa
Copy link
Contributor

@jorgsowa jorgsowa commented Oct 3, 2024

Fixes the UnusedImportRemovingPostRector rule when imported namespace doesn't match the letter case in the code, by comparing all names in lowercase.

Example:

<?php

use Json;


class Json {
}

var_dump(JSON::class);

https://getrector.com/demo/f2508805-2d69-4792-908b-3555589439e1

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@samsonasik
Copy link
Member

Let's give it a try, thank you @jorgsowa

@samsonasik samsonasik merged commit 447afc1 into rectorphp:main Oct 4, 2024
36 checks passed
@samsonasik
Copy link
Member

@jorgsowa Ooops, it seems cause skipped valid removal for constant use, which allow to have case sensitive naming, see

I will check if it resolvable, or just revert it, this is why we don't compare strtolower on constant import as case sensitive import is allowed for constant

// don't compare strtolower for use const as insensitive is allowed, see https://3v4l.org/lteVa
if ($fileConstantUseImportType->getShortName() === $shortName) {
return true;
}

@samsonasik
Copy link
Member

samsonasik commented Oct 5, 2024

@jorgsowa sorry, I am going to revert it, as it cause skipped valid removal, removeUnusedImports is optional, and I think you can cherry-pick part by part on your code base if you want to apply it, then fix to correct case after that.

@samsonasik
Copy link
Member

Reverting at #6354

@jorgsowa
Copy link
Contributor Author

jorgsowa commented Oct 7, 2024

Thanks for checking. I will continue the investigation in another PR: #6362

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.

None yet

3 participants