Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update rector-migrate.php #375

Open
Aerendir opened this issue Aug 2, 2022 · 7 comments
Open

Update rector-migrate.php #375

Aerendir opened this issue Aug 2, 2022 · 7 comments

Comments

@Aerendir
Copy link

Aerendir commented Aug 2, 2022

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
Copy link
Collaborator

@Aerendir This would be fixed by #372, correct?

@Aerendir
Copy link
Author

Aerendir commented Aug 5, 2022

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

@Aerendir
Copy link
Author

Aerendir commented Aug 5, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator

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

@Kharhamel
Copy link
Collaborator

I will try to do it tomorrow

@moufmouf
Copy link
Member

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.

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

No branches or pull requests

4 participants