Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add RectorPHP #163

Draft
wants to merge 2 commits into
base: 3.1.x
Choose a base branch
from

Conversation

internalsystemerror
Copy link
Member

Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC yes
QA yes

Description

I thought I'd take a look at how we might use https://github.com/rectorphp/rector. This PR contains 2 commits to first add rector as a dependency with configuration, and then to actually run rector and see what has changed.

Its not my intention for this to merged as is, but as a demonstration to get feedback and help discuss an approach.

Gary Lockett added 2 commits June 2, 2023 16:09
Copy link
Member

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this is very interesting as it highlights a number of changes that are easy to overlook as we bump versions.

@@ -139,7 +140,7 @@ public function testConstructorRewindsBodyStream(): void
$json = ['test' => 'data'];
$response = new JsonResponse($json);

$actual = json_decode($response->getBody()->getContents(), true);
$actual = json_decode($response->getBody()->getContents(), true, 512, JSON_THROW_ON_ERROR);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first change I'm not entirely clear on. I cannot remember what that third argument is supposed to be so seeing the 512 value makes me scratch my head. That said having an exception here makes a lot of sense since it will ensure that the assertion never gets run. I wonder if it can use named arguments here instead? Perhaps @samsonasik can answer that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be best to use flags: JSON_THROW_ON_ERROR here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

512 is the default value. Named parameters were introduced in PHP 8.0, so curious why it didn't use them for this. I guess this demonstrates that rector may be a start but it will nearly always required manual intervention?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it certainly always needs human review

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think JSON_THROW_ON_ERROR is fine here as this is test class

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

named arg currently used on rector for attribute creation

@@ -48,6 +48,7 @@
"php-http/psr7-integration-tests": "^1.3",
"phpunit/phpunit": "^9.5.28",
"psalm/plugin-phpunit": "^0.18.4",
"rector/rector": "^0.17.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something I'd really like to see in our CI, rather than in a dev-dependency.

That said, it is fine here too, for now

Copy link
Member Author

@internalsystemerror internalsystemerror Jun 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no phar available for rector, and we could include it as a global dependency in CI, but that could lead to an introduced failure without any code changes in the repositories.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@internalsystemerror I would make a separate composer.lock just for that, installed globally in our CI container.

Not a problem for now though: just raising the idea :-)

Related: laminas/laminas-ci-matrix-action#131

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No please keep it coming, thats the point of this PR as a draft, to get feedback!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use pinned version 0.17.0 is better imo

$rectorConfig->sets([
LevelSetList::UP_TO_PHP_80,
]);
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is my current config for some laminas components I run:

<?php

declare(strict_types=1);

use Rector\CodingStyle\Rector\ArrowFunction\StaticArrowFunctionRector;
use Rector\CodingStyle\Rector\Closure\StaticClosureRector;
use Rector\Config\RectorConfig;
use Rector\Php55\Rector\String_\StringClassNameToClassConstantRector;
use Rector\Php56\Rector\FunctionLike\AddDefaultValueForUndefinedVariableRector;
use Rector\Php70\Rector\FuncCall\RandomFunctionRector;
use Rector\Php71\Rector\FuncCall\CountOnNullRector;
use Rector\Php73\Rector\FuncCall\JsonThrowOnErrorRector;
use Rector\Privatization\Rector\Property\PrivatizeFinalClassPropertyRector;
use Rector\Set\ValueObject\LevelSetList;
use Rector\TypeDeclaration\Rector\ArrowFunction\AddArrowFunctionReturnTypeRector;
use Rector\TypeDeclaration\Rector\Closure\AddClosureReturnTypeRector;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->sets([
        LevelSetList::UP_TO_PHP_80,
    ]);

    $rectorConfig->rules([
        StaticArrowFunctionRector::class,
        StaticClosureRector::class,
        AddClosureReturnTypeRector::class,
        PrivatizeFinalClassPropertyRector::class,
        AddArrowFunctionReturnTypeRector::class,
    ]);

    $rectorConfig->parallel();
    $rectorConfig->paths([
        __DIR__ . '/src',
        __DIR__ . '/test',
    ]);

    $rectorConfig->skip([
        // possibly too detail on some cases?
        CountOnNullRector::class,

        // possibly null undefined on purpose?
        AddDefaultValueForUndefinedVariableRector::class,

        // define list of services?
        StringClassNameToClassConstantRector::class,

        // not a secure lib on purpose?
        RandomFunctionRector::class,

        // probably handle after execute?
        JsonThrowOnErrorRector::class =>  [
            __DIR__ . '/src',
        ],
    ]);
};

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

Successfully merging this pull request may close these issues.

4 participants