PHPStan 1.12.1 requireFile new rule

[remyperona]

Following PHPStan update 1.12.1, there is a new rule and error showing up when running it (with bleeding edge):

Path in require_once() "1wp-admin/includes/plugin-install.php" is not a file or it does not exist.

The usage in our code looks like so:
require_once ABSPATH . 'wp-admin/includes/plugin-install.php';

Does this new rule require an update of your extension, or would it just be a configuration change to do on our side?

[szepeviktor]

You could require phpstan with version constraint <1.12.1
or ignore that rule requireFileExists: false

The solution will come one day!

@IanDelMar What do you think?

[remyperona]

We currently updated the config to ignore the rule indeed, wanted to check with you if you already had a different solution or plan for it.

Thank you!

[IanDelMar]

I'm not sure, but I don't think there is anything more that can be done about this out of the box, apart from disabling the check using the toggle requireFileExists: false or ignoring the error.

The only other option I can think of is to disable the rule extension-wide and reintroduce it with WordPress core files being ignored.

[IanDelMar]

I experimented a bit. My findings so far:

• Using a NodeVisitor to add error ignore comments does not work. At least, I could not get PHPStan to respect those comments.
• Adding a wrapper rule for RequireFileExistsRule works but requires ignoring all the 'require' error identifiers and replacing them with custom ones.

Ultimately, I concluded that unless we can ensure that those files actually exist—i.e. by checking against all existing WordPress core files—we should not ignore those errors. I tried checking whether the string contains 'wp-includes/', 'wp-admin/', or 'wp-content/', but that also suppresses errors for 'wp-includes/nonExistentFile.php'. I believe it is something that should be left to the user to decide whether the file is supposed to be there. I do not think it is worth the effort at the moment. However, I suggest adding a note to the README that this is something the package intentionally does not cover.

Note: code below won't work. See #239 (comment).

Here are the NodeVisitor and the test I used, in case someone else wants to try:

<?php // src/RequireFileVisitor.php

declare(strict_types=1);

namespace SzepeViktor\PHPStan\WordPress;

use PhpParser\Comment\Doc;
use PhpParser\Node;
use PhpParser\Node\Expr\Include_;
use PhpParser\Node\Scalar\String_;
use PhpParser\Node\Stmt\Expression;
use PhpParser\NodeFinder;

final class RequireFileVisitor extends \PhpParser\NodeVisitorAbstract
{
    private const EXCLUSIONS = [
        'wp-admin/',
        'wp-includes/',
        'wp-content/',
    ];

    private const ERROR_IDENTIFIERS = [
        Include_::TYPE_REQUIRE => 'require.fileNotFound',
        Include_::TYPE_REQUIRE_ONCE => 'requireOnce.fileNotFound',
        Include_::TYPE_INCLUDE => 'include.fileNotFound',
        Include_::TYPE_INCLUDE_ONCE => 'includeOnce.fileNotFound',
    ];

    /** @var \PhpParser\NodeFinder $nodeFinder */
    private $nodeFinder;

    public function __construct()
    {
        $this->nodeFinder = new NodeFinder();
    }

    public function enterNode(Node $node): ?Expression
    {
        if (! $node instanceof Expression) {
            return null;
        }

        if (! $node->expr instanceof Include_) {
            return null;
        }

        if (! $this->needsIgnoreComment($node)) {
            return null;
        }

        if (! $node->getDocComment() instanceof Doc) {
            $node->setDocComment(
                new Doc(sprintf('/** %s */', $this->getIgnoreTag($node->expr)))
            );
            return $node;
        }

        $docComment = $node->getDocComment()->getText();
        $docComment = str_replace(
            '*/',
            sprintf(
                '* %s%s */',
                $this->getIgnoreTag($node->expr),
                PHP_EOL
            ),
            $docComment
        );
        $node->setDocComment(new Doc($docComment));

        return $node;
    }

    private function needsIgnoreComment(Expression $node): bool
    {
        $foundStrings = $this->nodeFinder->find(
            [$node],
            static function ($node): bool {
                return $node instanceof String_;
            }
        );

        $foundStrings = array_map([$this, 'isExcludedString'], $foundStrings);
        return count($foundStrings) !== 0;
    }

    private function isExcludedString(String_ $stringToCheck): bool
    {
        foreach (self::EXCLUSIONS as $exclusion) {
            if (strpos($stringToCheck->value, $exclusion) !== false) {
                return true;
            }
        }
        return false;
    }

    private function getIgnoreTag(Include_ $node): string
    {
        return sprintf('@phpstan-ignore %s', self::ERROR_IDENTIFIERS[$node->type]);
    }
}

<?php // tests/RequireFileVisitorTest.php

declare(strict_types=1);

namespace SzepeViktor\PHPStan\WordPress\Tests;

/**
 * @extends \PHPStan\Testing\RuleTestCase<\PHPStan\Rules\Keywords\RequireFileExistsRule>
 */
class RequireFileExistsVisitorTest extends \PHPStan\Testing\RuleTestCase
{
    protected function getRule(): \PHPStan\Rules\Rule
    {
        return new \PHPStan\Rules\Keywords\RequireFileExistsRule(__DIR__);
    }

    // phpcs:ignore NeutronStandard.Functions.LongFunction.LongFunction
    public function testRule(): void
    {
        $expectedErrors =  [
            [
                'Path in include() "a-file-that-does-not-exist.php" is not a file or it does not exist.',
                15,
            ],
            [
                'Path in include_once() "a-file-that-does-not-exist.php" is not a file or it does not exist.',
                16,
            ],
            [
                'Path in require() "a-file-that-does-not-exist.php" is not a file or it does not exist.',
                17,
            ],
            [
                'Path in require_once() "a-file-that-does-not-exist.php" is not a file or it does not exist.',
                18,
            ],
        ];

        $this->analyse([__DIR__ . '/data/require-file.php'], $expectedErrors);
    }

    public static function getAdditionalConfigFiles(): array
    {
        return [dirname(__DIR__) . '/vendor/szepeviktor/phpstan-wordpress/extension.neon'];
    }
}

<?php // tests/data/require-file.php

declare(strict_types=1);

namespace SzepeViktor\PHPStan\WordPress\Tests;

$fileThatDoesNotExist = 'a-file-that-does-not-exist.php';
$fileThatDoesExist = __DIR__ . '/../bootstrap.php';

include $fileThatExists;
include_once $fileThatExists;
require $fileThatExists;
require_once $fileThatExists;

include $fileThatDoesNotExist;
include_once $fileThatDoesNotExist;
require $fileThatDoesNotExist;
require_once $fileThatDoesNotExist;

include '/wp-admin/someFile.php';
include_once '/wp-includes/someFile.php';
require '/wp-content/someFile.php';
require_once '/wp-admin/someFile.php';

[szepeviktor]

I think this should be optional with a switch to enable it.

[IanDelMar]

I think this should be optional with a switch to enable it.

Certainly, if we implement a workaround for the new rule!

I found no way to implement the wrapper rule variant conditionally, as it requires ignoring errors, which, I believe, cannot be done conditionally. I couldn't get the visitor variant to work (likely due to my lack of PHPStan knowledge), but it can be implemented with a toggle.

@szepeviktor If you think this is something the package should cover, I will continue exploring options. I can open PRs with both variants—this might make it easier for others to jump in and suggest solutions.

[szepeviktor]

I generally oppose including files. My plugins are using an autoloader.

[IanDelMar]

I finally realised what I had not noticed before and what is causing the variant using a NodeVisitor to not work: while you can change the nodes (including adding comments) using a NodeVisitor, the ignore error tags are extracted from the original nodes.

From RichParser.php:

public function parseString(string $sourceCode): array
{
	// [...]

	$tokens = $this->lexer->getTokens(); 
	// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	// This is where ignore tags are extracted from

	if ($errorHandler->hasErrors()) {
		throw new ParserErrorsException($errorHandler->getErrors(), null);
	}
	if ($nodes === null) {
		throw new ShouldNotHappenException();
	}


	$nodeTraverser = new NodeTraverser();
	$nodeTraverser->addVisitor($this->nameResolver);


	$traitCollectingVisitor = new TraitCollectingVisitor();
	$nodeTraverser->addVisitor($traitCollectingVisitor);


	foreach ($this->container->getServicesByTag(self::VISITOR_SERVICE_TAG) as $visitor) {
		$nodeTraverser->addVisitor($visitor);
	}


	/** @var array<Node\Stmt> */
	$nodes = $nodeTraverser->traverse($nodes);
	// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	// Conditional ignore tags are added here via the NodeVisitor

	['lines' => $linesToIgnore, 'errors' => $ignoreParseErrors] = $this->getLinesToIgnore($tokens);
	// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
	// Conditional ignore tags will not be respected here, as they are added after the tokens have been retrieved

	// [...]
}