Skip to content

[dx] log rules registered in both withRules() and sets, keep them once to avoid duplications or invalid use#6761

Merged
TomasVotruba merged 3 commits into
mainfrom
tv-dx-rules-2
Mar 3, 2025
Merged

[dx] log rules registered in both withRules() and sets, keep them once to avoid duplications or invalid use#6761
TomasVotruba merged 3 commits into
mainfrom
tv-dx-rules-2

Conversation

@TomasVotruba

Copy link
Copy Markdown
Member

No description provided.

@samsonasik

samsonasik commented Feb 28, 2025

Copy link
Copy Markdown
Member

By this, this config will no longer working:

<?php

use Rector\Config\RectorConfig;
use Rector\TypeDeclaration\Rector\Property\TypedPropertyFromAssignsRector;

return RectorConfig::configure()
    // A. whole set
    ->withPreparedSets(typeDeclarations: true)
    // B. or few rules
    ->withRules([
        TypedPropertyFromAssignsRector::class
    ]);

for default demo page config https://getrector.com/demo

However, when withConfiguredRule() called, it MUST keep working, as override with configurable eg:

<?php

use Rector\Config\RectorConfig;
use Rector\TypeDeclaration\Rector\Property\TypedPropertyFromAssignsRector;

return RectorConfig::configure()
    // A. whole set
    ->withPreparedSets(typeDeclarations: true)
    // B. OVERRIDE RULE
    ->withConfiguredRule(TypedPropertyFromAssignsRector::class, [
        'inline_public' => true,
    ]);

ref https://getrector.com/demo/c74dab61-ff51-47fe-9391-f14bc34fc84a

this also needs test for configurable rule registered multiple times to keep working, eg: multiple config included with configurable rules.

@TomasVotruba

Copy link
Copy Markdown
Member Author

The first example is mutually exclusive. Either use A, or B. Not together.

For second use case, I think the configured rules are now skipped, as withRules() accepts only non-configured ones, right?

@samsonasik

samsonasik commented Feb 28, 2025

Copy link
Copy Markdown
Member

No..., withRules() accept configurable rule with default config, so when collect rules, instanceof ConfigurableRectorInterface should be skipped

Comment thread src/Configuration/RectorConfigBuilder.php Outdated
Comment thread src/Configuration/RectorConfigBuilder.php
Comment thread src/Configuration/RectorConfigBuilder.php
@TomasVotruba

Copy link
Copy Markdown
Member Author

I've just tagged a new Rector release https://github.com/rectorphp/rector-src/releases/tag/2.0.10,
so we can test this in the wild separately 👍

Let's ship it :)

@TomasVotruba TomasVotruba merged commit 2bd14ca into main Mar 3, 2025
@TomasVotruba TomasVotruba deleted the tv-dx-rules-2 branch March 3, 2025 17:54
@ottsch

ottsch commented Apr 15, 2025

Copy link
Copy Markdown

@TomasVotruba I'm getting this warning, although I'm only using sets:

[WARNING] These rules are registered in both sets and "withRules()". Remove them from "withRules()" to avoid
duplications:

       * Rector\Symfony\Twig134\Rector\Return_\SimpleFunctionAndFilterRector   
    ->withPhp74Sets()
    ->withSets([
        SetList::CODING_STYLE,
        SetList::DEAD_CODE,
        SetList::TYPE_DECLARATION,
        SetList::CODE_QUALITY,
        TwigSetList::TWIG_112,
        TwigSetList::TWIG_127,
        TwigSetList::TWIG_134,
        TwigSetList::TWIG_140,
    ])

@samsonasik

Copy link
Copy Markdown
Member

I can reproduce:

                                                                                                                        
 [WARNING] These rules are registered in both sets and "withRules()". Remove them from "withRules()" to avoid           
           duplications:                                                                                                
                                                                                                                        
           * Rector\Symfony\Twig134\Rector\Return_\SimpleFunctionAndFilterRector                                        
                                                                                                                        

 1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

It seems due to RectorConfig::configure()->withRules() there

https://github.com/rectorphp/rector-symfony/blob/3681411a79a40459a9e8beb49bd855878fb5f2a7/config/sets/twig/twig134.php#L8C8-L8C44

that's should not happen as it should uniquely set, it probably due to it re-merged on RectorConfig::configure() called, the way we can do for fast fix this is possibly with change to normal:

-return RectorConfig::configure()->withRules([SimpleFunctionAndFilterRector::class]);
+return static function (RectorConfig $rectorConfig): void {
+    $rectorConfig->rule(SimpleFunctionAndFilterRector::class);
+};

config, but that's seems will cause keep issue on rector extension, as more and more of them may use new syntax of RectorConfig::configure()

@samsonasik

Copy link
Copy Markdown
Member

#6840

@github-actions

github-actions Bot commented Dec 1, 2025

Copy link
Copy Markdown
Contributor

This pull request has been automatically locked because it has been closed for 150 days. Please open a new PR if you want to continue the work.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Dec 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants