feat(EXPERIMENTAL): ClassKeywordFixer

[keradus]

creates ages ago, yet problematic due to keradus@33be691#commitcomment-23110574
if you have any nice idea how to deal with it, feel free to propose.

[TomasVotruba]

I'm glad you've created this PR. It will get things moving

[keradus]

Feel free to collaborate

[TomasVotruba]

I'll be testing my fixer for edge cases at the moment, that's fair enough for me.
Feel free to take anything from it

[keradus]

sadly, not solving any of mentioned issues.

[Slamdunk]

if you have any nice idea how to deal with it, feel free to propose.

Well, the following is not a solution, but an idea.
As you pointed out, it is impossible to make this fixer content-unaware.
https://github.com/Symplify/CodingStandard could not be a solution neither, because of the same issue.

What we use in our business is to put the logic/algorithm in a unit-test, that can be inserted in the SUT of the specific project. Any SUT must be content-aware by definition:
https://github.com/Slamdunk/phpunit-extensions#checks

In particular, this way we can have two checks, that can be (I don't know how) turned in a fixer:

1. Classes must be referred with ::class keyword instead of strings
2. MyClass::class aliases must refer to real classes

I really don't know as this time how could we write a neat and simple solution for PHP-CS-Fixer, but here's how we do it.

[TomasVotruba]

Could you refer specific code lines in your package you talk about?

[Slamdunk]

The code is raw and does something that is 90% alike of your project (written long before I knew it), although is 100% tested:

1. 'Class' => Class::class https://github.com/Slamdunk/phpunit-extensions/blob/master/lib/ClassStandardsTrait.php#L242
2. Class::class existence check https://github.com/Slamdunk/phpunit-extensions/blob/master/lib/ClassStandardsTrait.php#L113

[TomasVotruba]

Thanks for sharing this, that's a lot of good work, especially with tests.

I wonder, what would be the 10% different behavior? I think Symplify Fixer respects both conditions you mentioned.

[Slamdunk]

I think we are going a bit off topic, by the way:

1. Symplify/CodingStandard takes 'Class' string, asserts that is a real class with *_exists() functions or with a regex, and then converts it to Class::class, but you cannot be sure *_exists() functions give the right result, because the autoloading of the tool may be different from the one of the project
2. Slamdunk/phpunit-extensions takes 'Class' string and asserts that is a real class with *_exists() functions, without fixing it. In its context *_exists() functions are reliable because the project autoload is used.
3. Slamdunk/phpunit-extensions takes Class::class and asserts that the alias exists; Symplify/CodingStandard done this check only for 'Class', not for Class::class. Remember that ::class notation may be used with non existen class too, PHP checks this only at runtime.

[TomasVotruba]

I think we are going a bit off topic, by the way:

I find this algorithm relevant to architecture of Class Fixer.

I see you Fixer is more advanced than mine 🙇‍♂️ . Would you be able to put that into PR Fixer?

[Slamdunk]

Would you be able to put that into PR Fixer?

No because, as said, a Fixer must be content-unaware, but this type of check needs to be content-aware.

[TomasVotruba]

I see.

Do you have an idea how to make use of your code and keep that rule to improve current situation in way?

[Slamdunk]

Yes, we could re-use a lot of PHP-CS-Fixer code in a new PHP-CS-Fixer+PHPUnit test component/plugin, in order to have checks/sniffs that are content-aware, because test suites are by definition content-aware.

[TomasVotruba]

I meant in improving this PR, this Fixer here. Or do you think there is nothing we could do about that?

[Slamdunk]

There is something we can do.

1. If PHP-CS-Fixer is installed by the composer.json of the project, then vendor/bin/php-cs-fixer binary loads the project autoload.php, and so PHP-CS-Fixer becomes content-aware
2. If, otherwise, php-cs-fixer binary is installed globally, then the autoload of the project isn't loaded and PHP-CS-Fixer remains content-unaware

We could in theory detect the two scenarios, and adapt this fixer behaviour accordingly - by throwing an error or simply dinamically disabling the single fixer - but I think @keradus considers this solution not appropriate for this project

[Jean85]

I think that option 1 (being context-aware when possible) could be a good solution, but the mantainer seems against that approach.

Maybe we can pass the project's autoloader's file path as a rule option? By default it should be always vendor/autoload.php if composer is being used and the php_cs file is in the root of the project (which seems fair assumptions to me).

[Slamdunk]

@Jean85 yup, this can be a solution, making the vendor/autoload.php option path mandatory for this Fixer.

Reminder: if we are going to do so, after requiring the autoload it should immediately be disabled, and queried without classes being effectively loaded:

$loader = require $option['vendor_autoload_path']; // Composer\Autoload\ClassLoader instance
$loader->unregister(); // because register() is calles by default while requiring
if ($loader->findFile($classString)) { // and not loadClass()
    // ...
}

So we don't mess autoloading up and can avoid runtime autoloading issues.

Of course this couple the fixer to che composer project as a dependency.

[SpacePossum]

being context-aware when possible [...] the mantainer seems against that approach.

I think you jump to conclusions here a bit :) We would very much like it, however it must be done nicely and within the scope of this project (remember it is mainly about fixing CS not a SCA tool with fix options).

Including the autoload from composer can cause problems when there a conflicts with classes already loaded/defined by the tool itself (especially when different versions are used). A better solution would be to build a AST from the project that is being fixed. However non is available that supports white space. Since we deal with white space a lot we cannot leave the Token stream solution (simply because there is no good alternative available) and having both the Tokens and the AST will become very complex.

So again, we would like it and it is discussed and looked into regularly, however a nice way to do it has not yet been found.

[Slamdunk]

Including the autoload from composer can cause problems when there a conflicts with classes already loaded/defined by the tool itself (especially when different versions are used). A better solution would be to build a AST from the project that is being fixed.

@SpacePossum wait, either I miss something or you do. Le'ts sum up a real example.

Suppose:

1. We are fixing a project using php-cs-fixer as a binary external to the project, maybe a phar, we don't know
2. The project we are going to fix is PHP-CS-Fixer itself, so potential confilcts are real: we must read project code, but we mustn't load it
3. The project of course was installed through composer install, and so a /project/vendor/autoload.php is present inside the project
4. Inside our project, e.g. inside PhpCsFixer\AbstractFixer, someone wrote $x = class_exists("PhpCsFixer\Config"); and we want to fix it

With the proposed solution what will happen is:

1. php-cs-fixer external binary loads its classes
2. Our new fixer receives /project/vendor/autoload.php as a string by the config
3. Our new fixer calls $projectLoader = require '/project/vendor/autoload.php';
4. (*) the class ComposerAutoloaderInit<RandomHash> is declared and loaded
5. (*) the function composerRequire<RandomHash> is declared and loaded
6. (*) the class Composer\Autoload\ClassLoader is loaded (but not re-declared)
7. (*) Composer\Autoload\ClassLoader::register is called
8. Our new fixer disables the freshly registered autoloader calling $projectLoader->unregister();
9. php-cs-fixer binary reads /project/PhpCsFixer/AbstractFixer.php file without loading it
10. Our new fixer receives /project/PhpCsFixer/AbstractFixer.php tokens
11. Our new fixer reads "PhpCsFixer\Config" string and triggers its checks
12. Our new fixer asks to the project loader if this string corresponds to a file that resembles a class, calling $projectLoader->findFile("PhpCsFixer\Config")
13. (*) the project loader replies that /project/PhpCsFixer/Config.php file exists, but the file is neither loaded nor readed
14. Our new fixer substitute $x = class_exists("PhpCsFixer\Config"); with $x = class_exists(\PhpCsFixer\Config::class);

(*) = statement valid with current Composer version ^1.5

As you can see

1. /project/PhpCsFixer/AbstractFixer.php isn't loaded
2. /project/PhpCsFixer/Config.php isn't loaded nor readed
3. No conflict can happen between the PhpCsFixer\Config class loaded in the php-cs-fixer binary and the one present in the project, because the project's one is never readed nor declared

But the are some issues:

1. Project's ComposerAutoloaderInit<RandomHash> and composerRequire<RandomHash> loaded by the fixer may conflict with php-cs-fixer one if the external binary is also installed with composer, but it is very unlikely to happen, we trust <RandomHash> is wisely choosen in the exact purpose two autoloader can be autoloaded together. So this is no issue.
2. Project or project's dependencies may declare autoload files, and they are instantly loaded when /project/vendor/autoload.php is required; calling $projectLoader->unregister(); doesn't affect this behaviour

The latter point may represent a real problem, but has mitigations:

1. autoload files is an infrequent pattern, and the trend is to not use them in the future in favour of PSR-4
2. if PHP-CS-Fixer and its dependencies don't have them, the only conflict is when a user declares a PHP-CS-Fixer identical class inside one of his/her autoloaded file. Very unlikely too and I don't know why someone would do something like this

Spawning a thread to do the entire $projectLoader->findFile("PhpCsFixer\Config") thing would be possible, but overkilling. A simple documentation on this edge case would be enough I think.

I don't know why you are talking about ASTs here, I see no need for them.

[Slamdunk]

Other two points came to my mind:

1. Alongside ComposerAutoloaderInit<RandomHash> and composerRequire<RandomHash> there also is ComposerStaticInit<RandomHash> but has the exact same implication, so no issue
2. If instead of being external, php-cs-fixer is run inside the project as a dependency, we need to detect it we musn't use the vendor/autoload.php provided by the config because it would be the exact one already required during php-cs-fixer binary call, and of course it will cause fatal conflict. By the way this scenario is easily interceptable and we can fallback to the normal class_exists, interface_exists and trait_exists functions instead of $projectLoader->findFile()

[SpacePossum]

Thanks for the detailed concept, I'll have a better look on it later.

I don't know why you are talking about ASTs here, I see no need for them.

Because it will provide the data we need, similar to class_exists, interface_exists and trait_exists you mentioned. The benefit is that it will also provide other information that is of interest for this project in a much easier to handle, for example if statements are hard to analyze by looking at the tokens and very easy in an AST.

[Slamdunk]

UPDATE The autoload files is not an issue since composer v1 due to this check:
https://github.com/composer/composer/blob/1.5.1/src/Composer/Autoload/AutoloadGenerator.php#L714

In the next days I would like to commit my idea: @keradus your FEAT_php55_class is 513 commits behind FriendsOfPHP:2.5 is it ok for you if I cherry-pick this PR and create a new one? Otherwise tell me what to do please 🙏

[SpacePossum]

Just a friendly warning;
it might be wise to discuss the proposed idea until an conclusion about it has been reached before putting a lot of time and effort into a PR. It might frustrating to find out it will not be accepted after a lot of work, not because the is not done nicely or the amount of time and effort is not recognized, but because it might be something that doesn't fit in the roadmap or scope of the project (or something totally else) in the view of the maintainers.

[keradus]

@Slamdunk , yeah, please do not start work yet. I pushed the idea as there were rising costum fixers for it in the ecosystem. It's out of date, it's old, idea is shown.
What is needed is not a rebase, is not finishing this fixer, but whole handling of contextual rules. big as hell.

[CDRO]

Is this feature dead?

[Wirone]

Can we close it, or do we still want it?

[coveralls]

coverage: 94.902% (-0.006%) from 94.908%
when pulling f9539db on keradus:FEAT_php55_class
into 1a4cce9 on PHP-CS-Fixer:master.

[keradus]

I decided to merge this PR.

We finally have an aligned approach how to mark fixers as EXPERIMENTAL. This one is exactly like this.

This rule is not safe to use, it will not detect all cases, and it will potentially change cases you do not want to change.

It is not planned to get this fixer out of EXPERIMENTAL zone - JohnDoe should likely NOT use this fixer at all.

The issue of "lack of context" (ie "is that class exists in given codebase) is out of scope for PHP CS Fixer and the fact that we did not bring it since 2017 (creation of this PR) leads to conclusion that we will rather never do so (and I have no intention to work on that).

I do not plan to act anyhow further on this rule other than keeping it under main project, ensuring compatibility with all changes happening around over time.

[Wirone]

Most probably should be marked as risky 😉.

[keradus]

good eye thanks

maybe isRisky => false shouldn't be in abstract class 🤔

[keradus]

-=> #7550

[Wirone]

I believe most of the rules are non-risky, so this is sane default. Probably would be too much to put it explicitly in every fixer.

[Wirone]

Not sure if var_dumps should be here 😅.

[keradus]

-=> #7550