safe icon indicating copy to clipboard operation
safe copied to clipboard

Update `rector-migrate.php`

Open Aerendir opened this issue 3 years ago • 7 comments

The new configuration is this:

<?php

declare(strict_types=1);

use Rector\Renaming\Rector\FuncCall\RenameFunctionRector;

// This file configures rector/rector to replace all PHP functions with their equivalent "safe" functions.
return static function (\Rector\Config\RectorConfig $containerConfigurator): void {
    $containerConfigurator->ruleWithConfiguration(
        RenameFunctionRector::class,
        [...]
    );
};

To support older versions:

<?php
// Don't tested, but should work
declare(strict_types=1);

use Rector\Renaming\Rector\FuncCall\RenameFunctionRector;
use Symfony\Component\DependencyInjection\Loader\Configurator\ContainerConfigurator;

// This file configures rector/rector to replace all PHP functions with their equivalent "safe" functions.
return class_exists(\Rector\Config\RectorConfig)
? static function (\Rector\Config\RectorConfig $containerConfigurator) : void {
    $containerConfigurator->ruleWithConfiguration(
        RenameFunctionRector::class,
        []
    );
}
: static function (ContainerConfigurator $containerConfigurator): void {
    $services = $containerConfigurator->services();

    $services->set(RenameFunctionRector::class)
        ->call('configure', [[ RenameFunctionRector::OLD_FUNCTION_TO_NEW_FUNCTION => []]]);
};

Aerendir avatar Aug 02 '22 21:08 Aerendir

@Aerendir This would be fixed by https://github.com/thecodingmachine/safe/pull/372, correct?

dbrekelmans avatar Aug 04 '22 14:08 dbrekelmans

@dbrekelmans yes, it will, but will break compatibility with older versions. The code I proposed maintains backward compatibility.

Aerendir avatar Aug 05 '22 11:08 Aerendir

I just saw you released the version 2.2.3, so you merged the PR #372 in a patch release.

This change, however, is potentially backward breaking change, so it should have been merged in a major release if you follow the semver.

Using the code I suggested, instead, it would not break backward compatibility and can be released also in a patch release. My 2 cents.

Aerendir avatar Aug 05 '22 12:08 Aerendir

Sorry I was in holidays for the last 2 weeks, i didn't see your thread.

We definitly don't want to break semVer. If you are confident about your code, i am in favor of merging it over #372 and quickly publishing a new tag to cover our mistake. Or we delete the current tag and create a major release. (but a major release just to fix rector is weird)

@dbrekelmans what do you think?

Kharhamel avatar Aug 09 '22 14:08 Kharhamel

Agreed. I would be in favor of doing a new patch release after this is merged.

dbrekelmans avatar Aug 09 '22 14:08 dbrekelmans

I will try to do it tomorrow

Kharhamel avatar Aug 09 '22 15:08 Kharhamel

Thanks a lot for the suggested patch.

Real question here:

Do we want to ensure SEMVER for Rector? I mean, it is not run at runtime, and I doubt someone is running this in a CI/CD too. Shouldn't we exclude it from semver support? (I'm saying this because if at some point, we cannot do rector-migrate file that supports all rector versions, I'd rather drop support for older Rector releases than prevent Safe users to use a newer Rector release.

moufmouf avatar Aug 19 '22 19:08 moufmouf