NormalizeNamespaceByPSR4ComposerAutoloadRector does not update references

[jtojnar]

Bug Report

Subject	Details
Rector version	0.13.5

I am trying to migrate some very old projects (e.g. https://github.com/RSS-Bridge/rss-bridge) to PSR-4 and NormalizeNamespaceByPSR4ComposerAutoloadRector adds the namespaces correctly but it does not seem to update the references to the moved classes, not even with ClassRenamingPostRector.

2 files with changes
====================

1) src/Foo/Foo.php:0

    ---------- begin diff ----------
@@ @@
 <?php

+namespace Test\Foo;
+
 class Foo {
-    public function test(): Bar {
-        return new Bar;
+    public function test(): \Bar {
+        return new \Bar;
     }
 }
    ----------- end diff -----------

Applied rules:
 * NormalizeNamespaceByPSR4ComposerAutoloadRector


2) src/Bar.php:0

    ---------- begin diff ----------
@@ @@
 <?php

+namespace Test;
+
 class Bar {
-    public function test(): Foo {
-        return new Foo;
+    public function test(): \Foo {
+        return new \Foo;
     }
 }
    ----------- end diff -----------

Applied rules:
 * NormalizeNamespaceByPSR4ComposerAutoloadRector

Minimal PHP Code Causing Issue

https://github.com/jtojnar/repro/tree/f0bf15d733116050f1526140c6b9210b5beeea87/rector-7232

Expected Behaviour

The references should be updated appropriately.

[samsonasik]

That's seems expected, the Bar was non-namespaced, so keep as non-namespaced during migration of namespace to avoid invalid assumption, eg: using stdClass changed to \Test\stdClass.

You may need to use RenameClassRector for that.

[jtojnar]

Using RenameClassRector would be quite hard as there will be hundreds of classes moved. I would expect Rector to notice that it moved class Bar into a namespace so it should also update all references to it from non-namespaced classes.

And it is not even limited to non-namespaced classes. First time I noticed this was in a project which used the same namespace App\Model namespace for app/Model/{CategoryData.php,Orm/{Orm.php,Page/Page.php}}, see the simplified example:

2 files with changes
====================

1) src/Foo/Foo.php:0

    ---------- begin diff ----------
@@ @@
 <?php

-namespace Test;
+namespace Test\Foo;

 class Foo {
-    public function test(): Bar {
-        return new Bar;
+    public function test(): \Test\Bar {
+        return new \Test\Bar;
     }
 }
    ----------- end diff -----------

Applied rules:
 * NormalizeNamespaceByPSR4ComposerAutoloadRector


2) src/Bar/Bar.php:0

    ---------- begin diff ----------
@@ @@
 <?php

-namespace Test;
+namespace Test\Bar;

 class Bar {
-    public function test(): Foo {
-        return new Foo;
+    public function test(): \Test\Foo {
+        return new \Test\Foo;
     }
 }
    ----------- end diff -----------

Applied rules:
 * NormalizeNamespaceByPSR4ComposerAutoloadRector

[samsonasik]

I tried that, the \Test\Bar is not exists yet in autoload during migration, so it will be an assumption for Bar is in inner namespace or existing migrated class, eg, check the following:

https://github.com/rectorphp/rector/blob/7a181db4cecb3d797a55b993c06792423915cd25/rules/PSR4/NodeManipulator/FullyQualifyStmtsAnalyzer.php#L51

with:

-    public function process(array $stmts): void
+    public function process(array $stmts, string $expectedNamespace): void
     {
         // no need to
         if ($this->parameterProvider->provideBoolParameter(Option::AUTO_IMPORT_NAMES)) {
@@ -39,7 +40,7 @@ final class FullyQualifyStmtsAnalyzer
         }
 
         // FQNize all class names
-        $this->simpleCallableNodeTraverser->traverseNodesWithCallable($stmts, function (Node $node): ?FullyQualified {
+        $this->simpleCallableNodeTraverser->traverseNodesWithCallable($stmts, function (Node $node) use ($expectedNamespace): ?FullyQualified {
             if (! $node instanceof Name) {
                 return null;
             }
@@ -53,10 +54,39 @@ final class FullyQualifyStmtsAnalyzer
                 return null;
             }
 
+            if ($this->isNativeClass($node)) {
+                return new FullyQualified($name);
+            }
+
+            if (str_contains($name, '\\')) {
+                return new FullyQualified($name);
+            }
+
+            $parent = $node->getAttribute(AttributeKey::PARENT_NODE);
+            if ($parent instanceof ConstFetch) {
+                return new FullyQualified($name);
+            }
+
+            if ($this->reflectionProvider->hasClass($expectedNamespace . '\\' . $name)) {
+                return new FullyQualified($expectedNamespace . '\\' . $name);
+            }
+
             return new FullyQualified($name);
         });
     }
 
+    private function isNativeClass(Name $name): bool
+    {
+        $className = $name->toString();
+
+        if (! $this->reflectionProvider->hasClass($className)) {
+            return false;
+        }
+
+        $classReflection = $this->reflectionProvider->getClass($className);
+        return $classReflection->isBuiltin();
+    }

above, the check of namespaced reference name is goes to false:

+            if ($this->reflectionProvider->hasClass($expectedNamespace . '\\' . $name)) {
+                return new FullyQualified($expectedNamespace . '\\' . $name);
+            }

without that check, it wil be always an assumption.

[samsonasik]

I think the solution is to add configurable , eg $migrateInnerClassReference which default to false so it can be enabled when needed.

[samsonasik]

I am trying create PR https://github.com/rectorphp/rector-src/pull/2480 for it.

[jtojnar]

I will try to implement the multi-phase approach mentioned in https://github.com/rectorphp/rector-src/pull/2480#issuecomment-1153140368

[jtojnar]

Do you think the approach outlined in https://github.com/rectorphp/rector-src/pull/2482 is feasible?

[samsonasik]

I am closing it as the rule no longer exists.