Incorrect behavior of PseudoNamespaceToNamespaceRector

[zimzat]

Bug Report

When PseudoNamespaceToNamespaceRector renames a class and adds a namespace declaration it doesn't maintain references to other unprefixed classes (e.g. below DateTimeInterface becomes invalid and becomes a reference to Do\Stuff\DateTimeInterface instead).

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

Minimal PHP Code Causing Issue

See https://getrector.org/demo/e326b671-4568-4929-8b12-efb661d68596

<?php

class Do_Some_Stuff
{
    public function test(): DateTimeInterface
    {
        return new DateTimeImmutable('now');
    }
}

{
    "autoload": {
        "psr-4": {
            "Do\\Some\\": "src/"
        }
    }
}

• With only NormalizeNamespaceByPSR4ComposerAutoloadRector it handles other class references correctly but the namespace + class becomes Do\Some\Do_Some_Stuff.
• If both rules are specified then only PseudoNamespaceToNamespaceRector is used.
• If the autoload psr-4 is set to "\\Do\\Some\\": "src/" then it correctly prefixes other classes and shortens the actual class name, but it prefixes the namespace declaration itself with an extra \ which is a syntax error and makes all class references use a FQCN (including those covered by an existing use).
  • This may be close enough that could be manually massaged to fix the syntax error and then other rules or fixers could be used to reach the desired outcome.

1) src/Stuff.php:0

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

-use FFI\Exception;
+namespace \Do\Some;

-class Do_Some_Stuff
+use \FFI\Exception;
+class Stuff
 {
-    public function test(): DateTimeInterface
+    public function test(): \DateTimeInterface
     {
-        return new DateTimeImmutable('now');
+        return new \DateTimeImmutable('now');
     }

     public function uhoh(): void
     {
-        throw new Exception('Test');
+        throw new \FFI\Exception('Test');
     }
 }
    ----------- end diff -----------

Applied rules:
 * NormalizeNamespaceByPSR4ComposerAutoloadRector
 * PseudoNamespaceToNamespaceRector

Responsible rules

• PseudoNamespaceToNamespaceRector
• NormalizeNamespaceByPSR4ComposerAutoloadRector

Expected Behavior

Ideally PseudoNamespaceToNamespaceRector is all that would be necessary to do what it already does and prefix all other class references not already covered by a use statement with \.

[TomasVotruba]

Hi, thanks for reporting 👍

For more complex issues like this, we need a reproducible repository. I saw you've invested way too much energy, so I'll make one myself based on your input. Then we'll take it from there.

[TomasVotruba]

I've created the repository here: https://github.com/TomasVotruba/rector-underscore-to-psr4

I'm testing the NormalizeNamespaceByPSR4ComposerAutoloadRector rule and it does what is supposed to do.
It changes namespace to match the definition in composer.json > psr-4. It does not touch the short class.

+namespace Do\Some;

 final class Do_Some_Stuff
 {
-    public function test(): DateTimeInterface
+    public function test(): \DateTimeInterface
     {
-        return new DateTimeImmutable('now');
+        return new \DateTimeImmutable('now');
     }
 }

👍

Yet, this rule does not matter for your use case at all. We won't use it.

---

For second rule, PseudoNamespaceToNamespaceRector, it changes class references from underscores to slashes. First, it was created to ease the PHPUnit upgrade:

https://github.com/rectorphp/rector-phpunit/blob/139b5da303844c08bc3f6cc395136a9d8dcc0803/config/sets/phpunit60.php#L32-L43

    $services->set(PseudoNamespaceToNamespaceRector::class)
        ->configure([
            new PseudoNamespaceToNamespace('PHPUnit_');
        ]);

↓

-class MyTest extends PHPUnit_Framework_TestCase {}
+class MyTest extends \PHPUnit\Framework\TestCase {}

It applies to use of code, not the classes. You cannot refactor your own code with it, as it's not deterministic where the class should be located after unwrapping the underscore namespaces.

We will not need this rule, as your use case is different.

---

It seems your use case kinda combines both rules together.

If I understand it correctly, you need to:

• unwrap the class names underscores to slahed name
• move the class to nested directory
• rename all class references

No Rector rule can do it at the moment. You'd have to create a new rule, that would handle the 1st step, unwrapping of namespaces to directory. Then generatate the list of renamed classes, and run RenameClassRector on it.

[TomasVotruba]

This is how such rule would look like: TomasVotruba/rector-underscore-to-psr4#1

[zimzat]

Thank you for the detailed responses and example repository. I don't have the energy to get into the details tonight but I'll check these out some time tomorrow and get back to you then. Again, thank you for following up from here and Reddit.

[zimzat]

Hi Tomas, I've read through your reply and the two example repositories.

One thing I should clarify is that, prior to the discovery of invalid classes after the transformation was applied, there wasn't an expectation that the file would move. The existing code was using PSR-0 autoloading so dir/Do/Some/Stuff.php matched to Do_Some_Stuff with a composer PSR-0 declaration of "": "dir/" (yeah, not an ideal way to declare it but that was done prior to my joining the team)

The NormalizeNamespaceByPSR4ComposerAutoloadRector usage outlined makes sense when starting with a library that only used global names with the primary purpose being to introduce a vendor\library prefix. 👌

If PseudoNamespaceToNamespaceRector isn't intended to be used on class definitions then it would be good to call that out in the documentation as well as prevent it from adding the namespace declaration to the file it modifies (see demo link in issue description). I think that's what led me to think it should work.

In the fixture for UnwrapUnderscoreClassToNamespacedClassRector it doesn't demonstrate if other existing references (e.g. to DateTime) would be prefixed with \ or left as-is. However, I appreciate the effort you put into demonstrating it, given that our classes will generate invalid namespaces and/or class names we'll need to jump straight to RenameClassRector to handle those problems first so there will be more steps involved anyway.

It sounds like, other than maybe clarifying the documentation on the Pseudo rector, that this issue can be closed as "working as intended / unsupported scenario". ¯\_(ツ)_/¯ I'll take it from here when I revisit this in a week or two. Thank you again for your effort and following up!

[TomasVotruba]

Thanks for reaching out. I wish we got a question issue before you started, so I could save you from the suffering. Sometimes I struggle with other PHP tools (all the dev dependencies) for hours or days, then I pose the question in despair on GitHub/Twitter. Just to learn there is a new 3rd solution to my problem.

It might help (also for other renaming rules) to grasp the philosophy of Rector. It does not replace PHPStorm. If we need to rename single class itself and all its references, we use refactor -> rename class.

But if we upgrade from PHPUnit 4 to 8 and somebody else renamed 50 classes, here is where Rector helps. This also applies for rename method, rename property etc.

I hope this will help with use of other rules. If you find better wording for the mentioned rules docs, please send an improvement, to clarify it for future developers.

If you have more refactoring you need help in your project, reach me via email. I'll share what I know to automate the process and hopefully, make your life easier 👍