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

feat: Refactor ILogger::* constants to PSR log levels #6

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
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
10 changes: 10 additions & 0 deletions config/nextcloud-31/nextcloud-31-deprecations.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

declare(strict_types=1);

use ChristophWurst\Nextcloud\Rector\Rector\ILoggerToPsrLoggerLevelsRector;
use Rector\Config\RectorConfig;

return static function (RectorConfig $rectorConfig): void {
$rectorConfig->rule(ILoggerToPsrLoggerLevelsRector::class);
};
81 changes: 81 additions & 0 deletions src/Rector/ILoggerToPsrLoggerLevelsRector.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
<?php

declare(strict_types=1);

/*
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-only
*/

namespace ChristophWurst\Nextcloud\Rector\Rector;

use PhpParser\Node;
use PhpParser\Node\Expr\ClassConstFetch;
use PhpParser\Node\Expr\MethodCall;
use PhpParser\Node\Identifier;
use PhpParser\Node\Name\FullyQualified;
use Rector\Rector\AbstractRector;
use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;

use function array_slice;

class ILoggerToPsrLoggerLevelsRector extends AbstractRector
{
/**
* @return array<class-string<Node>>
*/
public function getNodeTypes(): array
{
return [MethodCall::class];
}

public function refactor(Node $node): ?Node
{
if (!($node instanceof MethodCall)) {
return null;
}
$methodCallName = $this->getName($node->name);
if ($methodCallName !== 'log') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it’s only for log method maybe we should directly replace with the proper method? debug/error/whatever?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right. If someone uses the logger with a static log level it should be the dedicated method anyway.

The use case to migrate with this rector is the use of dynamic log levels. For example in https://github.com/nextcloud/server/pull/47510/files.

This means we don't even need a custom rector. We can just replace the ILogger constant with a LogLevel constant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But that does not work in all situations I think. If an app is using the ILogger:: constant to compare against log content or configured loglevel, it needs the ILogger one and not the LogLevel one.
This is why we intend on undeprecating ILogger:: const in 31.
See nextcloud/server#47978

return null;
}
if (
!($node->args[0] instanceof Node\Arg)
|| !($node->args[0]->value instanceof ClassConstFetch)
|| $this->getName($node->args[0]->value->class) !== 'OCP\ILogger'
) {
return null;
}

return new MethodCall(
$node->var,
'log',
[
new Node\Arg(new ClassConstFetch(
new FullyQualified('Psr\Log\LogLevel'),
new Identifier(match ($this->getName($node->args[0]->value->name)) {
'DEBUG' => 'DEBUG',
'INFO' => 'INFO',
'WARN' => 'WARNING',
'FATAL' => 'EMERGENCY',
default => 'ERROR',
}),
)),
...array_slice($node->args, 1),
],
);
}

public function getRuleDefinition(): RuleDefinition
{
return new RuleDefinition(
'Change method calls from set* to change*.',
[
new CodeSample(
'\OC::$server->get(IAppConfig::class);',
'\OCP\Server::get(IAppConfig::class);',
),
],
);
}
}
1 change: 1 addition & 0 deletions src/Set/NextcloudSets.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ final class NextcloudSets implements SetListInterface
public const NEXTCLOUD_25 = __DIR__ . '/../../config/nextcloud-25/nextcloud-25-deprecations.php';
public const NEXTCLOUD_26 = __DIR__ . '/../../config/nextcloud-26/nextcloud-26-deprecations.php';
public const NEXTCLOUD_27 = __DIR__ . '/../../config/nextcloud-27/nextcloud-27-deprecations.php';
public const NEXTCLOUD_31 = __DIR__ . '/../../config/nextcloud-31/nextcloud-31-deprecations.php';
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php
namespace Foo\Bar;

use Psr\Log\LoggerInterface;
use Psr\Log\LogLevel;
use OCP\ILogger;

class Foo {
public function __construct(private LoggerInterface $logger) {}

public function logGenerically(): void {
$this->logger->log(LogLevel::ALERT, 'alarm!');
$this->logger->log(ILogger::ERROR, 'warn!');
$this->logger->log(ILogger::DEBUG, 'debug!');
}
}
?>
-----
<?php
namespace Foo\Bar;

use Psr\Log\LoggerInterface;
use Psr\Log\LogLevel;
use OCP\ILogger;

class Foo {
public function __construct(private LoggerInterface $logger) {}

public function logGenerically(): void {
$this->logger->log(LogLevel::ALERT, 'alarm!');
$this->logger->log(\Psr\Log\LogLevel::ERROR, 'warn!');
$this->logger->log(\Psr\Log\LogLevel::DEBUG, 'debug!');
}
}
?>
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
<?php

declare(strict_types=1);

namespace ChristophWurst\Nextcloud\Rector\Test\Rector\OcpUtilAddScriptRector;

use Iterator;
use PHPUnit\Framework\Attributes\DataProvider;
use Rector\Testing\PHPUnit\AbstractRectorTestCase;

final class ILoggerToPsrLoggerLevelsRectorTest extends AbstractRectorTestCase
{
#[DataProvider('provideData')]
public function test(string $filePath): void
{
$this->doTestFile($filePath);
}

public static function provideData(): Iterator
{
return self::yieldFilesFromDirectory(__DIR__ . '/Fixture');
}

public function provideConfigFilePath(): string
{
return __DIR__ . '/config/config.php';
}
}
11 changes: 11 additions & 0 deletions tests/Rector/ILoggerToPsrLoggerLevelsRector/config/config.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

declare(strict_types=1);

use ChristophWurst\Nextcloud\Rector\Rector\ILoggerToPsrLoggerLevelsRector;
use Rector\Config\RectorConfig;

return RectorConfig::configure()
->withRules([
ILoggerToPsrLoggerLevelsRector::class,
]);
Loading