Skip to content

add custom rector to cleanup command usages#348

Merged
LordSimal merged 7 commits into6.xfrom
6.x-command-args-io
Nov 17, 2025
Merged

add custom rector to cleanup command usages#348
LordSimal merged 7 commits into6.xfrom
6.x-command-args-io

Conversation

@LordSimal
Copy link
Copy Markdown
Contributor

Refs: cakephp/cakephp#18983

With a lot of help from ChatGPT this miracle has been achieved.

Let me know which other kind of command structures we should add to the tests to see if everything is handled as we expect it to.

@LordSimal
Copy link
Copy Markdown
Contributor Author

I am confused why the following rector config doesn't work anymore even though it previously did just fine:

    $rectorConfig->ruleWithConfiguration(RenameStringRector::class, [
        'accessibleFields' => 'patchableFields',
    ]);

nothing in this PR changed anything about this config. I even tried adjusting the order of the rule configuration, but nothing helps.

@samsonasik may I humbly request your help here please 😅

@samsonasik
Copy link
Copy Markdown
Contributor

Could you show at https://getrector.com/demo for RenameStringRector that not works?

Tried here and seems working ok

https://getrector.com/demo/bffbb6d3-022f-4810-959f-d134f06d4f0b

@LordSimal
Copy link
Copy Markdown
Contributor Author

Its not that the default RenameStringRector does not work, but it doesnt work anymore in combination with my custom rule.

thats what i dont understand.

if i remove my custom rule above my test related to that string change works fine

@samsonasik
Copy link
Copy Markdown
Contributor

Do you write unit test for it, you can register 2+ rules and test it, something like in rector-src:

https://github.com/rectorphp/rector-src/tree/main/tests/Issues/RenameString

@LordSimal
Copy link
Copy Markdown
Contributor Author

We don't have unit tests for custom rector rules, but usually test the original and upgraded folders for the same content. So the original folder contains the code which should be upgraded and the upgraded contains the same file, just with the expected changes.

My custom rector rule above works fine, you can see that in the CI run as it just complains about the StringRector problem.

As you can see with the latest commit CI, it now only complains about my new test files as the new rector rule has been disabled.

@samsonasik
Copy link
Copy Markdown
Contributor

I think you can add unit test so it easier to debug per expection output

Copy link
Copy Markdown
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work 👏

Comment on lines +75 to +77
if (! $node instanceof ClassMethod) {
return null;
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The operating spacing here doesn't seem to be consistent with the cakephp cs rules. Perhaps we aren't running those phpcs rules on this repo?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to the phpcs.xml file everything inside the src folder should be checked. don't know why it doesn't fix this kind of cs violation.

@LordSimal
Copy link
Copy Markdown
Contributor Author

@markstory there is a weird redeclare issue which I don't know how to fix (as can be seen in previous CI runs)
First I thought its a PHP 8.1 only issue but it happens in 8.2 as well. Any ideas?

And thanks to @samsonasik, actually adding unit tests for the new custom rule made me realize, that it didn't behave correctly in all cases, so i had to adjust the conditions when the rule applies and now everything seems to work as we want it 👍🏻

@LordSimal LordSimal force-pushed the 6.x-command-args-io branch from 6557ef8 to 3f1ce70 Compare November 1, 2025 19:27
@LordSimal LordSimal merged commit 32b6175 into 6.x Nov 17, 2025
6 of 12 checks passed
@LordSimal LordSimal deleted the 6.x-command-args-io branch November 17, 2025 18:10
LordSimal added a commit that referenced this pull request Jan 6, 2026
* add custom rector to cleanup command usages

* test with disabled custom rector

* add unit tests

* adjust CI

* adjust composer.json

* Revert "adjust composer.json"

This reverts commit d68f12b.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants