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.

Sign up for GitHub

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

Fix: double deprecated calls when already refactored #288

Merged
merged 2 commits into from
Jan 17, 2024
Merged

Fix: double deprecated calls when already refactored #288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fb8b5f
Fix double deprecated calls when already refactored
bbrala Jan 16, 2024
f486107
Codestyle fixes, sure.
bbrala Jan 16, 2024
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
22 changes: 11 additions & 11 deletions src/Drupal8/Rector/Deprecation/DBRector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,37 +98,37 @@ public function refactor(Node $node): ?Node
{
assert($node instanceof Node\Stmt\Expression);

$isFuncCall = $node->expr instanceof Node\Expr\FuncCall;
$isMethodCall = $node->expr instanceof Node\Expr\MethodCall;
$isAssignedFuncCall = $node->expr instanceof Node\Expr\Assign && $node->expr->expr instanceof Node\Expr\FuncCall;
$isFuncCall = $node->expr instanceof Expr\FuncCall;
$isMethodCall = $node->expr instanceof MethodCall;
$isAssignedFuncCall = $node->expr instanceof Expr\Assign && $node->expr->expr instanceof Expr\FuncCall;
if (!$isFuncCall && !$isAssignedFuncCall && !$isMethodCall) {
return null;
}

foreach ($this->configuration as $configuration) {
if ($node->expr instanceof Node\Expr\FuncCall && $this->getName($node->expr->name) !== $configuration->getDeprecatedMethodName()) {
if ($node->expr instanceof Expr\FuncCall && $this->getName($node->expr->name) !== $configuration->getDeprecatedMethodName()) {
continue;
}

if ($node->expr instanceof Node\Expr\Assign && $node->expr->expr instanceof Node\Expr\FuncCall && $this->getName($node->expr->expr->name) !== $configuration->getDeprecatedMethodName()) {
if ($node->expr instanceof Expr\Assign && $node->expr->expr instanceof Expr\FuncCall && $this->getName($node->expr->expr->name) !== $configuration->getDeprecatedMethodName()) {
continue;
}

if ($node->expr instanceof Node\Expr\FuncCall) {
if ($node->expr instanceof Expr\FuncCall) {
$methodCall = $this->getMethodCall($node->expr, $node, $configuration);
$node->expr = $methodCall;

return $node;
}

if ($node->expr instanceof Node\Expr\Assign && $node->expr->expr instanceof Node\Expr\FuncCall) {
if ($node->expr instanceof Expr\Assign && $node->expr->expr instanceof Expr\FuncCall) {
$methodCall = $this->getMethodCall($node->expr->expr, $node, $configuration);
$node->expr->expr = $methodCall;

return $node;
}

if ($node->expr instanceof Node\Expr\MethodCall) {
if ($node->expr instanceof MethodCall) {
$funcCall = $this->findRootFuncCallForMethodCall($node->expr);
if ($funcCall === null || $this->getName($funcCall->name) !== $configuration->getDeprecatedMethodName()) {
continue;
Expand Down Expand Up @@ -187,7 +187,7 @@ public function replaceFuncCallForMethodCall(MethodCall $expr, MethodCall $metho
return null;
}

public function getMethodCall(Node\Expr\FuncCall $expr, Node\Stmt\Expression $statement, DBConfiguration $configuration): Node\Expr\MethodCall
public function getMethodCall(Expr\FuncCall $expr, Node\Stmt\Expression $statement, DBConfiguration $configuration): MethodCall
{
// TODO: Check if we have are in a class and inject \Drupal\Core\Database\Connection
// TODO: Check if we have are in a class and don't have access to the container, use `\Drupal\core\Database\Database::getConnection()`.
Expand Down Expand Up @@ -229,11 +229,11 @@ public function getMethodCall(Node\Expr\FuncCall $expr, Node\Stmt\Expression $st
$this->commentService->addDrupalRectorComment($statement, 'You will need to use `\Drupal\core\Database\Database::getConnection()` if you do not yet have access to the container here.');
}

$var = new Node\Expr\StaticCall($name, $call, $method_arguments);
$var = new Expr\StaticCall($name, $call, $method_arguments);

$method_name = new Node\Identifier(substr($configuration->getDeprecatedMethodName(), 3));

$methodCall = new Node\Expr\MethodCall($var, $method_name, $expr->args);
$methodCall = new MethodCall($var, $method_name, $expr->args);

return $methodCall;
}
Expand Down
2 changes: 1 addition & 1 deletion src/Drupal8/Rector/Deprecation/EntityLoadRector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public function refactor(Node $node): ?Node
if (!class_exists('\PhpParser\Node\ArrayItem')) {
$arrayItems = [new Node\Expr\ArrayItem($entity_id->value)];
} else {
$arrayItems = [new \PhpParser\Node\ArrayItem($entity_id->value)];
$arrayItems = [new Node\ArrayItem($entity_id->value)];
}

$reset_args = [
Expand Down
2 changes: 1 addition & 1 deletion src/Drupal8/Rector/Deprecation/FunctionalTestDefaultThemePropertyRector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ public function getNodeTypes(): array
}

/**
* @param \PhpParser\Node\Stmt\Class_ $node
* @param Node\Stmt\Class_ $node
*/
public function refactorWithScope(Node $node, Scope $scope): ?Node
{
Expand Down
2 changes: 1 addition & 1 deletion src/Drupal9/Rector/Deprecation/AssertLegacyTraitRector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ public function getNodeTypes(): array
* @param string $method
* @param array<Arg|VariadicPlaceholder> $args
*
* @return \PhpParser\Node\Expr\MethodCall
* @return Node\Expr\MethodCall
*/
protected function createAssertSessionMethodCall(string $method, array $args): Node\Expr\MethodCall
{
Expand Down
2 changes: 1 addition & 1 deletion src/Drupal9/Rector/Deprecation/AssertNoFieldByNameRector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ public function refactor(Node $node): ?Node
* @param string $method
* @param array<Arg|VariadicPlaceholder> $args
*
* @return \PhpParser\Node\Expr\MethodCall
* @return Node\Expr\MethodCall
*/
protected function createAssertSessionMethodCall(string $method, array $args): Node\Expr\MethodCall
{
Expand Down
2 changes: 1 addition & 1 deletion src/Drupal9/Rector/Deprecation/UiHelperTraitDrupalPostFormRector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public function getNodeTypes(): array
}

/**
* @param \PhpParser\Node\Expr\MethodCall $node
* @param Node\Expr\MethodCall $node
*
* @throws ShouldNotHappenException
*
Expand Down
2 changes: 1 addition & 1 deletion src/Drupal9/Rector/Property/ProtectedStaticModulesPropertyRector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public function getNodeTypes(): array
}

/**
* @param \PhpParser\Node\Stmt\Property $node
* @param Node\Stmt\Property $node
*/
public function refactor(Node $node): ?Node
{
Expand Down
31 changes: 31 additions & 0 deletions src/Rector/AbstractDrupalCoreRector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
use DrupalRector\Contract\VersionedConfigurationInterface;
use PhpParser\Node;
use PhpParser\Node\Expr\ArrowFunction;
use PHPStan\Reflection\MethodReflection;
use Rector\Contract\Rector\ConfigurableRectorInterface;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\Rector\AbstractRector;

abstract class AbstractDrupalCoreRector extends AbstractRector implements ConfigurableRectorInterface
Expand All @@ -29,6 +31,31 @@ public function configure(array $configuration): void
$this->configuration = $configuration;
}

protected function isInBackwardsCompatibleCall(Node $node): bool
{
if (!class_exists(DeprecationHelper::class)) {
return false;
}

$scope = $node->getAttribute(AttributeKey::SCOPE);

$callStack = $scope->getFunctionCallStackWithParameters();
if (count($callStack) === 0) {
return false;
}
[$function, $parameter] = $callStack[0];
if (!$function instanceof MethodReflection) {
return false;
}
if ($function->getName() !== 'backwardsCompatibleCall'
|| $function->getDeclaringClass()->getName() !== DeprecationHelper::class
) {
return false;
}

return $parameter !== null && $parameter->getName() === 'deprecatedCallable';
}
Comment on lines +34 to +57
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does Rector have a node visitor to just stop processing so that it doesn't handle nodes in this execution tree?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Think so yes. My old fix for phpstan would probably also work. This seemed easier is 0retty much why I used this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we're calling it in our base class, which all rules should extend. This is fine, but could lead to a "bug" if a rule didn't. But probably a minimal problem

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

valid point. But if it doesn't extend this class, it should not add the deprecation helper. But you are right though, it could slip through if we need some sort of exotic way to do things. Still would probably use the base class for the helpers.


public function refactor(Node $node)
{
$drupalVersion = str_replace(['.x-dev', '-dev'], '.0', \Drupal::VERSION);
Expand All @@ -38,6 +65,10 @@ public function refactor(Node $node)
continue;
}

if ($this->isInBackwardsCompatibleCall($node)) {
continue;
}

$result = $this->refactorWithConfiguration($node, $configuration);

// Skip if no result.
Expand Down
4 changes: 4 additions & 0 deletions ...rc/Drupal10/Rector/Deprecation/WatchdogExceptionRector/fixture/watchdog_exception.php.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ function advanced() {
watchdog_exception('update', $exception, 'My custom message @foo', ['@foo' => 'bar'], RfcLogLevel::CRITICAL, 'http://example.com');

watchdog_exception('update', $exception, 'My custom message @foo', ['@foo' => 'bar']);

\Drupal\Component\Utility\DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.1.0', fn() => \Drupal\Core\Utility\Error::logException(\Drupal::logger('update'), $exception), fn() => watchdog_exception('update', $exception));
}
?>
-----
Expand All @@ -27,5 +29,7 @@ function advanced() {
\Drupal\Component\Utility\DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.1.0', fn() => \Drupal\Core\Utility\Error::logException(\Drupal::logger('update'), $exception, 'My custom message @foo', ['@foo' => 'bar', 'link' => 'http://example.com'], RfcLogLevel::CRITICAL), fn() => watchdog_exception('update', $exception, 'My custom message @foo', ['@foo' => 'bar', 'link' => 'http://example.com'], RfcLogLevel::CRITICAL, 'http://example.com'));

\Drupal\Component\Utility\DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.1.0', fn() => \Drupal\Core\Utility\Error::logException(\Drupal::logger('update'), $exception, 'My custom message @foo', ['@foo' => 'bar']), fn() => watchdog_exception('update', $exception, 'My custom message @foo', ['@foo' => 'bar']));

\Drupal\Component\Utility\DeprecationHelper::backwardsCompatibleCall(\Drupal::VERSION, '10.1.0', fn() => \Drupal\Core\Utility\Error::logException(\Drupal::logger('update'), $exception), fn() => watchdog_exception('update', $exception));
}
?>