thecodingmachine / safe

All PHP functions, rewritten to throw exceptions instead of returning false
MIT License
2.34k stars 140 forks source link

Update `rector-migrate.php` #375

Open Aerendir opened 2 years ago

Aerendir commented 2 years ago

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 => []]]);
};
dbrekelmans commented 2 years ago

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

Aerendir commented 2 years ago

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

Aerendir commented 2 years ago

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.

Kharhamel commented 2 years ago

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?

dbrekelmans commented 2 years ago

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

Kharhamel commented 2 years ago

I will try to do it tomorrow

moufmouf commented 2 years ago

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.