Merge pull request #1882 from nextcloud/techdebt/44770/notification-e…

…xceptions

techdebt(exceptions): Migrate to new exceptions

docs/notification-workflow.md

@@ -11,7 +11,7 @@ with it, we want to remove the notification again.
   1. Grab a new notification object (`\OCP\Notification\INotification`) from the manager
   (`\OCP\Notification\IManager`):
 ```php
-$manager = \OC::$server->get(\OCP\Notification\IManager::class);
+$manager = \OCP\Server::get(\OCP\Notification\IManager::class);
 $notification = $manager->createNotification();
 ```
 
@@ -51,16 +51,18 @@ $manager->notify($notification);
 
 ### Preparing a notification for display
 
-  1. In `app.php` register your Notifier (`\OCP\Notification\INotifier`) interface to the manager,
+1. In `OCA\MyApp\Application::register(IRegistrationContext $context)` register your Notifier (implementing the `\OCP\Notification\INotifier` interface) to the manager,
   using a `\Closure` returning the Notifier and a `\Closure` returning an array of the id and name:
 ```php
-$manager = \OC::$server->get(\OCP\Notification\IManager::class);
-$manager->registerNotifierService(\OCA\Files_Sharing\Notification\Notifier::class);
+	public function register(IRegistrationContext $context): void {
+		$context->registerNotifierService(\OCA\Files_Sharing\Notification\Notifier::class);
+	}
 ```
 
-  2. The manager will execute the closure and then call the `prepare()` method on your notifier.
-  If the notification is not known by your app, just throw an `\InvalidArgumentException`,
-  but if it is actually from your app, you must set the parsed subject, message and action labels:
+2. The manager will execute the closure and then call the `prepare()` method on your notifier.
+  If the notification is not known by your app, just throw an `OCP\Notification\UnknownNotificationException` (*Added in Nextcloud 30, before throw `\InvalidArgumentException`*),
+  but if it is actually from your app, you must set the parsed subject, message and action labels.
+  If the notification is obsolete by now, e.g. because the share was revoked already, you can throw a `OCP\Notification\AlreadyProcessedException` which will remove the notification also from the devices.
 ```php
 
 class Notifier implements \OCP\Notification\INotifier {
@@ -96,7 +98,7 @@ class Notifier implements \OCP\Notification\INotifier {
 	public function prepare(INotification $notification, string $languageCode): INotification {
 		if ($notification->getApp() !== 'files_sharing') {
 			// Not my app => throw
-			throw new \InvalidArgumentException();
+			throw new \OCP\Notification\UnknownNotificationException();
 		}
 
 		// Read the language from the notification
@@ -150,7 +152,7 @@ class Notifier implements \OCP\Notification\INotifier {
 
 			default:
 				// Unknown subject => Unknown notification => throw
-				throw new \InvalidArgumentException();
+				throw new \OCP\Notification\UnknownNotificationException();
 		}
 	}
 
@@ -186,7 +188,7 @@ call the `markProcessed()` method on the manager with the necessary information
 notification object:
 
 ```php
-$manager = \OC::$server->get(\OCP\Notification\IManager::class);
+$manager = \OCP\Server::get(\OCP\Notification\IManager::class);
 $notification->setApp('files_sharing')
     ->setObject('remote', 1337)
     ->setUser('recipient1');
@@ -198,7 +200,7 @@ will be marked as processed for all users that have it. So the following example
 remove all notifications for the app files_sharing on the object "remote #1337":
 
 ```php
-$manager = \OC::$server->get(\OCP\Notification\IManager::class);
+$manager = \OCP\Server::get(\OCP\Notification\IManager::class);
 $notification->setApp('files_sharing')
     ->setObject('remote', 1337);
 $manager->markProcessed($notification);
@@ -210,7 +212,7 @@ Sometimes you might send multiple notifications in one request.
 In that case it makes sense to defer the sending, so in the end only one connection
 is done to the push server instead of 1 per notification.
 ```php
-$manager = \OC::$server->get(\OCP\Notification\IManager::class);
+$manager = \OCP\Server::get(\OCP\Notification\IManager::class);
 $shouldFlush = $manager->defer();
 
 // Your application code generating notifications …

lib/App.php

@@ -28,18 +28,15 @@
 use OCA\Notifications\Exceptions\NotificationNotFoundException;
 use OCP\Notification\IDeferrableApp;
 use OCP\Notification\INotification;
+use Psr\Log\LoggerInterface;
 use Symfony\Component\Console\Output\OutputInterface;
 
 class App implements IDeferrableApp {
-	/** @var Handler */
-	protected $handler;
-	/** @var Push */
-	protected $push;
-
-	public function __construct(Handler $handler,
-		Push $push) {
-		$this->handler = $handler;
-		$this->push = $push;
+	public function __construct(
+		protected Handler $handler,
+		protected Push $push,
+		protected LoggerInterface $logger,
+	) {
 	}
 
 	public function setOutput(OutputInterface $output): void {
@@ -48,7 +45,6 @@ public function setOutput(OutputInterface $output): void {
 
 	/**
 	 * @param INotification $notification
-	 * @throws \InvalidArgumentException When the notification is not valid
 	 * @since 8.2.0
 	 */
 	public function notify(INotification $notification): void {
@@ -57,7 +53,7 @@ public function notify(INotification $notification): void {
 		try {
 			$this->push->pushToDevice($notificationId, $notification);
 		} catch (NotificationNotFoundException $e) {
-			throw new \InvalidArgumentException('Error while preparing push notification');
+			$this->logger->error('Error while preparing push notification', ['exception' => $e]);
 		}
 	}
 

lib/Notifier/AdminNotifications.php

@@ -32,30 +32,18 @@
 use OCP\IUser;
 use OCP\IUserManager;
 use OCP\L10N\IFactory;
-use OCP\Notification\AlreadyProcessedException;
 use OCP\Notification\IAction;
 use OCP\Notification\INotification;
 use OCP\Notification\INotifier;
+use OCP\Notification\UnknownNotificationException;
 
 class AdminNotifications implements INotifier {
-	/** @var IFactory */
-	protected $l10nFactory;
-
-	/** @var IURLGenerator */
-	protected $urlGenerator;
-	/** @var IUserManager */
-	protected $userManager;
-	/** @var IRootFolder */
-	protected $rootFolder;
-
-	public function __construct(IFactory $l10nFactory,
-		IURLGenerator $urlGenerator,
-		IUserManager $userManager,
-		IRootFolder $rootFolder) {
-		$this->l10nFactory = $l10nFactory;
-		$this->urlGenerator = $urlGenerator;
-		$this->userManager = $userManager;
-		$this->rootFolder = $rootFolder;
+	public function __construct(
+		protected IFactory $l10nFactory,
+		protected IURLGenerator $urlGenerator,
+		protected IUserManager $userManager,
+		protected IRootFolder $rootFolder,
+	) {
 	}
 
 	/**
@@ -82,12 +70,11 @@ public function getName(): string {
 	 * @param INotification $notification
 	 * @param string $languageCode The code of the language that should be used to prepare the notification
 	 * @return INotification
-	 * @throws \InvalidArgumentException When the notification was not prepared by a notifier
-	 * @throws AlreadyProcessedException When the notification is not needed anymore and should be deleted
+	 * @throws UnknownNotificationException When the notification was not prepared by a notifier
 	 */
 	public function prepare(INotification $notification, string $languageCode): INotification {
 		if ($notification->getApp() !== 'admin_notifications' && $notification->getApp() !== 'admin_notification_talk') {
-			throw new \InvalidArgumentException('Unknown app');
+			throw new UnknownNotificationException('app');
 		}
 
 		switch ($notification->getSubject()) {
@@ -216,7 +203,7 @@ public function prepare(INotification $notification, string $languageCode): INot
 				return $notification;
 
 			default:
-				throw new \InvalidArgumentException('Unknown subject');
+				throw new UnknownNotificationException('subject');
 		}
 	}
 }

tests/Unit/AppTest.php

@@ -26,32 +26,32 @@
 use OCA\Notifications\Handler;
 use OCA\Notifications\Push;
 use OCP\Notification\INotification;
+use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
 
 class AppTest extends TestCase {
-	/** @var Handler|\PHPUnit_Framework_MockObject_MockObject */
-	protected $handler;
-	/** @var Push|\PHPUnit_Framework_MockObject_MockObject */
-	protected $push;
-	/** @var INotification|\PHPUnit_Framework_MockObject_MockObject */
-	protected $notification;
-
-	/** @var App */
-	protected $app;
+	protected Handler|MockObject $handler;
+	protected Push|MockObject $push;
+	protected INotification|MockObject $notification;
+	protected LoggerInterface|MockObject $logger;
+	protected App $app;
 
 	protected function setUp(): void {
 		parent::setUp();
 
 		$this->handler = $this->createMock(Handler::class);
 		$this->push = $this->createMock(Push::class);
 		$this->notification = $this->createMock(INotification::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
 
 		$this->app = new App(
 			$this->handler,
-			$this->push
+			$this->push,
+			$this->logger,
 		);
 	}
 
-	public function dataNotify() {
+	public static function dataNotify(): array {
 		return [
 			[23],
 			[42],
@@ -63,7 +63,7 @@ public function dataNotify() {
 	 *
 	 * @param int $id
 	 */
-	public function testNotify($id) {
+	public function testNotify(int $id): void {
 		$this->handler->expects($this->once())
 			->method('add')
 			->with($this->notification)
@@ -75,7 +75,7 @@ public function testNotify($id) {
 		$this->app->notify($this->notification);
 	}
 
-	public function dataGetCount() {
+	public static function dataGetCount(): array {
 		return [
 			[23],
 			[42],
@@ -87,7 +87,7 @@ public function dataGetCount() {
 	 *
 	 * @param int $count
 	 */
-	public function testGetCount($count) {
+	public function testGetCount(int $count): void {
 		$this->handler->expects($this->once())
 			->method('count')
 			->with($this->notification)
@@ -96,7 +96,7 @@ public function testGetCount($count) {
 		$this->assertSame($count, $this->app->getCount($this->notification));
 	}
 
-	public function testMarkProcessed() {
+	public function testMarkProcessed(): void {
 		$this->handler->expects($this->once())
 			->method('delete')
 			->with($this->notification);

tests/Unit/Notifier/NotifierTest.php

@@ -30,22 +30,16 @@
 use OCP\IUserManager;
 use OCP\L10N\IFactory;
 use OCP\Notification\INotification;
+use OCP\Notification\UnknownNotificationException;
 use PHPUnit\Framework\MockObject\MockObject;
 
 class NotifierTest extends \Test\TestCase {
-	/** @var AdminNotifications */
-	protected $notifier;
-
-	/** @var IFactory|MockObject */
-	protected $factory;
-	/** @var IURLGenerator|MockObject */
-	protected $urlGenerator;
-	/** @var IUserManager|MockObject */
-	protected $userManager;
-	/** @var IRootFolder|MockObject */
-	protected $rootFolder;
-	/** @var IL10N|MockObject */
-	protected $l;
+	protected IFactory|MockObject $factory;
+	protected IURLGenerator|MockObject $urlGenerator;
+	protected IUserManager|MockObject $userManager;
+	protected IRootFolder|MockObject $rootFolder;
+	protected IL10N|MockObject $l;
+	protected AdminNotifications $notifier;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -72,8 +66,8 @@ protected function setUp(): void {
 		);
 	}
 
-	public function testPrepareWrongApp() {
-		/** @var INotification|\PHPUnit_Framework_MockObject_MockObject $notification */
+	public function testPrepareWrongApp(): void {
+		/** @var INotification|MockObject $notification */
 		$notification = $this->createMock(INotification::class);
 
 		$notification->expects($this->exactly(2))
@@ -82,13 +76,13 @@ public function testPrepareWrongApp() {
 		$notification->expects($this->never())
 			->method('getSubject');
 
-		$this->expectException(\InvalidArgumentException::class);
-		$this->expectExceptionMessage('Unknown app');
+		$this->expectException(UnknownNotificationException::class);
+		$this->expectExceptionMessage('app');
 		$this->notifier->prepare($notification, 'en');
 	}
 
-	public function testPrepareWrongSubject() {
-		/** @var INotification|\PHPUnit_Framework_MockObject_MockObject $notification */
+	public function testPrepareWrongSubject(): void {
+		/** @var INotification|MockObject $notification */
 		$notification = $this->createMock(INotification::class);
 
 		$notification->expects($this->once())
@@ -98,27 +92,22 @@ public function testPrepareWrongSubject() {
 			->method('getSubject')
 			->willReturn('wrong subject');
 
-		$this->expectException(\InvalidArgumentException::class);
-		$this->expectExceptionMessage('Unknown subject');
+		$this->expectException(UnknownNotificationException::class);
+		$this->expectExceptionMessage('subject');
 		$this->notifier->prepare($notification, 'en');
 	}
 
-	public function dataPrepare() {
+	public static function dataPrepare(): array {
 		return [
 			['ocs', ['subject'], ['message'], true],
 		];
 	}
 
 	/**
 	 * @dataProvider dataPrepare
-	 *
-	 * @param string $subject
-	 * @param array $subjectParams
-	 * @param array $messageParams
-	 * @param bool $setMessage
 	 */
-	public function testPrepare($subject, $subjectParams, $messageParams, $setMessage) {
-		/** @var INotification|\PHPUnit_Framework_MockObject_MockObject $notification */
+	public function testPrepare(string $subject, array $subjectParams, array $messageParams, bool $setMessage): void {
+		/** @var INotification|MockObject $notification */
 		$notification = $this->createMock(INotification::class);
 
 		$notification->expects($this->once())