[Naming] Remove matchesStringName() check completely from NodeNameResolver, including endsWith() method - use getName() and compare directly instead #4954

TomasVotruba · 2023-09-09T14:37:50Z

Based on feedback from @staabm , I got an idea to remove the regexes from naming completely. This would make naming API intuitive and clean.

It's still possible to check any name against regex with $this->getName($node) if needed (not recommended though).

What do you think? :) cc @staabm @samsonasik

The single fixture would have to be adjusted, or skipped. Underscore rules were removed long time ago and type actually matches there.

rules/CodeQuality/Rector/If_/SimplifyIfReturnBoolRector.php

rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php

staabm · 2023-09-09T15:39:32Z

packages/NodeNameResolver/NodeNameResolver.php

     */
-    public function endsWith(string $currentName, string $expectedName): bool
+    public function endsWith(Node $node, string|array $suffix): bool


Do we need this method at all, or should just use the matchesName(.., 'myname*') instead?

A caller could even use getName() and invoke start_ or ends_with in its own.

ohh I see. you want todo it the other way arround. drop all regex magic, in case thats good enough for all existing use-cases

You're right. Removing endsWith() and startsWith() might make more sense. There are only 5 cases in our whole code base, so I think it would be better choice.

startsWith() seems already used in rector-phpunit and rector-symfony so that should be changed first

rectorphp/rector-phpunit@f07d7f0

rectorphp/rector-symfony@b9303b5

TomasVotruba · 2023-09-09T17:07:52Z

Ready for 2nd round of reviews 🙂

staabm · 2023-09-09T17:24:13Z

rules/EarlyReturn/Rector/If_/ChangeAndIfToEarlyReturnRector.php

@@ -201,27 +201,27 @@ private function isInLoopWithoutContinueOrBreak(If_ $if): bool
    private function processReplaceIfs(
        If_ $if,
        array $conditions,
-        Return_ $ifNextReturnClone,
+        Return_ $cloneIffNextReturn,


are these class renaming changes related to the actual goal of this PR?

if not, separating these would help to focus on whats important

They're result of fixed naming check 👍 The endsWith() was buggy.

...ts/Naming/Rector/Class_/RenamePropertyToMatchTypeRector/Fixture/change_bare_property.php.inc

…e as needed

TomasVotruba · 2023-09-09T22:49:55Z

Let's ship it 👍

samsonasik · 2023-09-10T01:43:53Z

@TomasVotruba @staabm it seems cause error on $this->isName($node->name, 'assert*') usage on rector-phpunit when rules-tests registered to phpunit.xml, it was never executed, see

rectorphp/rector-phpunit#251 (review)

https://github.com/rectorphp/rector-phpunit/blob/2800f5c374cffc20cbdfb221ce033b46a3a8003d/rules/CodeQuality/Rector/Class_/PreferPHPUnitThisCallRector.php#L97-L99

samsonasik · 2023-09-10T01:59:58Z

@TomasVotruba @staabm also, documentation need to be updated as it no longer works:

$this->isName($node->name, 'set*')

https://github.com/rectorphp/getrector-com/blob/eb645522359c6fed4dccc98556dab0d21658fad3/resources/docs/custom-rule.md?plain=1#L45

samsonasik · 2023-09-10T03:06:02Z

$this->isName($node->name, 'test*') resolved in rector-phpunit PR:

https://github.com/rectorphp/rector-phpunit/pull/251/files#diff-0469c1256426c98149a652ef838fd62475a18e529dc723ca0e8faa6c3fd8749e

samsonasik · 2023-09-10T03:24:25Z

Documentation updated at getrector.org PR:

[Doc] Use fnmatch() instead of $this->isName($node->name, 'set*') in docuemntation create custom rule getrector-com#1654

TomasVotruba requested a review from samsonasik September 9, 2023 14:39

TomasVotruba force-pushed the tv-naming-postfix-3 branch 2 times, most recently from bea1813 to 7ebaf11 Compare September 9, 2023 14:44