Make Update notficiations strict and fix all inspections

Signed-off-by: Joas Schilling <coding@schilljs.com>

apps/updatenotification/lib/AppInfo/Application.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2018, Joas Schilling <coding@schilljs.com>
  *

apps/updatenotification/lib/Controller/AdminController.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -76,7 +77,7 @@ class AdminController extends Controller {
 	 * @param string $channel
 	 * @return DataResponse
 	 */
-	public function setChannel($channel) {
+	public function setChannel(string $channel): DataResponse {
 		Util::setChannel($channel);
 		$this->config->setAppValue('core', 'lastupdatedat', 0);
 		return new DataResponse(['status' => 'success', 'data' => ['message' => $this->l10n->t('Channel updated')]]);
@@ -85,7 +86,7 @@ class AdminController extends Controller {
 	/**
 	 * @return DataResponse
 	 */
-	public function createCredentials() {
+	public function createCredentials(): DataResponse {
 		// Create a new job and store the creation date
 		$this->jobList->add(ResetTokenBackgroundJob::class);
 		$this->config->setAppValue('core', 'updater.secret.created', $this->timeFactory->getTime());

apps/updatenotification/lib/Notification/BackgroundJob.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -90,7 +91,7 @@ class BackgroundJob extends TimedJob {
 	 * Check for ownCloud update
 	 */
 	protected function checkCoreUpdate() {
-		if (in_array($this->getChannel(), ['daily', 'git'], true)) {
+		if (\in_array($this->getChannel(), ['daily', 'git'], true)) {
 			// "These aren't the update channels you're looking for." - Ben Obi-Wan Kenobi
 			return;
 		}
@@ -102,10 +103,10 @@ class BackgroundJob extends TimedJob {
 			$errors = 1 + (int) $this->config->getAppValue('updatenotification', 'update_check_errors', 0);
 			$this->config->setAppValue('updatenotification', 'update_check_errors', $errors);
 
-			if (in_array($errors, $this->connectionNotifications, true)) {
+			if (\in_array($errors, $this->connectionNotifications, true)) {
 				$this->sendErrorNotifications($errors);
 			}
-		} else if (is_array($status)) {
+		} else if (\is_array($status)) {
 			$this->config->setAppValue('updatenotification', 'update_check_errors', 0);
 			$this->clearErrorNotifications();
 
@@ -178,26 +179,31 @@ class BackgroundJob extends TimedJob {
 		if ($lastNotification === $version) {
 			// We already notified about this update
 			return;
-		} else if ($lastNotification !== false) {
+		}
+
+		if ($lastNotification !== false) {
 			// Delete old updates
 			$this->deleteOutdatedNotifications($app, $lastNotification);
 		}
 
-
 		$notification = $this->notificationManager->createNotification();
-		$notification->setApp('updatenotification')
-			->setDateTime(new \DateTime())
-			->setObject($app, $version);
-
-		if ($visibleVersion !== '') {
-			$notification->setSubject('update_available', ['version' => $visibleVersion]);
-		} else {
-			$notification->setSubject('update_available');
-		}
+		try {
+			$notification->setApp('updatenotification')
+				->setDateTime(new \DateTime())
+				->setObject($app, $version);
 
-		foreach ($this->getUsersToNotify() as $uid) {
-			$notification->setUser($uid);
-			$this->notificationManager->notify($notification);
+			if ($visibleVersion !== '') {
+				$notification->setSubject('update_available', ['version' => $visibleVersion]);
+			} else {
+				$notification->setSubject('update_available');
+			}
+
+			foreach ($this->getUsersToNotify() as $uid) {
+				$notification->setUser($uid);
+				$this->notificationManager->notify($notification);
+			}
+		} catch (\InvalidArgumentException $e) {
+			return;
 		}
 
 		$this->config->setAppValue('updatenotification', $app, $version);
@@ -206,12 +212,12 @@ class BackgroundJob extends TimedJob {
 	/**
 	 * @return string[]
 	 */
-	protected function getUsersToNotify() {
+	protected function getUsersToNotify(): array {
 		if ($this->users !== null) {
 			return $this->users;
 		}
 
-		$notifyGroups = json_decode($this->config->getAppValue('updatenotification', 'notify_groups', '["admin"]'), true);
+		$notifyGroups = (array) json_decode($this->config->getAppValue('updatenotification', 'notify_groups', '["admin"]'), true);
 		$this->users = [];
 		foreach ($notifyGroups as $group) {
 			$groupToNotify = $this->groupManager->get($group);
@@ -235,15 +241,19 @@ class BackgroundJob extends TimedJob {
 	 */
 	protected function deleteOutdatedNotifications($app, $version) {
 		$notification = $this->notificationManager->createNotification();
-		$notification->setApp('updatenotification')
-			->setObject($app, $version);
+		try {
+			$notification->setApp('updatenotification')
+				->setObject($app, $version);
+		} catch (\InvalidArgumentException $e) {
+			return;
+		}
 		$this->notificationManager->markProcessed($notification);
 	}
 
 	/**
 	 * @return VersionCheck
 	 */
-	protected function createVersionCheck() {
+	protected function createVersionCheck(): VersionCheck {
 		return new VersionCheck(
 			$this->client,
 			$this->config
@@ -253,7 +263,7 @@ class BackgroundJob extends TimedJob {
 	/**
 	 * @return string
 	 */
-	protected function getChannel() {
+	protected function getChannel(): string {
 		return \OC_Util::getChannel();
 	}
 

apps/updatenotification/lib/Notification/Notifier.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -33,6 +34,7 @@ use OCP\L10N\IFactory;
 use OCP\Notification\IManager;
 use OCP\Notification\INotification;
 use OCP\Notification\INotifier;
+use OCP\Util;
 
 class Notifier implements INotifier {
 
@@ -84,9 +86,9 @@ class Notifier implements INotifier {
 	 * @throws \InvalidArgumentException When the notification was not prepared by a notifier
 	 * @since 9.0.0
 	 */
-	public function prepare(INotification $notification, $languageCode) {
+	public function prepare(INotification $notification, $languageCode): INotification {
 		if ($notification->getApp() !== 'updatenotification') {
-			throw new \InvalidArgumentException();
+			throw new \InvalidArgumentException('Unknown app id');
 		}
 
 		$l = $this->l10NFactory->get('updatenotification', $languageCode);
@@ -94,7 +96,7 @@ class Notifier implements INotifier {
 			$errors = (int) $this->config->getAppValue('updatenotification', 'update_check_errors', 0);
 			if ($errors === 0) {
 				$this->notificationManager->markProcessed($notification);
-				throw new \InvalidArgumentException();
+				throw new \InvalidArgumentException('Update checked worked again');
 			}
 
 			$notification->setParsedSubject($l->t('The update server could not be reached since %d days to check for new updates.', [$errors]))
@@ -145,14 +147,14 @@ class Notifier implements INotifier {
 	protected function updateAlreadyInstalledCheck(INotification $notification, $installedVersion) {
 		if (version_compare($notification->getObjectId(), $installedVersion, '<=')) {
 			$this->notificationManager->markProcessed($notification);
-			throw new \InvalidArgumentException();
+			throw new \InvalidArgumentException('Update already installed');
 		}
 	}
 
 	/**
 	 * @return bool
 	 */
-	protected function isAdmin() {
+	protected function isAdmin(): bool {
 		$user = $this->userSession->getUser();
 
 		if ($user instanceof IUser) {
@@ -162,11 +164,11 @@ class Notifier implements INotifier {
 		return false;
 	}
 
-	protected function getCoreVersions() {
-		return implode('.', \OCP\Util::getVersion());
+	protected function getCoreVersions(): string {
+		return implode('.', Util::getVersion());
 	}
 
-	protected function getAppVersions() {
+	protected function getAppVersions(): array {
 		return \OC_App::getAppVersions();
 	}
 

apps/updatenotification/lib/ResetTokenBackgroundJob.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *

apps/updatenotification/lib/Settings/Admin.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -56,7 +57,7 @@ class Admin implements ISettings {
 	/**
 	 * @return TemplateResponse
 	 */
-	public function getForm() {
+	public function getForm(): TemplateResponse {
 		$lastUpdateCheckTimestamp = $this->config->getAppValue('core', 'lastupdatedat');
 		$lastUpdateCheck = $this->dateTimeFormatter->formatDateTime($lastUpdateCheckTimestamp);
 
@@ -99,7 +100,7 @@ class Admin implements ISettings {
 	/**
 	 * @return string the section ID, e.g. 'sharing'
 	 */
-	public function getSection() {
+	public function getSection(): string {
 		return 'server';
 	}
 
@@ -110,7 +111,7 @@ class Admin implements ISettings {
 	 *
 	 * E.g.: 70
 	 */
-	public function getPriority() {
+	public function getPriority(): int {
 		return 1;
 	}
 }

apps/updatenotification/lib/UpdateChecker.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -40,18 +41,18 @@ class UpdateChecker {
 	/**
 	 * @return array
 	 */
-	public function getUpdateState() {
+	public function getUpdateState(): array {
 		$data = $this->updater->check();
 		$result = [];
 
-		if(isset($data['version']) && $data['version'] !== '' && $data['version'] !== []) {
+		if (isset($data['version']) && $data['version'] !== '' && $data['version'] !== []) {
 			$result['updateAvailable'] = true;
 			$result['updateVersion'] = $data['versionstring'];
 			$result['updaterEnabled'] = $data['autoupdater'] === '1';
-			if(substr($data['web'], 0, 8) === 'https://') {
+			if (strpos($data['web'], 'https://') === 0) {
 				$result['updateLink'] = $data['web'];
 			}
-			if(substr($data['url'], 0, 8) === 'https://') {
+			if (strpos($data['url'], 'https://') === 0) {
 				$result['downloadLink'] = $data['url'];
 			}
 
@@ -68,7 +69,7 @@ class UpdateChecker {
 		$data['array']['oc_updateState'] =  json_encode([
 			'updateAvailable' => true,
 			'updateVersion' => $this->getUpdateState()['updateVersion'],
-			'updateLink' => isset($this->getUpdateState()['updateLink']) ? $this->getUpdateState()['updateLink'] : '',
+			'updateLink' => $this->getUpdateState()['updateLink'] ?? '',
 		]);
 	}
 }

apps/updatenotification/templates/admin.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 	script('updatenotification', 'admin');
 	style('updatenotification', 'admin');
 
@@ -40,7 +41,7 @@
 		<?php if (!$isDefaultUpdateServerURL) { ?>
 			<br />
 			<em>
-				<?php p($l->t("A non-default update server is in use to be checked for updates:")); ?>
+				<?php p($l->t('A non-default update server is in use to be checked for updates:')); ?>
 				<code><?php p($updateServerURL); ?></code>
 			</em>
 		<?php } ?>
@@ -65,10 +66,10 @@
 	<p id="oca_updatenotification_groups">
 		<?php p($l->t('Notify members of the following groups about available updates:')); ?>
 		<input name="oca_updatenotification_groups_list" type="hidden" id="oca_updatenotification_groups_list" value="<?php p($_['notify_groups']) ?>" style="width: 400px"><br />
-		<em class="<?php if (!in_array($currentChannel, ['daily', 'git'])) p('hidden'); ?>">
+		<em class="<?php if (!\in_array($currentChannel, ['daily', 'git'], true)) { p('hidden'); } ?>">
 			<?php p($l->t('Only notification for app updates are available.')); ?>
-			<?php if ($currentChannel === 'daily') p($l->t('The selected update channel makes dedicated notifications for the server obsolete.')); ?>
-			<?php if ($currentChannel === 'git') p($l->t('The selected update channel does not support updates of the server.')); ?>
+			<?php if ($currentChannel === 'daily') { p($l->t('The selected update channel makes dedicated notifications for the server obsolete.')); } ?>
+			<?php if ($currentChannel === 'git') { p($l->t('The selected update channel does not support updates of the server.')); } ?>
 		</em>
 	</p>
 </form>

apps/updatenotification/tests/Controller/AdminControllerTest.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -26,17 +27,13 @@ namespace OCA\UpdateNotification\Tests\Controller;
 
 use OCA\UpdateNotification\Controller\AdminController;
 use OCA\UpdateNotification\ResetTokenBackgroundJob;
-use OCA\UpdateNotification\UpdateChecker;
 use OCP\AppFramework\Http\DataResponse;
-use OCP\AppFramework\Http\TemplateResponse;
 use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\IJobList;
 use OCP\IConfig;
-use OCP\IDateTimeFormatter;
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\Security\ISecureRandom;
-use OCP\Util;
 use Test\TestCase;
 
 class AdminControllerTest extends TestCase {

apps/updatenotification/tests/Notification/BackgroundJobTest.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -105,7 +106,7 @@ class BackgroundJobTest extends TestCase {
 		self::invokePrivate($job, 'run', [null]);
 	}
 
-	public function dataCheckCoreUpdate() {
+	public function dataCheckCoreUpdate(): array {
 		return [
 			['daily', null, null, null, null],
 			['git', null, null, null, null],
@@ -142,7 +143,7 @@ class BackgroundJobTest extends TestCase {
 	 * @param null|string $readableVersion
 	 * @param null|int $errorDays
 	 */
-	public function testCheckCoreUpdate($channel, $versionCheck, $version, $readableVersion, $errorDays) {
+	public function testCheckCoreUpdate(string $channel, $versionCheck, $version, $readableVersion, $errorDays) {
 		$job = $this->getJob([
 			'getChannel',
 			'createVersionCheck',
@@ -197,13 +198,13 @@ class BackgroundJobTest extends TestCase {
 				->method('clearErrorNotifications');
 			$job->expects($this->once())
 				->method('createNotifications')
-				->willReturn('core', $version, $readableVersion);
+				->with('core', $version, $readableVersion);
 		}
 
 		self::invokePrivate($job, 'checkCoreUpdate');
 	}
 
-	public function dataCheckAppUpdates() {
+	public function dataCheckAppUpdates(): array {
 		return [
 			[
 				['app1', 'app2'],
@@ -235,18 +236,18 @@ class BackgroundJobTest extends TestCase {
 			->method('getInstalledApps')
 			->willReturn($apps);
 
-		$job->expects($this->exactly(count($apps)))
+		$job->expects($this->exactly(\count($apps)))
 			->method('isUpdateAvailable')
 			->willReturnMap($isUpdateAvailable);
 
-		$mockedMethod = $job->expects($this->exactly(count($notifications)))
+		$mockedMethod = $job->expects($this->exactly(\count($notifications)))
 			->method('createNotifications');
-		call_user_func_array([$mockedMethod, 'withConsecutive'], $notifications);
+		\call_user_func_array([$mockedMethod, 'withConsecutive'], $notifications);
 
 		self::invokePrivate($job, 'checkAppUpdates');
 	}
 
-	public function dataCreateNotifications() {
+	public function dataCreateNotifications(): array {
 		return [
 			['app1', '1.0.0', '1.0.0', false, false, null, null],
 			['app2', '1.0.1', '1.0.0', '1.0.0', true, ['user1'], [['user1']]],
@@ -265,7 +266,7 @@ class BackgroundJobTest extends TestCase {
 	 * @param string[]|null $users
 	 * @param array|null $userNotifications
 	 */
-	public function testCreateNotifications($app, $version, $lastNotification, $callDelete, $createNotification, $users, $userNotifications) {
+	public function testCreateNotifications(string $app, string $version, $lastNotification, $callDelete, $createNotification, $users, $userNotifications) {
 		$job = $this->getJob([
 			'deleteOutdatedNotifications',
 			'getUsersToNotify',
@@ -319,12 +320,12 @@ class BackgroundJobTest extends TestCase {
 				->willReturnSelf();
 
 			if ($userNotifications !== null) {
-				$mockedMethod = $notification->expects($this->exactly(count($userNotifications)))
+				$mockedMethod = $notification->expects($this->exactly(\count($userNotifications)))
 					->method('setUser')
 					->willReturnSelf();
-				call_user_func_array([$mockedMethod, 'withConsecutive'], $userNotifications);
+				\call_user_func_array([$mockedMethod, 'withConsecutive'], $userNotifications);
 
-				$this->notificationManager->expects($this->exactly(count($userNotifications)))
+				$this->notificationManager->expects($this->exactly(\count($userNotifications)))
 					->method('notify')
 					->willReturn($notification);
 			}
@@ -340,7 +341,7 @@ class BackgroundJobTest extends TestCase {
 		self::invokePrivate($job, 'createNotifications', [$app, $version]);
 	}
 
-	public function dataGetUsersToNotify() {
+	public function dataGetUsersToNotify(): array {
 		return [
 			[['g1', 'g2'], ['g1' => null, 'g2' => ['u1', 'u2']], ['u1', 'u2']],
 			[['g3', 'g4'], ['g3' => ['u1', 'u2'], 'g4' => ['u2', 'u3']], ['u1', 'u2', 'u3']],
@@ -353,7 +354,7 @@ class BackgroundJobTest extends TestCase {
 	 * @param array $groupUsers
 	 * @param string[] $expected
 	 */
-	public function testGetUsersToNotify($groups, array $groupUsers, array $expected) {
+	public function testGetUsersToNotify(array $groups, array $groupUsers, array $expected) {
 		$job = $this->getJob();
 
 		$this->config->expects($this->once())
@@ -373,7 +374,7 @@ class BackgroundJobTest extends TestCase {
 			}
 			$groupMap[] = [$gid, $group];
 		}
-		$this->groupManager->expects($this->exactly(count($groups)))
+		$this->groupManager->expects($this->exactly(\count($groups)))
 			->method('get')
 			->willReturnMap($groupMap);
 
@@ -385,7 +386,7 @@ class BackgroundJobTest extends TestCase {
 		$this->assertEquals($expected, $result);
 	}
 
-	public function dataDeleteOutdatedNotifications() {
+	public function dataDeleteOutdatedNotifications(): array {
 		return [
 			['app1', '1.1.0'],
 			['app2', '1.2.0'],
@@ -397,7 +398,7 @@ class BackgroundJobTest extends TestCase {
 	 * @param string $app
 	 * @param string $version
 	 */
-	public function testDeleteOutdatedNotifications($app, $version) {
+	public function testDeleteOutdatedNotifications(string $app, string $version) {
 		$notification = $this->createMock(INotification::class);
 		$notification->expects($this->once())
 			->method('setApp')
@@ -423,7 +424,7 @@ class BackgroundJobTest extends TestCase {
 	 * @param string[] $userIds
 	 * @return IUser[]|\PHPUnit_Framework_MockObject_MockObject[]
 	 */
-	protected function getUsers(array $userIds) {
+	protected function getUsers(array $userIds): array {
 		$users = [];
 		foreach ($userIds as $uid) {
 			$user = $this->createMock(IUser::class);
@@ -436,10 +437,10 @@ class BackgroundJobTest extends TestCase {
 	}
 
 	/**
-	 * @param $gid
+	 * @param string $gid
 	 * @return \OCP\IGroup|\PHPUnit_Framework_MockObject_MockObject
 	 */
-	protected function getGroup($gid) {
+	protected function getGroup(string $gid) {
 		$group = $this->createMock(IGroup::class);
 		$group->expects($this->any())
 			->method('getGID')

apps/updatenotification/tests/Notification/NotifierTest.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -88,7 +89,7 @@ class NotifierTest extends TestCase {
 		}
 	}
 
-	public function dataUpdateAlreadyInstalledCheck() {
+	public function dataUpdateAlreadyInstalledCheck(): array {
 		return [
 			['1.1.0', '1.0.0', false],
 			['1.1.0', '1.1.0', true],
@@ -103,7 +104,7 @@ class NotifierTest extends TestCase {
 	 * @param string $versionInstalled
 	 * @param bool $exception
 	 */
-	public function testUpdateAlreadyInstalledCheck($versionNotification, $versionInstalled, $exception) {
+	public function testUpdateAlreadyInstalledCheck(string $versionNotification, string $versionInstalled, bool $exception) {
 		$notifier = $this->getNotifier();
 
 		$notification = $this->createMock(INotification::class);
@@ -121,7 +122,7 @@ class NotifierTest extends TestCase {
 		}
 
 		try {
-			$this->invokePrivate($notifier, 'updateAlreadyInstalledCheck', [$notification, $versionInstalled]);
+			self::invokePrivate($notifier, 'updateAlreadyInstalledCheck', [$notification, $versionInstalled]);
 			$this->assertFalse($exception);
 		} catch (\Exception $e) {
 			$this->assertTrue($exception);

apps/updatenotification/tests/ResetTokenBackgroundJobTest.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *

apps/updatenotification/tests/Settings/AdminTest.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *

apps/updatenotification/tests/UpdateCheckerTest.php

@@ -1,4 +1,5 @@
 <?php
+declare(strict_types=1);
 /**
  * @copyright Copyright (c) 2016, ownCloud, Inc.
  *
@@ -24,12 +25,12 @@
 
 namespace OCA\UpdateNotification\Tests;
 
-use OC\Updater;
+use OC\Updater\VersionCheck;
 use OCA\UpdateNotification\UpdateChecker;
 use Test\TestCase;
 
 class UpdateCheckerTest extends TestCase {
-	/** @var Updater */
+	/** @var VersionCheck|\PHPUnit_Framework_MockObject_MockObject */
 	private $updater;
 	/** @var UpdateChecker */
 	private $updateChecker;
@@ -37,8 +38,7 @@ class UpdateCheckerTest extends TestCase {
 	public function setUp() {
 		parent::setUp();
 
-		$this->updater = $this->getMockBuilder('\OC\Updater\VersionCheck')
-			->disableOriginalConstructor()->getMock();
+		$this->updater = $this->createMock(VersionCheck::class);
 		$this->updateChecker = new UpdateChecker($this->updater);
 	}