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

Rector proposes unsafe changes for the new initializer rule #9056

Closed
theofidry opened this issue Mar 7, 2025 · 1 comment
Closed

Rector proposes unsafe changes for the new initializer rule #9056

theofidry opened this issue Mar 7, 2025 · 1 comment
Labels

Comments

@theofidry
Copy link

Bug Report

Subject Details
Rector version Rector 2.0.10
PHP version   8.4.2

Issue

Rector is proposing the following change:

1) src/Loader/PurgerLoader.php:40

    ---------- begin diff ----------
@@ @@
     private LoaderInterface $decoratedLoader;
     private PurgerFactoryInterface $purgerFactory;
     private PurgeMode $defaultPurgeMode;
-    private LoggerInterface $logger;

     public function __construct(
         private LoaderInterface $decoratedLoader,
         private PurgerFactoryInterface $purgerFactory,
         string $defaultPurgeMode,
-        ?LoggerInterface $logger = null
+        private LoggerInterface $logger = new NullLogger()
     ) {
         if (!isset(self::$PURGE_MAPPING)) {
             self::$PURGE_MAPPING = [
@@ @@
         }

         $this->defaultPurgeMode = self::$PURGE_MAPPING[$defaultPurgeMode];
-        $this->logger = $logger ?? new NullLogger();
     }

     /**
    ----------- end diff -----------

Applied rules:
 * NewInInitializerRector

Maybe I missed something in the rules policies, but I was under the impression that Rector should, when possible, provide safe changes? The above is a big BC break because if you have the following call:

new PurgerLoader($loader, $factory, $purgeMode, null);

Then after the rector change it will no longer work.

Reproducer

I could get this error in this PR theofidry/AliceDataFixtures#294 but to narrow down the issue I extracted theofidry/AliceDataFixtures#297.

Expected Behaviour

I do not think Rector should provide this change by default, or maybe it could by enabling an unsafe flag for the rule?

@samsonasik
Copy link
Member

duplicated with

the solution I can think of is unregister from set or move to other set, eg: rector-preset or code quality set, I will discuss with @TomasVotruba for it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants