Introducing Rector

[hulkoba]

Overview

This work is conducted in accordance with "Milestone 1: Use RectorPHP for upgrading PHPUnit tests," as outlined in the Scope of Work for the Sovereign Tech Agency Resilience Program.

Note: We open this PR on master branch. Please let us know if you want to have it elsewhere.

This PR:

• adds initial rector setup
• adds custom Rector rules for these patterns
• adds documentation for Rector rules

Run rector

This requires files in the directory path in rector.php file

vendor/bin/rector process src --dry-run

Run rector test

vendor/bin/phpunit tests --filter RuleName

[terrafrost]

I'll take a look this weekend - thanks!

[terrafrost]

Give me like two more days on this. I had some unexpected things come up over the weekend.

Thanks!

[terrafrost]

So ideally this would be in earlier versions of phpseclib. It's the earlier versions of phpseclib that run on a large gamut of PHP versions. Like although phpseclib 1.0 will run on both PHP 5.3 and PHP 8.5 I don't believe there's any version of PHPUnit that'll do the same, hence the need for tooling to automatically upgrade PHPUnit.

The master branch of phpseclib only works on PHP 8.2+. And who knows, maybe the minimum version of PHP will go up.

Like ideally rector would replace lines like this in .github/workflows/ci.yml (from the 1.0 branch):

-   name: Make Tests Compatiable With PHPUnit 9+
    if: contains(fromJSON('["7.3", "7.4", "8.0", "8.1", "8.2", "8.3", "8.4", "8.5"]'), matrix.php-version)
    run: php tests/make_compatible_with_phpunit9.php

It's not needed for the master branch because .github/workflows/ci.yml in the master branch doesn't do any php tests/make_compatible_with_phpunit9.php-like calls:

https://github.com/phpseclib/phpseclib/blob/master/.github/workflows/ci.yml

That said, in looking into it, now, I guess one problem with RectorPHP doing this is that it'd be running on older versions of PHP. Like I see that the latest version of RectorPHP requires PHP 7.4+ so we couldn't run that version on PHP 7.3 BUT we could do RectorPHP 0.8.9 on PHP 7.2. I'm not sure what the last version of RectorPHP was that worked on PHP 7.2 but Composer could figure that out.

I see that phpseclib 3.0 calls php tests/make_compatible_with_phpunit7.php for PHP 7.1+ and php tests/make_compatible_with_phpunit9.php for PHP 7.3+. It's strange that phpseclib 3.0 would need to do php tests/make_compatible_with_phpunit7.php but phpseclib 1.0 wouldn't. Like phpseclib 1.0 is unit tested on PHP 5.3+ whereas phpseclib 3.0 is only unit tested on PHP 7.0+ so why don't the unit tests for phpseclib 1.0 on PHP 7.1 need to be made compatible with PHPUnit 7 whereas they do for phpseclib 3.0? Maybe the key take away is that php tests/make_compatible_with_phpunit7.php isn't even necessary for phpseclib 3.0? idk. If it is absolutely necessary for the unit tests to work on PHP 7.1 with phpseclib 3.0 then I suppose we could just unit test phpseclib 3.0 on PHP 7.2+.

Unrelated to all of that... I would have thought you would have used https://github.com/rectorphp/rector-phpunit/ . Like instead of implementing a whole AddVoidLifecycleAssert class you could have just done something like this:

https://getrector.com/demo/b1390585-12e3-4f5d-9d4f-ad7f3312d937

Or am I missing something?

[ninetteadhikari]

@terrafrost on your last point regarding the use of rector-phpunit you are right. i thought i didnt see relevant rules to fit the use cases but i might have overlooked! let me review mentioned the link and see possible ways to incorporate it.

on your earlier point we can rebase the current PR to 1.0 branch for now. not sure of the best way to address the PHPUnit version discrepancies yet but let us know if you have any next steps we can consider

[terrafrost]

As an experiment, I disabled unit testing on GitHub Actions for PHP versions less than 7.4 for the 1.0 branch and am now getting PHP Fatal error: Declaration of PhpseclibTestCase::tearDown() must be compatible with PHPUnit\Framework\TestCase::tearDown(): void errors:

terrafrost@8e75dbe

So that needs to be resolved.

I also tried to do a proof of concept for how GitHub Actions could run on versions of PHP less than 7.4 without Rector and on versions equal to or newer than 7.4 with Rector:

terrafrost@b5ab8a7

The latter approach works but the underlying Rector issues remain. Looking at the latest rector.php it looks like this, in theory, should take care of it:

    ->withConfiguredRule(AddReturnTypeDeclarationRector::class, [
        // PHPUnit lifecycle methods
        new AddReturnTypeDeclaration('PHPUnit\Framework\TestCase', 'setUp', new VoidType()),
        new AddReturnTypeDeclaration('PHPUnit\Framework\TestCase', 'setUpBeforeClass', new VoidType()),
        new AddReturnTypeDeclaration('PHPUnit\Framework\TestCase', 'tearDown', new VoidType()),

        // PHPUnit assertion helpers
        new AddReturnTypeDeclaration('PHPUnit\Framework\Assert', 'assertIsArray', new VoidType()),
        new AddReturnTypeDeclaration('PHPUnit\Framework\Assert', 'assertIsString', new VoidType()),
        new AddReturnTypeDeclaration('PHPUnit\Framework\Assert', 'assertStringContainsString', new VoidType()),
        new AddReturnTypeDeclaration('PHPUnit\Framework\Assert', 'assertStringNotContainsString', new VoidType()),
    ]);

A simplified version of that does do the trick:

https://getrector.com/demo/c5395321-c273-46b7-acc2-9c07c5e12514

It seems like the issue is that it doesn't work when the first parameter - the class parameter - is the base class - and the actual classes that we want to make the changes to extend that base class:

https://getrector.com/demo/48c99a00-e135-4ce1-a9fa-bfe5dda2056c

Another thought. I feel like maybe the new rector rules ought to be in the phpseclib/ namespace.. Like the unit tests in the v2 branch are namespaced in phpseclib so using that same logic it stands to reason that the rector rules ought to be similarly namespaced. Like what if rectorphp decides to add a RemoveClassNamePrefix rule?

Thanks!

[terrafrost]

For the 1.0 branch this line can be deleted. The tests in the 1.0 branch aren't namespaced and, if they were, they'd be namespaced with phpseclib\ - not phpseclib4\.