Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MultiDirnameRector #6796

Merged
merged 1 commit into from
Mar 20, 2025

Conversation

carlos-granados
Copy link
Contributor

@carlos-granados carlos-granados commented Mar 20, 2025

Derived from #6794

@@ -7,16 +7,6 @@ function multiDirname()
dirname(dirname($path));

new dirname(dirname(dirname($path)));


// untouched
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved these cases to a new file to be able to check that the rule is not actually applied

Copy link
Member

Choose a reason for hiding this comment

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

Awesome 👏


dirname($path, 1);

dirname($path, 2);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In these cases the rule would have been applied even though no actual change was made in the code

@@ -56,12 +56,14 @@ public function refactor(Node $node): ?Node
$activeFuncCallNode = $node;
$lastFuncCallNode = $node;

$shouldUpdate = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We check that there is at least one nested dirname call, if not the rule is not applied

@TomasVotruba TomasVotruba merged commit 36f4570 into rectorphp:main Mar 20, 2025
44 checks passed
@carlos-granados carlos-granados deleted the fix-MultiDirnameRector branch March 21, 2025 08:24
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.

2 participants