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

PrivateInFinalClassRule throws false positive when using PHPUnit attribute #[Before] #845

Open
cosmastech opened this issue Oct 2, 2024 · 0 comments · May be fixed by #863
Open

PrivateInFinalClassRule throws false positive when using PHPUnit attribute #[Before] #845

cosmastech opened this issue Oct 2, 2024 · 0 comments · May be fixed by #863

Comments

@cosmastech
Copy link

I'm happy to implement a solution here, just not sure of the best approach. I think the most precise solution is to check if the $node in PrivateInFinalClass has a Before attribute. Maybe this should be configurable at some point 🤷

Steps required to reproduce the problem

I have a test class written like this:

namespace Tests\Feature\Models;

use App\Models\User;
use Illuminate\Database\Eloquent\Model;
use Illuminate\Foundation\Testing\LazilyRefreshDatabase;
use PHPUnit\Framework\Attributes\Before;
use PHPUnit\Framework\Attributes\CoversClass;
use Tests\TestCase;

#[CoversClass(User::class)]
final class UserTest extends TestCase
{
    use LazilyRefreshDatabase;
    #[Before(1)]
    protected function unguardModel(): void
    {
        Model::unguard();
    }

    // ... test cases here
}

Expected Result

No warning should be raised from PHPStan.

Actual Result

When running PHPStan with the PrivateInFinalClassRule, it tells me that this method can be private. However, if I convert the error to private, PHPUnit fails because Error: Call to private method Tests\Feature\Models\UserTest::unguardModels() from scope PHPUnit\Framework\TestCase.

Suggested Change

declare(strict_types=1);

/**
 * Copyright (c) 2018-2024 Andreas Möller
 *
 * For the full copyright and license information, please view
 * the LICENSE.md file that was distributed with this source code.
 *
 * @see https://github.com/ergebnis/phpstan-rules
 */

namespace Ergebnis\PHPStan\Rules\Methods;

use PhpParser\Node;
use PHPStan\Analyser;
use PHPStan\Reflection;
use PHPStan\Rules;
use PHPStan\ShouldNotHappenException;


final class PrivateInFinalClassRule implements Rules\Rule
{
    private static $attributesThatIndicateProtectedIsOk = [
        \PHPUnit\Framework\Attributes\Before::class
    ];

    public function getNodeType(): string
    {
        return Node\Stmt\ClassMethod::class;
    }

    public function processNode(
        Node $node,
        Analyser\Scope $scope,
    ): array {
        if (!$node instanceof Node\Stmt\ClassMethod) {
            throw new ShouldNotHappenException(\sprintf(
                'Expected node to be instance of "%s", but got instance of "%s" instead.',
                Node\Stmt\ClassMethod::class,
                $node::class,
            ));
        }

        /** @var Reflection\ClassReflection $containingClass */
        $containingClass = $scope->getClassReflection();

        if (!$containingClass->isFinal()) {
            return [];
        }

        if ($node->isPublic()) {
            return [];
        }

        if ($node->isPrivate()) {
            return [];
        }

        // Check attributes
        foreach($node->attrGroups as $attrGroup) {
            foreach($attrGroup->attrs as $attr) {
                if (in_array($attr->name->toString(), self::$attributesThatIndicateProtectedIsOk)) {
                    return [];
                }
            }
        }

        $methodName = $node->name->toString();

        $parentClass = $containingClass->getNativeReflection()->getParentClass();

        if ($parentClass instanceof \ReflectionClass && $parentClass->hasMethod($methodName)) {
            $parentMethod = $parentClass->getMethod($methodName);

            if ($parentMethod->isProtected()) {
                return [];
            }
        }

        $ruleErrorBuilder = Rules\RuleErrorBuilder::message(\sprintf(
            'Method %s::%s() is protected, but since the containing class is final, it can be private.',
            $containingClass->getName(),
            $methodName,
        ));

        return [
            $ruleErrorBuilder->build(),
        ];
    }
}
@cosmastech cosmastech linked a pull request Nov 17, 2024 that will close this issue
2 tasks
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 a pull request may close this issue.

1 participant