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\.

[ninetteadhikari]

thanks for the details @terrafrost! I removed the namespace for 1.0.
For the issue with rector rule not working on base class, we may have to write a custom class to make sure all the sub classes also return void. this might get complicated, let me know your thoughts then we can work on that.

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:

[ninetteadhikari]

added back the custom rule to add void to assert and lifecycle methods and also added some tests. 0989ba1

[terrafrost]

I was trying out the latest updates and... when I run vendor/bin/rector process tests/Unit/Crypt/AES --dry-run I get this:

4 files with changes
====================

1) tests/Unit/Crypt/AES/McryptTest.php:5

    ---------- begin diff ----------
@@ @@
  * @license   http://www.opensource.org/licenses/mit-license.html  MIT License
  */

-class Unit_Crypt_AES_McryptTest extends Unit_Crypt_AES_TestCase
+class McryptTest extends Unit_Crypt_AES_TestCase
 {
-    protected function setUp()
+    protected function setUp(): void
     {
         $this->engine = CRYPT_ENGINE_MCRYPT;
     }
    ----------- end diff -----------

Applied rules:
 * AddReturnTypeBaseClass
 * RemoveClassNamePrefix


2) tests/Unit/Crypt/AES/OpenSSLTest.php:5

    ---------- begin diff ----------
@@ @@
  * @license   http://www.opensource.org/licenses/mit-license.html  MIT License
  */

-class Unit_Crypt_AES_OpenSSLTest extends Unit_Crypt_AES_TestCase
+class OpenSSLTest extends Unit_Crypt_AES_TestCase
 {
-    protected function setUp()
+    protected function setUp(): void
     {
         $this->engine = CRYPT_ENGINE_OPENSSL;
     }
    ----------- end diff -----------

Applied rules:
 * AddReturnTypeBaseClass
 * RemoveClassNamePrefix


3) tests/Unit/Crypt/AES/PurePHPTest.php:5

    ---------- begin diff ----------
@@ @@
  * @license   http://www.opensource.org/licenses/mit-license.html  MIT License
  */

-class Unit_Crypt_AES_PurePHPTest extends Unit_Crypt_AES_TestCase
+class PurePHPTest extends Unit_Crypt_AES_TestCase
 {
-    protected function setUp()
+    protected function setUp(): void
     {
         $this->engine = CRYPT_ENGINE_INTERNAL;
     }
    ----------- end diff -----------

Applied rules:
 * AddReturnTypeBaseClass
 * RemoveClassNamePrefix


4) tests/Unit/Crypt/AES/TestCase.php:7

    ---------- begin diff ----------
@@ @@

 require_once 'Crypt/AES.php';

-abstract class Unit_Crypt_AES_TestCase extends PhpseclibTestCase
+abstract class TestCase extends PhpseclibTestCase
 {
     protected $engine;
    ----------- end diff -----------

Applied rules:
 * RemoveClassNamePrefix



 [OK] 4 files would have been changed (dry-run) by Rector

So Unit_Crypt_AES_TestCase is being renamed to TestCase but the newly renamed McryptTest is still extending the old class (Unit_Crypt_AES_TestCase) - not the new one (TestCase).

[terrafrost]

If y'all incorporate terrafrost@b5ab8a7 into your PR you can test code changes in the PR against phpseclib's GitHub Actions. I removed rector from composer.json because that was preventing installation on PHP 7.3 and earlier. Now, rectorphp is being installed via .github/workflows/ci.yml when the version of PHP being used is 7.4+

[ninetteadhikari]

good catch! I pushed a small fix. let me know if this satisfies the condition.

1) tests/Unit/Crypt/AES/McryptTest.php:5

    ---------- begin diff ----------
@@ @@
  * @license   http://www.opensource.org/licenses/mit-license.html  MIT License
  */

-class Unit_Crypt_AES_McryptTest extends Unit_Crypt_AES_TestCase
+class McryptTest extends TestCase
 {
-    protected function setUp()
+    protected function setUp(): void
     {
         $this->engine = CRYPT_ENGINE_MCRYPT;
     }
    ----------- end diff -----------

Applied rules:
 * AddReturnTypeBaseClass
 * RemoveClassNamePrefix

[terrafrost]

When I applied the ‎.github/workflows/ci.yml changes from terrafrost@b5ab8a7 into your changes I got this error:

PHP Fatal error:  Uncaught Error: Class 'TestCase' not found in /home/runner/work/phpseclib/phpseclib/tests/Unit/Crypt/AES/McryptTest.php:8

I'm a little surprised by that tbh and am not completely sure why that's an issue. Best guess is that it maybe has to do with the fact that phpseclib v1 uses PSR-0 autoloading vs PSR-4 autoloading (which every other version of phpseclib uses) but idk for sure.

One can reproduce this without using GitHub Actions by doing the following:

composer require rector/rector:^2.2
composer dump-autoload
vendor/bin/rector process
vendor/bin/phpunit --configuration=phpunit.xml.dist tests/Unit/Crypt/AES/McryptTest.php

In light of this I did some testing.

So php tests/make_compatible_with_phpunit9.php does this:

-class Unit_Crypt_AES_McryptTest extends Unit_Crypt_AES_TestCase
+class McryptTest extends Unit_Crypt_AES_TestCase

It doesn't change abstract class Unit_Crypt_AES_TestCase extends PhpseclibTestCase at all.

Your old changes did this:

-class Unit_Crypt_AES_McryptTest extends Unit_Crypt_AES_TestCase
+class McryptTest extends Unit_Crypt_AES_TestCase

-abstract class Unit_Crypt_AES_TestCase extends PhpseclibTestCase
+abstract class TestCase extends PhpseclibTestCase

Your new changes do this:

-class Unit_Crypt_AES_McryptTest extends Unit_Crypt_AES_TestCase
+class McryptTest extends TestCase

-abstract class Unit_Crypt_AES_TestCase extends PhpseclibTestCase
+abstract class TestCase extends PhpseclibTestCase

So I guess the most faithful thing to do would be to not rename abstract classes.

That said, I modified tests/make_compatible_with_phpunit9.php to comment out the following lines:

            '~^class Unit_Crypt_(AES|Hash|RSA)_~m' => 'class ',
            '~^class Unit_File_X509_~m' => 'class ',
            '~^class Unit_Math_BigInteger_~m' => 'class ',
            '~^class Unit_(Crypt|File|Math|Net)_~m' => 'class ',
            '~^class Functional_Net__~m' => 'class ',
            '~extends Unit_Crypt_Hash_(SHA512Test|SHA256Test)~' => 'extends $1'

And then ran the unit tests with PHPUnit 9.6.31 and the unit tests ran just fine.

In fact, if I modify your rector.php thusly the unit tests pass:

    ->withRules([
        RemoveClassNamePrefix::class,
-        ShortenShaExtends::class,
-        AddReturnTypeBaseClass::class
+        //ShortenShaExtends::class,
+        //AddReturnTypeBaseClass::class
    ]);

It's not actually clear to me why the classes are being renamed. In the 1.0 branch tests/make_compatible_with_phpunit9.php was introduced with 1b1e729 (backport enhancements from the 2.0 branch) and it's been renaming those files since it's initial inclusion. In the 2.0 branch it was introduced with 8e8b214 (use github actions instead of travis ci). Both of those commits were made on Feb 11, 2023.

In the 3.0 branch it was, initially, tests/make_compatible_with_new_phpunit_versions.php, which was added with 4675810 (GitHub actions), added on March 8, 2022 but files weren't being renamed at that point. In fact, none of the tests/make_compatible_with_phpunit*.php files in the 3.0 branch do any file renaming, altho then again, the files are already renamed in the 3.0 branch, anyway.

[terrafrost]

Well, on GitHub Actions, on Windows / PHP 7.4+ I get this error:

Error: ] Could not process "tests/Unit/Crypt/AES/McryptTest.php" file, due to:
"Call to undefined method PHPStan\PhpDocParser\Ast\PhpDoc\TemplateTagValueNode::__set_state()". On line: 83

Not sure what that's all about.

[terrafrost]

terrafrost@f559b04 is my latest attempt at this. Still getting the Windows errors that I don't understand.

One big departure from what this PR is doing is that I'm using https://github.com/rectorphp/rector-phpunit . The big advantage of that, in my mind, is that if PHPUnit 8 stops working on PHP 9.0 then I don't have to write my own custom rules to deal with it - I can just use one of the newer built in Rector rules.

[ninetteadhikari]

hi @terrafrost apologies for misunderstanding. i've dropped the commit that changed the extends and have added a commit to ignore the abstract classes, so hopefully this will not break the class hierarchy.. let me know if this is more compatible?