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

Adds DeprecatedHelperVisitor #659

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions extension.neon
Original file line number Diff line number Diff line change
Expand Up @@ -307,3 +307,7 @@ services:
class: mglaman\PHPStanDrupal\DeprecatedScope\GroupLegacyScope
tags:
- phpstan.deprecations.deprecatedScopeResolver
-
class: mglaman\PHPStanDrupal\Parser\DeprecatedHelperVisitor
tags:
- phpstan.parser.richParserNodeVisitor
1 change: 1 addition & 0 deletions phpstan.neon
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ parameters:
- tests/src/data/*.php
- tests/src/Type/data/*.php
- tests/src/Rules/data/*.php
- tests/src/Parser/DeprecatedHelperVisitor/data/*.php
- tests/src/DeprecatedScope/data/*.php
dynamicConstantNames:
- Drupal::VERSION
Expand Down
39 changes: 39 additions & 0 deletions src/Parser/DeprecatedHelperVisitor.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?php

namespace mglaman\PHPStanDrupal\Parser;

use PhpParser\Node;
use PhpParser\NodeTraverser;
use PhpParser\NodeVisitor;
use PhpParser\NodeVisitorAbstract;

class DeprecatedHelperVisitor extends NodeVisitorAbstract implements NodeVisitor
{

private ?Node\Arg $deprecatedCall = null;

public function enterNode(Node $node)
{
if ($node instanceof Node\Expr\StaticCall) {
if ($node->class instanceof Node\Name\FullyQualified && $node->class->toString() === 'Drupal\Component\Utility\DeprecationHelper') {
$this->deprecatedCall = $node->getArgs()[2];
return null;
}
}

if ($this->deprecatedCall !== null && $node instanceof Node\Arg && $this->isSavedArgument($node)) {
$this->deprecatedCall = null;
Copy link
Owner

Choose a reason for hiding this comment

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

The null check is technically duplicated in the isSavedArgument method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was making phpstan happy, the property can be null.

return NodeTraverser::DONT_TRAVERSE_CURRENT_AND_CHILDREN;
}
Copy link
Owner

Choose a reason for hiding this comment

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

This is the magic, yeah? PHPStan doesn't analyze anything in the tree? Seems problematic and instead we should try adding information if possible. Probably not, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it stops parsing as discussed on slack.


return null;
}
Comment on lines +25 to +30
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a chance our property won't get unset for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Calling deprecation helper without arguments? But that is impossible.

Perhaps if while moving into the function something changes the node, so the argument is no longer on the same line and therefor not equal? Not sure how, and if that is possible.

But the effect would basically be if would check the array a few times extra, which should be quite cheap.

There might be a way to add some more checks when in the argument. Or do something like keeping a stack of the last static call and check if that call is nothing weird.

        if ($node instanceof Node\Expr\StaticCall) {
            $this->staticCall = $node; // this and check this?
            if ($node->class instanceof Node\Name\FullyQualified && $node->class->toString() === 'Drupal\Component\Utility\DeprecationHelper') {
                $this->deprecatedCall = $node->getArgs()[2];
                return null;
            }
        }

But that seems like overkill.


private function isSavedArgument(Node\Arg $node): bool
{
if ($this->deprecatedCall !== null && $node->getAttributes() === $this->deprecatedCall->getAttributes()) {
return true;
Copy link
Owner

Choose a reason for hiding this comment

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

Okay so when we detect a static call on the methods, we fetch and stash the callable for the deprecated case. Then when then node parser reaches the argument we check and unset.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes!

}
return false;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
<?php

declare(strict_types=1);

namespace mglaman\PHPStanDrupal\Tests\Parser\DeprecatedHelperVisitor;

use mglaman\PHPStanDrupal\Tests\DrupalRuleTestCase;
use PHPStan\Rules\Deprecations\CallToDeprecatedFunctionRule;
use PHPStan\Rules\Deprecations\DeprecatedScopeHelper;
use PHPStan\Rules\Rule;

final class DeprecatedHelperVisitorTest extends DrupalRuleTestCase {

protected function getRule(): Rule
{
// @phpstan-ignore-next-line
return new CallToDeprecatedFunctionRule(
self::createReflectionProvider(),
self::getContainer()->getByType(DeprecatedScopeHelper::class)
);
}

public function testCustomScope(): void
{
require_once __DIR__ . '/data/deprecated-data-definition.php';
$this->analyse(
[
__DIR__ . '/data/deprecation-helper-test.php',
],
[
[
'Call to deprecated function Deprecated\deprecated_function_call().',
16,
],
]
);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

namespace Deprecated;

/**
* @deprecated
*/
function deprecated_function_call(): void {

}

Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
<?php

namespace GroupLegacy;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be a more sane namespace i guess

Copy link
Owner

Choose a reason for hiding this comment

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

It's good enough for the test data :)


use Drupal\Component\Utility\DeprecationHelper;
use function Deprecated\deprecated_function_call;

final class FooTest {

public function methodCallingThings(): void {

\Drupal\Component\Utility\DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.1.0', fn() => deprecated_function_call(), fn() => count([]));

DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.1.0', fn() => deprecated_function_call(), fn() => count([]));

deprecated_function_call();

DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.1.0', function() {
deprecated_function_call();
}, function() {
count([]);
});

}

}