[Core] Merge Config instead of Replace on RectorConfig->ruleWithConfiguration() take - 2

[samsonasik]

Re-create closed PR #2655

[samsonasik]

@TomasVotruba Finally working 🎉 🎉 🎉 , the trick is:

• Save to variable the original config which not converted to InlineServiceConfigurator yet early
• Merge config if there is existing config with rule
• Convert to InlineServiceConfigurator after merged if values are object and use to decorate the service.
• Save the original config to property fetch configuration collection.

[samsonasik]

All checks have passed 🎉 @TomasVotruba it is ready for review.

[MartinMystikJonas]

I would really like to see this merged. Current way with possible unnoticed accidental loose of configured rule does not seem to follow rule of least surprise. Merging inside config same was as between different configs seems logical to me.

[TomasVotruba]

This should rather raise an exception, as convention in DI configs is to add them once.

[samsonasik]

raise an exception could cause issue if user want to use SetList but want to override one of the rule, eg:

<?php

use Rector\Config\RectorConfig;
use Rector\Set\ValueObject\SetList;

return static function (RectorConfig $rectorConfig): void {
    
    $rectorConfig->sets([
        SetList::CODING_STYLE
    ]);
    
    $rectorConfig
        ->ruleWithConfiguration(Rector\Transform\Rector\FuncCall\FuncCallToConstFetchRector::class, [
            'php_sapi' => 'NEW_Constant'
        ]);

    };

to override existing config

rector-src/config/set/coding-style.php

Line 37 in b1ae9a1

->ruleWithConfiguration(FuncCallToConstFetchRector::class, [

[MartinMystikJonas]

@samsonasik My solution do not raise exception in this case.

[MartinMystikJonas]

@TomasVotruba From user point of view I do not know and do not care about fact that RectorConfig internally uses Symfony DI to register rules. I expect that if I add two rules to rename class it would rename them both. Or at least notify me that I must add rule only once with merged configs.

[TomasVotruba]

Even if we don't use any fw package, registering the same service twice in single config is not valid. Allowing it spread bad practise and expose us to handle all other unexpected behaviors. We won't go that way.

[TomasVotruba]

@samsonasik That's different case. I talk about same config with 2 explicit registration of 1 service.

[samsonasik]

@MartinMystikJonas I see, that's possibly due to imported config, that's will need unit tests tho, eg, under tests/Issues

@TomasVotruba ok, I am closing it then.

[MartinMystikJonas]

@TomasVotruba Ok so do we keep current status quo with silently ignoring these usage error or should I add tests coverage as @samsonasik suggested and finish my PR that throws error when same rule is added twice?

[TomasVotruba]

This must be resolved on Symfony level, as that's the architecture pattern we respect.
Let's mark it won't fix here.