[new] FQCN to ::class

[veewee]

Hello,

Thanks for your great plugin! I've got a suggestion for a new inspection:

In frameworks like Zend Framework 2, there are a lot of configuration files looking like this:

namespace Application; 

return [
    'service_manager' => [
        'factories' => [
           'Application\EventListener\SomeListener' => 'Application\EventListener\Factory\SomeListener',
        ],
    ]
];

It would be great if there is an inspection that detects this FQCN and converts it to something like this:

namespace Application; 

return [
    'service_manager' => [
        'factories' => [
           EventListener\SomeListener::class => EventListener\Factory\SomeListener::class,
        ],
    ]
];

Another option would be to import all the use statements like this:

namespace Application; 

use Application\EventListener\SomeListener;
use Application\EventListener\Factory\SomeListenerFactory;

return [
    'service_manager' => [
        'factories' => [
           SomeListener::class => SomeListenerFactory::class,
        ],
    ]
];

With this code there is the problem that both classes can't have the same classname even though they are in another namespace. This inspection could also apply to the default phpspec specification classes that are being generated.

For older projects, it should be possible to run this inspection by name on the full codebase.

[kalessil]

Hello,

You are welcome, glad that you found it useful )

I like the idea, it seems that for Zend2/Symfony2/Symfony3 I can filter file names with config//*.php mask. The rest already exists in PhpUnit inspections.

[veewee]

I don't think this is something that should be config or framework related. These replacements can also occur in other code. For example:

Phpspec:

class ClassSpec extends ObjectBehavior
{
    function it_is_initializable()
    {
        $this->shouldHaveType('Some\Class');
    }
}

Reflection:

$rc = new ReflectionClass('Some\\Class\\Name');

There are propably tons of other places where these strings can occur.

[kalessil]

It means every single string has to be checked, I wondering if this going to hit IDE performance...

[veewee]

I tried solving the issue with both regex find / replace and structural search and it looked pretty fast. But indeed: This is something that might need some benchmarking.

[kalessil]

Implemented the first version with Quick-Fix (UTing still ahead), but you can use the binary from master already for testing the feature.

[veewee]

Great, thanks for the work! Can you tell me how to test the master version in PHP storm? I'll try it out tomorrow or Monday.

[kalessil]

Download .jar file from the branch, then in IDE settings: Plugins, click Install plugin from disk (under plugins list), select the downloaded .jar and restart IDE.

[kalessil]

I pushed fresh binary with bug-fixes, 100% done here.

[veewee]

I just installed the plugin on the master branch and checked some projects that I recently refactored. When I run inspection by name on a phpspec folder, following behaviour occurs:

---
string: "SplFileObject"
AS-IS: inspection not available
TO-BE: \SplFileObject::class
STATUS: NOK!
---
string: "Some\Known\ClassName"
AS-IS: inspection not available
TO-BE: \Some\Known\ClassName::class
STATUS: NOK!
---
string: "\Some\Known\ClassName"
AS-IS: \Some\Known\ClassName::class
TO-BE: \Some\Known\ClassName::class
Extra: In this case, you might want to import the classname in the use statements and use the class shortname automatically.
STATUS: OK!
---
string: "\\Some\\Known\\ClassName"
AS-IS: \Some\Known\ClassName::class
TO-BE: \Some\Known\ClassName::class
STATUS: OK!
---
string: "\SplFileObject"
AS-IS: \SplFileObject::class
TO-BE: \SplFileObject::class
Extra: In this case, you might want to import the classname in the use statements and use the class shortname automatically.
STATUS: OK!
---
string: "SplFileInfo"
context: Following use statement is added: "use Symfony\Component\Finder\SplFileInfo"
AS-IS: SplFileInfo::class
TO-BE: \SplFileInfo::class
Extra: In this case the inspection detects the class shortname, but in this case there are 2 different classes: the Symfony SplFileInfo class and the built-in SPlFilenfo class
STATUS: NOK!
---
string: "Some\UnKnown\ClassName"
AS-IS: inspection not available
TO-BE: inspection not available
STATUS: OK!
---

I'll also try with a Zend Framework 2 project today or tomorrow, but that would probably result in the same list as mentioned above.

Thanks for the work so far! :)

[kalessil]

Are these cases located in a file without a namespace declaration?

---
string: "SplFileObject"
AS-IS: inspection not available
TO-BE: \SplFileObject::class
STATUS: NOK!
---
string: "Some\Known\ClassName"
AS-IS: inspection not available
TO-BE: \Some\Known\ClassName::class
STATUS: NOK!
---
string: "SplFileInfo"
context: Following use statement is added: "use Symfony\Component\Finder\SplFileInfo"
AS-IS: SplFileInfo::class
TO-BE: \SplFileInfo::class
Extra: In this case the inspection detects the class shortname, but in this case there are 2 different classes: the Symfony SplFileInfo class and the built-in SPlFilenfo class
STATUS: NOK!

[veewee]

1. The first one is NOK in both with and without namespaces.
2. The second one is OK in non-namespaced scenarios. In namespaced scenarios, it is NOK.
3. The last one results in the same in namespaced / non namespaced scenario's.

The last one is wrong because the short classname is used twice. Since the class is used, the class is found:

E.g:

use GrumPHP\Collection\FilesCollection;

'FilesCollection';

In this case, the FilesCollection string triggers the inspection even though it is not a FQCN. This is a nice additional feature in my opinion but most of the time, the FQCN will need to be converted instead of a short class name.

[kalessil]

I see now where I got the idea wrong) So we are targeting:

• FQNs in namespace/no-namespace contexts;
• Root NS classes in no-namespace context;
• We are skipping the rest of current implementation (aliases, looking up classes in the same NS and etc.)

[kalessil]

New binary pushed, hope this will work as expected

[veewee]

@kalessil,

That seems to work great! Some small things I've noticed:

• This does not work with interfaces. An interface can also be shortened like this: \SomeInterface::class.
• It always results in a complete classname which triggers the 'unnecessary fully qualified name' inspection from "PHP | Code Style". Is it possible to run the import / simplify the intentions automatically after converting to ::class?

I've also ran this inspection an ZF2 configuration file. It is super effective! ;)

[kalessil]

Trying my best in efficiency)

• [x] will add interfaces support;
• [x] will give a try adding settings for generating use-statements;

[kalessil]

Okay, the current binary will make necessary imports creation (if exists at least one import already), if something was already imported (incl. with an alias) it will be picked up correctly.

GUI for creating imports switching on/off will be available on release.

Now: would you please share final feedback with me?

[kalessil]

Bugs:

• [x] function() use () {} constructs shall not be processed when searching for imports
• [x] imports can be created after NS declaration;

[kalessil]

Aaaaand, now we are done ^_^

[veewee]

Great! I'll run some additional tests tomorrow.

[kalessil]

Thank you, looking forward)

[veewee]

Hi @kalessil,

This feature is starting to look very good! I've found some last (minor) issues:

• "RuntimeException" is transformed to \RuntimeException::class. I would think that this should also add a use-statement and transforms to RuntimeException::class instead.
• In following case, the transfomation results in unexpected code class for the $y variable. In this case you might want to use the FQN?

  
  use SplFileInfo;

$y = new SplFileInfo('somefile.txt'); $x = '\Symfony\Component\Finder\SplFileInfo'; // ---- Converts to: ---- use SplFileInfo; use Symfony\Component\Finder\SplFileInfo;

$y = new SplFileInfo('somefile.txt'); $x = SplFileInfo::class;

- Another minor point which I don't know is something that is possible to do: In large ZF2 configuration files we like to namespace stuff based on it's base namespace or its imported base-namspace. This is to prevent that every controller is added to the use statements. In a big application, this could generate many use statements :) Maybe this could also be an option in the admin GUI?
```php
namespace Applicaton:

return [
'controllers' => [
        'invokables' => [
            'Application\Controller\SomeController' => 'Application\Controller\SomeController'
            // ---- Converted to: ------
            Controller\SomeController::class => Controller\SomeController::class
        ],
    ],
];

[kalessil]

Great)

• "RuntimeException" -> \RuntimeException::class - intended behaviour for all root NS classes. Reason is that I'd like polluting imports;
• [x] Root and Symfony NS SplFileInfo: good catch, this is bug;
• [x] Enhancement, let me check if this can be done;

[kalessil]

The last 2 minor issues being fixed. Also using relative QN by default is something I'd like to give a try)

[veewee]

I've tested this again and the relative QN is currently not working as expected:

namespace Applicaton:

return [
'controllers' => [
        'invokables' => [
            'Application\Controller\SomeController' => 'Application\Controller\SomeController',
            // ---- Converted to: ------
            SomeController::class => SomeController::class,
            // ------ Expected: -------
            Controller\SomeController::class => Controller\SomeController::class,
        ],
    ],
];

I've also tried by adding the use statement for Application\Controller, but that also does not seem te work.

About setting the relative QN by default:

• For config files like the one above, I agree: This is just some additional noise.
• For regular classes, I prefer all the use statements. This way it is clear which classes the class depends on explicitly. It gives a good idea about how many dependencies it has.

[kalessil]

I'll need a github link for the file to check what's wrong.

So you suggest using relative paths only for config/*.php files? Why not - getting your point better now)

[veewee]

The project on which I was testing is a private project, I can give you following gist as an more in-depth example: https://gist.github.com/veewee/0d70eb9ee88c24f5f7306c5d31e8a593

I am not suggesting that the path should match config/*.php. I think a checkbox would be better to toggle this feature on / off. Something like: "Do you want to use relative imports?" This way, the developer can choose how he/she wants to import the classes. Mostly I would prefer to import all the classes, but in those config files an possibly other situations, I would rather like the relative import.

[kalessil]

• Checked the sample: know already what's went wrong with fixing configs.
• Separate option: will do, arguments for having it are reasonable.

[kalessil]

Done, options are available in the inspection settings. Thank you for proposal and fast iterations @veewee )

[veewee]

@kalessil, Currently, this feature seems broken in the config file: it isn't transforming / importing any class. It does detect the class FQCN when i run inspection by name. Currently it is only transforming root namespaces in the config files. I also didn't get the relative FQCN working yet. When I retry by unchecking the relative FQCN box, It does convert and import the classes.

So I guess this has something to do with the relative FQCNs.

[kalessil]

Weird, I did manual testing before pushing binary... Will re-test today.

[kalessil]

Found the issue: code parser generates different tree structure for namespace x and namespace x\y[\z]. Relative QN were broken due to this.

@veewee can you confirm if this working for you now?

[veewee]

Hello @kalessil,

That seems to work perfectly, Great! I see you resolved false-positive in class_exist(). In my code, I've got folloing string:

return class_exists('Symfony\Component\Yaml\Yaml');

The ::class inspection does not show up there. Is this a bug?

[kalessil]

Yohoo, it was good piece of work here)

The inspection shouldn't fire there: context meaning that class might not exist, but using ::name will trigger autoloading of potentially non-existing class. So class_exists context is not reported by the inspection.

[veewee]

I can imagine! It was a bumpy road :) The ::class operator does NOT trigger autoloading the class. So it should also work in class_exist() context. something::class outputs "something" even if the class does not exist.

[kalessil]

I guess it was the last bump: confirming, you are 100% right about the case - fixing this

[kalessil]

Done

[veewee]

Super, it looks like everything gets converted correctly! Thanks for your hard work on this feature! This one will sure come in handy while actively pushing some projects to the latest PHP versions!