Skip to content

Commit

Permalink
refactor: Migrate away from deprecated ILogger to PSR-3
Browse files Browse the repository at this point in the history 
Mostly replace `ILogger` with `LoggerInterface` and some minor cleanup (constructor property promotion).

Some places used the deprecated `logException`, this is easy to migrate: Simply use the appropriate log level on the logger and place the exception under the `exception` key in the context.

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
  • Loading branch information
@susnux
susnux committed Aug 20, 2024
1 parent 8cc9ccf commit 7444b39
Show file tree
Hide file tree
Showing 7 changed files with 43 additions and 44 deletions.
18 changes: 9 additions & 9 deletions lib/Controller/DocumentController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
use \OCP\IRequest;
use \OCP\IConfig;
use \OCP\IL10N;
use \OCP\ILogger;
use \OCP\AppFramework\Http\ContentSecurityPolicy;
use \OCP\AppFramework\Http\TemplateResponse;
use \OCA\Officeonline\AppConfig;
Expand All @@ -37,6 +36,7 @@
use OCP\Share\Exceptions\ShareNotFound;
use OCP\Share\IManager;
use OC\Files\Type\TemplateManager;
use Psr\Log\LoggerInterface;

class DocumentController extends Controller {
/** @var string */
Expand All @@ -47,7 +47,7 @@ class DocumentController extends Controller {
private $settings;
/** @var AppConfig */
private $appConfig;
/** @var ILogger */
/** @var LoggerInterface */
private $logger;
/** @var IManager */
private $shareManager;
Expand Down Expand Up @@ -77,7 +77,7 @@ class DocumentController extends Controller {
* @param IRootFolder $rootFolder
* @param ISession $session
* @param string $UserId
* @param ILogger $logger
* @param LoggerInterface $logger
*/
public function __construct(
$appName,
Expand All @@ -90,7 +90,7 @@ public function __construct(
IRootFolder $rootFolder,
ISession $session,
$UserId,
ILogger $logger,
LoggerInterface $logger,
\OCA\Officeonline\TemplateManager $templateManager,
FederationService $federationService,
Helper $helper
Expand Down Expand Up @@ -145,7 +145,7 @@ public function extAppGetData($fileId) {
'token' => $token
];
} catch (\Exception $e) {
$this->logger->logException($e, ['app' => 'officeonline']);
$this->logger->error($e->getMessage(), ['app' => 'officeonline', 'exception' => $e]);
}
}
}
Expand Down Expand Up @@ -199,7 +199,7 @@ public function open($fileId) {
$this->logger->warning('Failed to connect to remote collabora instance for ' . $fileId);
}
} catch (\Exception $e) {
$this->logger->logException($e, ['app' => 'officeonline']);
$this->logger->error($e->getMessage(), ['app' => 'officeonline', 'exception' => $e]);
$params = [
'errors' => [['error' => $e->getMessage()]]
];
Expand Down Expand Up @@ -274,7 +274,7 @@ public function index($fileId, $path = null) {
$response->addHeader('Pragma', 'no-cache');
return $response;
} catch (\Exception $e) {
$this->logger->logException($e, ['app' => 'officeonline']);
$this->logger->error($e->getMessage(), ['app' => 'officeonline', 'exception' => $e]);
return $this->renderErrorPage('Failed to open the requested file.');
}

Expand Down Expand Up @@ -396,7 +396,7 @@ public function publicPage($shareToken, $fileName, $fileId) {
return $response;
}
} catch (\Exception $e) {
$this->logger->logException($e, ['app' => 'officeonline']);
$this->logger->error($e->getMessage(), ['app' => 'officeonline', 'exception' => $e]);
}

return $this->renderErrorPage('Failed to open the requested file.');
Expand Down Expand Up @@ -467,7 +467,7 @@ public function remote($shareToken, $remoteServer, $remoteServerToken, $filePath
} catch (ShareNotFound $e) {
return new TemplateResponse('core', '404', [], 'guest');
} catch (\Exception $e) {
$this->logger->logException($e, ['app' => 'officeonline']);
$this->logger->error($e->getMessage(), ['app' => 'officeonline', 'exception' => $e]);
return $this->renderErrorPage('Failed to open the requested file.');
}

Expand Down
8 changes: 4 additions & 4 deletions lib/Controller/SettingsController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\Http\JSONResponse;
use OCP\ILogger;
use \OCP\IRequest;
use \OCP\IL10N;
use OCA\Officeonline\AppConfig;
use Psr\Log\LoggerInterface;

class SettingsController extends Controller {

Expand All @@ -30,15 +30,15 @@ class SettingsController extends Controller {
private $appConfig;
/** @var DiscoveryManager */
private $discoveryManager;
/** @var ILogger */
/** @var LoggerInterface */
private $logger;

public function __construct($appName,
IRequest $request,
IL10N $l10n,
AppConfig $appConfig,
DiscoveryManager $discoveryManager,
ILogger $logger
LoggerInterface $logger
) {
parent::__construct($appName, $request);
$this->l10n = $l10n;
Expand All @@ -55,7 +55,7 @@ public function checkSettings(): DataResponse {
try {
$response = $this->discoveryManager->fetchFromRemote();
} catch (Exception $e) {
$this->logger->logException($e, ['app' => 'officeonline']);
$this->logger->error('Could not fetch discovery details', ['app' => 'officeonline', 'exception' => $e]);
return new DataResponse([
'status' => $e->getCode(),
'message' => 'Could not fetch discovery details'
Expand Down
26 changes: 13 additions & 13 deletions lib/Controller/WopiController.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@
use OCP\Files\NotFoundException;
use OCP\Files\NotPermittedException;
use OCP\IConfig;
use OCP\ILogger;
use OCP\IRequest;
use OCP\IURLGenerator;
use OCP\AppFramework\Http\StreamResponse;
Expand All @@ -64,6 +63,7 @@
use OCP\PreConditionNotMetException;
use OCP\Share\Exceptions\ShareNotFound;
use OCP\Share\IManager;
use Psr\Log\LoggerInterface;

class WopiController extends Controller {
/** @var IRootFolder */
Expand All @@ -80,7 +80,7 @@ class WopiController extends Controller {
private $userManager;
/** @var WopiMapper */
private $wopiMapper;
/** @var ILogger */
/** @var LoggerInterface */
private $logger;
/** @var TemplateManager */
private $templateManager;
Expand Down Expand Up @@ -118,7 +118,7 @@ class WopiController extends Controller {
* @param TokenManager $tokenManager
* @param IUserManager $userManager
* @param WopiMapper $wopiMapper
* @param ILogger $logger
* @param LoggerInterface $logger
* @param TemplateManager $templateManager
* @param IManager $shareManager
* @param UserScopeService $userScopeService
Expand All @@ -136,7 +136,7 @@ public function __construct(
TokenManager $tokenManager,
IUserManager $userManager,
WopiMapper $wopiMapper,
ILogger $logger,
LoggerInterface $logger,
TemplateManager $templateManager,
IManager $shareManager,
UserScopeService $userScopeService,
Expand Down Expand Up @@ -200,7 +200,7 @@ public function checkFileInfo($fileId, $access_token) {
$this->logger->debug($e->getMessage(), ['app' => 'officeonline', '']);
return new JSONResponse([], Http::STATUS_NOT_FOUND);
} catch (Exception $e) {
$this->logger->logException($e, ['app' => 'officeonline']);
$this->logger->error($e->getMessage(), ['app' => 'officeonline', 'exception' => $e]);
return new JSONResponse([], Http::STATUS_NOT_FOUND);
}

Expand Down Expand Up @@ -329,7 +329,7 @@ public function getFile($fileId,
$response->addHeader('Content-Type', 'application/octet-stream');
return $response;
} catch (Exception $e) {
$this->logger->logException($e, ['level' => ILogger::ERROR, 'app' => 'officeonline', 'message' => 'getFile failed']);
$this->logger->error('getFile failed', ['exception' => $e, 'app' => 'officeonline']);
return new JSONResponse([], Http::STATUS_FORBIDDEN);
}
}
Expand Down Expand Up @@ -457,7 +457,7 @@ private function lock(Wopi $wopi): JSONResponse {
$response->setStatus(Http::STATUS_CONFLICT);
return $response;
} catch (Exception $e) {
$this->logger->logException($e);
$this->logger->error($e->getMessage(), ['exception' => $e]);
return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR);
}
}
Expand Down Expand Up @@ -485,7 +485,7 @@ private function unlock(Wopi $wopi): JSONResponse {
} catch (NoLockProviderException|PreConditionNotMetException $e) {
return new JSONResponse([], Http::STATUS_BAD_REQUEST);
} catch (Exception $e) {
$this->logger->logException($e);
$this->logger->error($e->getMessage(), ['exception' => $e]);
return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR);
}
}
Expand All @@ -509,7 +509,7 @@ private function refreshLock(Wopi $wopi): JSONResponse {
$response->setStatus(Http::STATUS_CONFLICT);
return $response;
} catch (Exception $e) {
$this->logger->logException($e);
$this->logger->error($e->getMessage(), ['exception' => $e]);
return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR);
}
}
Expand Down Expand Up @@ -616,7 +616,7 @@ public function putFile($fileId,
});
});
} catch (LockedException $e) {
$this->logger->logException($e);
$this->logger->error($e->getMessage(), ['exception' => $e]);
return new JSONResponse(['message' => 'File locked'], Http::STATUS_INTERNAL_SERVER_ERROR);
}

Expand All @@ -636,7 +636,7 @@ public function putFile($fileId,
}
return new JSONResponse(['LastModifiedTime' => Helper::toISO8601($file->getMTime())]);
} catch (Exception $e) {
$this->logger->logException($e, ['level' => ILogger::ERROR, 'app' => 'officeonline', 'message' => 'getFile failed']);
$this->logger->errpr('getFile failed', ['app' => 'officeonline', 'exception' => $e]);
return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR);
}
}
Expand Down Expand Up @@ -839,7 +839,7 @@ public function postFile($fileId, $access_token) {

return new JSONResponse([ 'Name' => $file->getName(), 'Url' => $url ], Http::STATUS_OK);
} catch (Exception $e) {
$this->logger->logException($e, ['level' => ILogger::ERROR, 'app' => 'officeonline', 'message' => 'putRelativeFile failed']);
$this->logger->error('putRelativeFile failed', ['app' => 'officeonline', 'exception' => $e]);
return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR);
}
}
Expand Down Expand Up @@ -928,7 +928,7 @@ public function getTemplate($fileId, $access_token) {
$response->addHeader('Content-Type', 'application/octet-stream');
return $response;
} catch (Exception $e) {
$this->logger->logException($e, ['level' => ILogger::ERROR, 'app' => 'officeonline', 'message' => 'getTemplate failed']);
$this->logger->error('getTemplate failed', ['app' => 'officeonline', 'exception' => $e]);
return new JSONResponse([], Http::STATUS_INTERNAL_SERVER_ERROR);
}
}
Expand Down
6 changes: 3 additions & 3 deletions lib/Db/WopiMapper.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
use OCP\AppFramework\Utility\ITimeFactory;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\ILogger;
use OCP\Security\ISecureRandom;
use Psr\Log\LoggerInterface;

class WopiMapper extends QBMapper {
// Tokens expire after this many seconds (not defined by WOPI specs).
Expand All @@ -36,15 +36,15 @@ class WopiMapper extends QBMapper {
/** @var ISecureRandom */
private $random;

/** @var ILogger */
/** @var LoggerInterface */
private $logger;

/** @var ITimeFactory */
private $timeFactory;

public function __construct(IDBConnection $db,
ISecureRandom $random,
ILogger $logger,
LoggerInterface $logger,
ITimeFactory $timeFactory) {
parent::__construct($db, 'officeonline_wopi', Wopi::class);

Expand Down
12 changes: 6 additions & 6 deletions lib/Hooks/WopiLockHooks.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
use OCP\Files\IRootFolder;
use OCP\Files\Node;
use OCP\Files\NotFoundException;
use OCP\ILogger;
use OCP\Lock\ILockingProvider;
use OCP\Lock\LockedException;
use Psr\Log\LoggerInterface;

class WopiLockHooks {
private $rootFolder;
Expand All @@ -29,11 +29,11 @@ class WopiLockHooks {
*/
private $lockBypass;
/**
* @var ILogger
* @var LoggerInterface
*/
private $logger;

public function __construct(IRootFolder $rootFolder, ITimeFactory $timeFactory, ILogger $logger, WopiLockMapper $lockMapper) {
public function __construct(IRootFolder $rootFolder, ITimeFactory $timeFactory, LoggerInterface $logger, WopiLockMapper $lockMapper) {
$this->rootFolder = $rootFolder;
$this->lockMapper = $lockMapper;
$this->timeFactory = $timeFactory;
Expand All @@ -59,11 +59,11 @@ public function preWrite(Node $node) {
$node->lock(ILockingProvider::LOCK_SHARED);
}
} catch (InvalidPathException $e) {
$this->logger->logException($e);
$this->logger->error('Invalid path', ['exception' => $e]);
} catch (NotFoundException $e) {
$this->logger->debug('not a file');
$this->logger->debug('not a file', ['exception' => $e]);
} catch (LockedException $e) {
$this->logger->logException($e);
$this->logger->error('Node already locked', ['exception' => $e]);
}
}
}
Expand Down
11 changes: 5 additions & 6 deletions lib/Preview/Office.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@
use OCA\Officeonline\Capabilities;
use OCP\Http\Client\IClientService;
use OCP\IConfig;
use OCP\ILogger;
use OCP\Image;
use Psr\Log\LoggerInterface;

abstract class Office extends Provider {

Expand All @@ -39,10 +39,10 @@ abstract class Office extends Provider {
/** @var array */
private $capabilitites;

/** @var ILogger */
/** @var LoggerInterface */
private $logger;

public function __construct(IClientService $clientService, IConfig $config, Capabilities $capabilities, ILogger $logger) {
public function __construct(IClientService $clientService, IConfig $config, Capabilities $capabilities, LoggerInterface $logger) {
$this->clientService = $clientService;
$this->config = $config;
$this->capabilitites = $capabilities->getCapabilities()['officeonline'];
Expand Down Expand Up @@ -89,9 +89,8 @@ public function getThumbnail($path, $maxX, $maxY, $scalingup, $fileview) {
try {
$response = $client->post($this->getWopiURL(). '/lool/convert-to/png', $options);
} catch (\Exception $e) {
$this->logger->logException($e, [
'message' => 'Failed to convert file to preview',
'level' => ILogger::INFO,
$this->logger->info('Failed to convert file to preview', [
'exception' => $e,
'app' => 'officeonline',
]);
return false;
Expand Down
6 changes: 3 additions & 3 deletions lib/Service/FederationService.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,22 +33,22 @@
use OCP\Http\Client\IClientService;
use OCP\ICache;
use OCP\ICacheFactory;
use OCP\ILogger;
use Psr\Log\LoggerInterface;

class FederationService {

/** @var ICache */
private $cache;
/** @var IClientService */
private $clientService;
/** @var ILogger */
/** @var LoggerInterface */
private $logger;
/** @var TrustedServers */
private $trustedServers;
/** @var TokenManager */
private $tokenManager;

public function __construct(ICacheFactory $cacheFactory, IClientService $clientService, ILogger $logger, TokenManager $tokenManager) {
public function __construct(ICacheFactory $cacheFactory, IClientService $clientService, LoggerInterface $logger, TokenManager $tokenManager) {
$this->cache = $cacheFactory->createLocal('officeonline_remote/');
$this->clientService = $clientService;
$this->logger = $logger;
Expand Down

0 comments on commit 7444b39

Please sign in to comment.