Skip to content

[e2e] $rectorConfig->skip() should not skip inside different path over config#4882

Merged
TomasVotruba merged 4 commits intomainfrom
different-path
Aug 30, 2023
Merged

[e2e] $rectorConfig->skip() should not skip inside different path over config#4882
TomasVotruba merged 4 commits intomainfrom
different-path

Conversation

@samsonasik
Copy link
Copy Markdown
Member

@TomasVotruba @staabm this is e2e test for check the config:

    $rectorConfig->paths([
        __DIR__ . '/src',
    ]);

    $rectorConfig->skip([
        RemoveEmptyClassMethodRector::class => [
            __DIR__ . '/src/controllers',
        ],
    ]);

    $rectorConfig->rule(RemoveEmptyClassMethodRector::class);

which means, if the path is different, eg: src/models, it should still run, it currently show:

[WARNING] Register rules or sets in your "rector.php" config

which incorrect.

Directly use __DIR__ . '/src/controllers' in skip paths() seems completely skip all, not sure if there is different issue, that probably need separate PR.

@samsonasik
Copy link
Copy Markdown
Member Author

@TomasVotruba $rectorConfig->skip() seems trying to read getcwd() which somehow can't goes to realpath(), and if we try on getrector demo site, it goes error: Unable to write file '/var/www/public/rector.php'. which strange

https://getrector.com/demo/5871515d-0885-4df1-bd65-ef36442d5703

@samsonasik
Copy link
Copy Markdown
Member Author

@TomasVotruba it seems bug on ContainerMemento class, the service is unset() when using skip() even when define path over the skip class:

@samsonasik
Copy link
Copy Markdown
Member Author

@TomasVotruba @staabm fixed 🎉 , the forgetting service on skip() should only when path is not specified fec890d

@samsonasik
Copy link
Copy Markdown
Member Author

All checks have passed 🎉 🎉 🎉 @TomasVotruba I think it is ready 🎉 🎉 🎉 🎉 🎉

@TomasVotruba
Copy link
Copy Markdown
Member

Thank you 😊👍

@TomasVotruba TomasVotruba merged commit 5f52c69 into main Aug 30, 2023
@TomasVotruba TomasVotruba deleted the different-path branch August 30, 2023 11:37
}
}

$hasForgotten = true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this flag still correct then?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That seems used on line 727 to run once on phpunit

foreach ($skippedElements as $skippedClass => $path) {
// completely forget the Rector rule only when no path specified
if ($path === null) {
ContainerMemento::forgetService($container, $skippedClass);
Copy link
Copy Markdown
Contributor

@staabm staabm Aug 30, 2023

Choose a reason for hiding this comment

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

could you give me an idea why we need to drop classes from the container when rules are skipped?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@staabm
Copy link
Copy Markdown
Contributor

staabm commented Aug 30, 2023

thanks for the fix!

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants