Open Tabrisrp opened 1 week ago
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?
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!
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.
I experimented a bit. My findings so far:
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 https://github.com/szepeviktor/phpstan-wordpress/issues/239#issuecomment-2336402218.
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';
I think this should be optional with a switch to enable it.
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.
I generally oppose including files. My plugins are using an autoloader.
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
// [...]
}
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?