-
-
Notifications
You must be signed in to change notification settings - Fork 374
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
[AutoImport] [Renaming] Skip remove used use statement on annotation during rename + auto import when no replacement on auto import #5168
Conversation
8feb910
to
80cd2a4
Compare
@jambonfarci @stefantalen it fixed by skip remove use statement when no replacement during auto import cd3ee41 🎉 |
// nothing to remove, as no replacement | ||
if ($useImportTypes === []) { | ||
return $use; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on auto import enabled, when nothing to replace, the removing use statement should not happen, as it cause invalid removal on rename process.
$classRenamingPostRector, | ||
// priority: 600 | ||
$nameImportingPostRector, | ||
// priority: 500 | ||
// priority: 600 | ||
$useAddingPostRector, | ||
// priority: 500 | ||
$classRenamingPostRector, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the re-index to set name importing and use adding post rector first is on purpose to know that auto import collecting the statements that need to be added, which class renaming depends.
All checks have passed 🎉 @TomasVotruba I am merging it to have faster feedback to test ;) |
👍 |
@TomasVotruba @jambonfarci @stefantalen I tested in laminas-servicemanager-migration usage on auto import usage, it seems cause error: There were 5 failures:
1) LaminasTest\ServiceManager\Migration\Rector\RenameClassRector\AutoImportTest::test with data set #0 ('/Users/samsonasik/www/laminas...hp.inc')
Failed on fixture file "factory_invoke_typehint.php.inc"
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
namespace LaminasTest\ServiceManager\Migration\Rector\RenameClassRector\FixtureAutoImport;
-use Psr\Container\ContainerInterface;
+use Interop\Container\ContainerInterface;
class FactoryInvokeTypehint
{
- public function __invoke(ContainerInterface $container)
+ public function __invoke(\Psr\Container\ContainerInterface $container) which auto import should be allowed, this definitely need rework, I will check if the original allow change can be solution, but on consequency,
instead of
Give me time :) |
Reverting at #5179 |
* Revert "[DX] Sync order of PostFileProcessor dependencies (#5176)" This reverts commit 4f50ad5. * Revert "[PostRector] Fix ClassRenamingPostRector return when no auto import replacement (#5175)" This reverts commit d14ec5e. * Revert "[PostRector] Reduce loop on ClassRenamingPostRector (#5174)" This reverts commit a9908f6. * Revert "[CodingStyle] Clean up check last name on UseImportsRemover (#5173)" This reverts commit c5d3a0e. * Revert "[AutoImport] [Renaming] Skip remove used use statement on annotation during rename + auto import when no replacement on auto import (#5168)" This reverts commit 29370c7.
Added test or testing skip remove use statement part of rename during auto import.
The rename annotation is allowed only on specific:
see
rector-src/packages/BetterPhpDocParser/PhpDocManipulator/PhpDocClassRenamer.php
Lines 36 to 38 in d55a35b
see
on auto import, the process is overlapped, which annotation not renamed, but use statement removed during auto import.