refactor: CommandStatusResult

Signed-off-by: Sebastian Krupinski <krupinski01@gmail.com>

lib/Client/ClientInterface.php

@@ -17,6 +17,10 @@ interface ClientInterface
     public function capabilities(): array;
 
     /**
+     * Unsuccessful command completion throws ImapException. For streamed
+     * results, completion is checked as the returned generator is consumed.
+     *
+     * @throws ImapException
      * @template TResult
      * @param CommandInterface<TResult> $command
      * @return TResult

lib/Client/Protocol/Command/CreateCommand.php

@@ -5,7 +5,7 @@ declare(strict_types=1);
 namespace KTXM\ProviderImap\Client\Protocol\Command;
 
 use KTXM\ProviderImap\Client\Protocol\StringEncoder;
-use KTXM\ProviderImap\Client\Result\CommandStatusResult;
+use KTXM\ProviderImap\Client\Result\CommandCompletion;
 use KTXM\ProviderImap\Client\ImapException;
 use KTXM\ProviderImap\Client\Protocol\RequestFrame;
 use KTXM\ProviderImap\Client\Protocol\Response\TaggedResponse;
@@ -14,7 +14,7 @@ use KTXM\ProviderImap\Client\Protocol\SessionContext;
 use KTXM\ProviderImap\Client\Protocol\SessionState;
 
 /**
- * @implements CommandInterface<CommandStatusResult>
+ * @implements CommandInterface<CommandCompletion>
  */
 final class CreateCommand implements CommandInterface
 {
@@ -42,7 +42,7 @@ final class CreateCommand implements CommandInterface
         return new RequestFrame(sprintf('CREATE %s', StringEncoder::quote($this->mailbox)));
     }
 
-    public function handle(ResponseStream $responses, SessionContext $context): CommandStatusResult
+    public function handle(ResponseStream $responses, SessionContext $context): CommandCompletion
     {
         unset($context);
 
@@ -52,7 +52,7 @@ final class CreateCommand implements CommandInterface
                     throw new ImapException('CREATE failed: ' . $response->text());
                 }
 
-                return new CommandStatusResult($response->status(), $response->text());
+                return new CommandCompletion($response->status(), $response->text());
             }
         }
 

lib/Client/Protocol/Command/DeleteCommand.php

@@ -5,7 +5,7 @@ declare(strict_types=1);
 namespace KTXM\ProviderImap\Client\Protocol\Command;
 
 use KTXM\ProviderImap\Client\Protocol\StringEncoder;
-use KTXM\ProviderImap\Client\Result\CommandStatusResult;
+use KTXM\ProviderImap\Client\Result\CommandCompletion;
 use KTXM\ProviderImap\Client\ImapException;
 use KTXM\ProviderImap\Client\Protocol\RequestFrame;
 use KTXM\ProviderImap\Client\Protocol\Response\TaggedResponse;
@@ -14,7 +14,7 @@ use KTXM\ProviderImap\Client\Protocol\SessionContext;
 use KTXM\ProviderImap\Client\Protocol\SessionState;
 
 /**
- * @implements CommandInterface<CommandStatusResult>
+ * @implements CommandInterface<CommandCompletion>
  */
 final class DeleteCommand implements CommandInterface
 {
@@ -42,7 +42,7 @@ final class DeleteCommand implements CommandInterface
         return new RequestFrame(sprintf('DELETE %s', StringEncoder::quote($this->mailbox)));
     }
 
-    public function handle(ResponseStream $responses, SessionContext $context): CommandStatusResult
+    public function handle(ResponseStream $responses, SessionContext $context): CommandCompletion
     {
         if ($context->selectedMailbox() === $this->mailbox) {
             $context->setSelectedMailbox(null);
@@ -55,7 +55,7 @@ final class DeleteCommand implements CommandInterface
                     throw new ImapException('DELETE failed: ' . $response->text());
                 }
 
-                return new CommandStatusResult($response->status(), $response->text());
+                return new CommandCompletion($response->status(), $response->text());
             }
         }
 

lib/Client/Protocol/Command/LoginCommand.php

@@ -5,7 +5,7 @@ declare(strict_types=1);
 namespace KTXM\ProviderImap\Client\Protocol\Command;
 
 use KTXM\ProviderImap\Client\Protocol\StringEncoder;
-use KTXM\ProviderImap\Client\Result\CommandStatusResult;
+use KTXM\ProviderImap\Client\Result\CommandCompletion;
 use KTXM\ProviderImap\Client\ImapException;
 use KTXM\ProviderImap\Client\Protocol\RequestFrame;
 use KTXM\ProviderImap\Client\Protocol\Response\TaggedResponse;
@@ -14,7 +14,7 @@ use KTXM\ProviderImap\Client\Protocol\SessionContext;
 use KTXM\ProviderImap\Client\Protocol\SessionState;
 
 /**
- * @implements CommandInterface<CommandStatusResult>
+ * @implements CommandInterface<CommandCompletion>
  */
 final class LoginCommand implements CommandInterface
 {
@@ -44,7 +44,7 @@ final class LoginCommand implements CommandInterface
         ));
     }
 
-    public function handle(ResponseStream $responses, SessionContext $context): CommandStatusResult
+    public function handle(ResponseStream $responses, SessionContext $context): CommandCompletion
     {
         foreach ($responses as $response) {
             if ($response instanceof TaggedResponse) {
@@ -54,7 +54,7 @@ final class LoginCommand implements CommandInterface
 
                 $context->setState(SessionState::Authenticated);
 
-                return new CommandStatusResult($response->status(), $response->text());
+                return new CommandCompletion($response->status(), $response->text());
             }
         }
 

lib/Client/Protocol/Command/LogoutCommand.php

@@ -4,7 +4,7 @@ declare(strict_types=1);
 
 namespace KTXM\ProviderImap\Client\Protocol\Command;
 
-use KTXM\ProviderImap\Client\Result\CommandStatusResult;
+use KTXM\ProviderImap\Client\Result\CommandCompletion;
 use KTXM\ProviderImap\Client\ImapException;
 use KTXM\ProviderImap\Client\Protocol\RequestFrame;
 use KTXM\ProviderImap\Client\Protocol\Response\TaggedResponse;
@@ -13,7 +13,7 @@ use KTXM\ProviderImap\Client\Protocol\SessionContext;
 use KTXM\ProviderImap\Client\Protocol\SessionState;
 
 /**
- * @implements CommandInterface<CommandStatusResult>
+ * @implements CommandInterface<CommandCompletion>
  */
 final class LogoutCommand implements CommandInterface
 {
@@ -38,7 +38,7 @@ final class LogoutCommand implements CommandInterface
         return new RequestFrame('LOGOUT');
     }
 
-    public function handle(ResponseStream $responses, SessionContext $context): CommandStatusResult
+    public function handle(ResponseStream $responses, SessionContext $context): CommandCompletion
     {
         foreach ($responses as $response) {
             if ($response instanceof TaggedResponse) {
@@ -50,7 +50,7 @@ final class LogoutCommand implements CommandInterface
                 $context->setState(SessionState::Logout);
                 $context->connection()->disconnect();
 
-                return new CommandStatusResult($response->status(), $response->text());
+                return new CommandCompletion($response->status(), $response->text());
             }
         }
 

lib/Client/Protocol/Command/NoopCommand.php

@@ -4,7 +4,7 @@ declare(strict_types=1);
 
 namespace KTXM\ProviderImap\Client\Protocol\Command;
 
-use KTXM\ProviderImap\Client\Result\CommandStatusResult;
+use KTXM\ProviderImap\Client\Result\CommandCompletion;
 use KTXM\ProviderImap\Client\ImapException;
 use KTXM\ProviderImap\Client\Protocol\RequestFrame;
 use KTXM\ProviderImap\Client\Protocol\Response\TaggedResponse;
@@ -13,7 +13,7 @@ use KTXM\ProviderImap\Client\Protocol\SessionContext;
 use KTXM\ProviderImap\Client\Protocol\SessionState;
 
 /**
- * @implements CommandInterface<CommandStatusResult>
+ * @implements CommandInterface<CommandCompletion>
  */
 final class NoopCommand implements CommandInterface
 {
@@ -38,7 +38,7 @@ final class NoopCommand implements CommandInterface
         return new RequestFrame('NOOP');
     }
 
-    public function handle(ResponseStream $responses, SessionContext $context): CommandStatusResult
+    public function handle(ResponseStream $responses, SessionContext $context): CommandCompletion
     {
         unset($context);
 
@@ -48,7 +48,7 @@ final class NoopCommand implements CommandInterface
                     throw new ImapException('NOOP failed: ' . $response->text());
                 }
 
-                return new CommandStatusResult($response->status(), $response->text());
+                return new CommandCompletion($response->status(), $response->text());
             }
         }
 

lib/Client/Protocol/Command/RenameCommand.php

@@ -5,7 +5,7 @@ declare(strict_types=1);
 namespace KTXM\ProviderImap\Client\Protocol\Command;
 
 use KTXM\ProviderImap\Client\Protocol\StringEncoder;
-use KTXM\ProviderImap\Client\Result\CommandStatusResult;
+use KTXM\ProviderImap\Client\Result\CommandCompletion;
 use KTXM\ProviderImap\Client\ImapException;
 use KTXM\ProviderImap\Client\Protocol\RequestFrame;
 use KTXM\ProviderImap\Client\Protocol\Response\TaggedResponse;
@@ -14,7 +14,7 @@ use KTXM\ProviderImap\Client\Protocol\SessionContext;
 use KTXM\ProviderImap\Client\Protocol\SessionState;
 
 /**
- * @implements CommandInterface<CommandStatusResult>
+ * @implements CommandInterface<CommandCompletion>
  */
 final class RenameCommand implements CommandInterface
 {
@@ -47,7 +47,7 @@ final class RenameCommand implements CommandInterface
         ));
     }
 
-    public function handle(ResponseStream $responses, SessionContext $context): CommandStatusResult
+    public function handle(ResponseStream $responses, SessionContext $context): CommandCompletion
     {
         foreach ($responses as $response) {
             if ($response instanceof TaggedResponse) {
@@ -59,7 +59,7 @@ final class RenameCommand implements CommandInterface
                     $context->setSelectedMailbox($this->toMailbox);
                 }
 
-                return new CommandStatusResult($response->status(), $response->text());
+                return new CommandCompletion($response->status(), $response->text());
             }
         }
 

lib/Client/Protocol/Command/StartTlsCommand.php

@@ -4,7 +4,7 @@ declare(strict_types=1);
 
 namespace KTXM\ProviderImap\Client\Protocol\Command;
 
-use KTXM\ProviderImap\Client\Result\CommandStatusResult;
+use KTXM\ProviderImap\Client\Result\CommandCompletion;
 use KTXM\ProviderImap\Client\ImapException;
 use KTXM\ProviderImap\Client\Protocol\RequestFrame;
 use KTXM\ProviderImap\Client\Protocol\Response\TaggedResponse;
@@ -13,7 +13,7 @@ use KTXM\ProviderImap\Client\Protocol\SessionContext;
 use KTXM\ProviderImap\Client\Protocol\SessionState;
 
 /**
- * @implements CommandInterface<CommandStatusResult>
+ * @implements CommandInterface<CommandCompletion>
  */
 final class StartTlsCommand implements CommandInterface
 {
@@ -34,7 +34,7 @@ final class StartTlsCommand implements CommandInterface
         return new RequestFrame('STARTTLS');
     }
 
-    public function handle(ResponseStream $responses, SessionContext $context): CommandStatusResult
+    public function handle(ResponseStream $responses, SessionContext $context): CommandCompletion
     {
         foreach ($responses as $response) {
             if ($response instanceof TaggedResponse) {
@@ -45,7 +45,7 @@ final class StartTlsCommand implements CommandInterface
                 $context->connection()->upgradeToTls();
                 $context->replaceCapabilities();
 
-                return new CommandStatusResult($response->status(), $response->text());
+                return new CommandCompletion($response->status(), $response->text());
             }
         }
 

lib/Client/Protocol/Command/StoreCommand.php

@@ -4,7 +4,7 @@ declare(strict_types=1);
 
 namespace KTXM\ProviderImap\Client\Protocol\Command;
 
-use KTXM\ProviderImap\Client\Result\CommandStatusResult;
+use KTXM\ProviderImap\Client\Result\CommandCompletion;
 use KTXM\ProviderImap\Client\Protocol\Command\Argument\MessageTarget;
 use KTXM\ProviderImap\Client\Protocol\IdentifierMode;
 use KTXM\ProviderImap\Client\ImapException;
@@ -16,7 +16,7 @@ use KTXM\ProviderImap\Client\Protocol\SessionContext;
 use KTXM\ProviderImap\Client\Protocol\SessionState;
 
 /**
- * @implements CommandInterface<CommandStatusResult>
+ * @implements CommandInterface<CommandCompletion>
  */
 final class StoreCommand implements CommandInterface
 {
@@ -82,7 +82,7 @@ final class StoreCommand implements CommandInterface
         ));
     }
 
-    public function handle(ResponseStream $responses, SessionContext $context): CommandStatusResult
+    public function handle(ResponseStream $responses, SessionContext $context): CommandCompletion
     {
         if ($context->selectedMailbox() === null) {
             throw new ImapException('STORE requires a selected mailbox.');
@@ -94,7 +94,7 @@ final class StoreCommand implements CommandInterface
                     throw new ImapException('STORE failed: ' . $response->text());
                 }
 
-                return new CommandStatusResult($response->status(), $response->text());
+                return new CommandCompletion($response->status(), $response->text());
             }
         }
 

lib/Client/Result/CommandCompletion.php

@@ -4,7 +4,10 @@ declare(strict_types=1);
 
 namespace KTXM\ProviderImap\Client\Result;
 
-final class CommandStatusResult
+/**
+ * Successful command output; unsuccessful completion is reported by exception.
+ */
+final class CommandCompletion
 {
     public function __construct(
         private readonly string $status,
@@ -20,9 +23,4 @@ final class CommandStatusResult
     {
         return $this->text;
     }
-
-    public function isOk(): bool
-    {
-        return $this->status === 'OK';
-    }
 }

lib/Client/Result/MessageTransferResult.php

@@ -4,6 +4,9 @@ declare(strict_types=1);
 
 namespace KTXM\ProviderImap\Client\Result;
 
+/**
+ * Successful command output; unsuccessful completion is reported by exception.
+ */
 final class MessageTransferResult
 {
     /**
@@ -33,11 +36,6 @@ final class MessageTransferResult
         return $this->text;
     }
 
-    public function isOk(): bool
-    {
-        return $this->status === 'OK';
-    }
-
     /**
      * @return list<array{source:string, name:string, arguments:list<string>, text:string}>
      */

lib/Service/Remote/RemoteMailService.php

@@ -220,11 +220,7 @@ class RemoteMailService
      */
     public function collectionCreate(string $name): Mailbox
     {
-        $result = $this->imapClient()->perform(new CreateCommand($name));
-
-        if (!$result->isOk()) {
-            throw new ImapException('Failed to create mailbox: ' . $name);
-        }
+        $this->imapClient()->perform(new CreateCommand($name));
 
         // Attempt to refetch the new mailbox from the server
         $mailbox = $this->collectionFetch($name);
@@ -240,11 +236,7 @@ class RemoteMailService
      */
     public function collectionRename(string $oldName, string $newName): Mailbox
     {
-        $result = $this->imapClient()->perform(new RenameCommand($oldName, $newName));
-
-        if (!$result->isOk()) {
-            throw new ImapException('Failed to rename mailbox: ' . $oldName . ' to ' . $newName);
-        }
+        $this->imapClient()->perform(new RenameCommand($oldName, $newName));
 
         $mailbox = $this->collectionFetch($newName);
 
@@ -260,11 +252,7 @@ class RemoteMailService
      */
     public function collectionDestroy(string $name): bool
     {
-        $result = $this->imapClient()->perform(new DeleteCommand($name));
-
-        if (!$result->isOk()) {
-            throw new ImapException('Failed to delete mailbox: ' . $name);
-        }
+        $this->imapClient()->perform(new DeleteCommand($name));
 
         return true;
     }
@@ -617,7 +605,6 @@ class RemoteMailService
                 MessageTarget::uid(SequenceSet::items(...array_values($uids))),
                 $targetCollection,
             ));
-            if ($response->isOk()) {
             $this->imapClient()->perform(new StoreCommand(
                 MessageTarget::uid(SequenceSet::items(...array_values($uids))),
                 ['\\Deleted'],
@@ -627,11 +614,6 @@ class RemoteMailService
                 MessageTarget::uid(SequenceSet::items(...array_values($uids))),
             ));
         }
-        }
-
-        if (!$response->isOk()) {
-            throw new ImapException('Failed to move messages: ' . implode(', ', $response->responseCodes()));
-        }
 
         // construct operation result as a map of source UID to boolean or destination UID, depending on server support
         $map = $response->copyUidMap();
@@ -660,10 +642,6 @@ class RemoteMailService
             $targetCollection,
         ));
 
-        if (!$response->isOk()) {
-            throw new ImapException('Failed to copy messages: ' . implode(', ', $response->responseCodes()));
-        }
-
         // construct operation result as a map of source UID to boolean or destination UID, depending on server support
         $map = $response->copyUidMap();
         if ($map === []) {

tests/php/Unit/CommandCompletionTest.php

@@ -0,0 +1,109 @@
+<?php
+
+declare(strict_types=1);
+
+namespace KTXT\ProviderImap\Tests\Unit;
+
+use KTXM\ProviderImap\Client\{Client, ConnectionConfig, ImapException};
+use KTXM\ProviderImap\Client\Protocol\Command\CreateCommand;
+use KTXM\ProviderImap\Client\Result\CommandCompletion;
+use KTXM\ProviderImap\Client\Transport\{ConnectionInterface, ConnectionFactoryInterface};
+use KTXM\ProviderImap\Providers\Service;
+use KTXM\ProviderImap\Service\Remote\RemoteMailService;
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\TestCase;
+
+final class CommandCompletionTest extends TestCase
+{
+    private array $commands = [];
+
+    private function client(?string $failure = null, bool $move = false, string $failureStatus = 'NO'): Client
+    {
+        $lines = ["* PREAUTH Ready\r\n"];
+        $connection = $this->createStub(ConnectionInterface::class);
+        $connection->method('readLine')->willReturnCallback(static function () use (&$lines): string {
+            return array_shift($lines) ?? throw new \RuntimeException('Unexpected response read');
+        });
+        $connection->method('write')->willReturnCallback(function (string $wire) use (&$lines, $failure, $move, $failureStatus): void {
+            [$tag, $command] = explode(' ', trim($wire), 2);
+            $this->commands[] = $command;
+            $operation = str_starts_with($command, 'UID ') ? explode(' ', $command)[1] : explode(' ', $command)[0];
+            if ($operation === 'CAPABILITY') {
+                $lines[] = '* CAPABILITY IMAP4rev1 UIDPLUS' . ($move ? ' MOVE' : '') . "\r\n";
+            }
+            $status = $operation === $failure ? $failureStatus : 'OK';
+            $text = $status === 'OK' ? 'Completed' : 'Denied';
+            if ($status === 'OK' && in_array($operation, ['COPY', 'MOVE'], true)) {
+                $text = '[COPYUID 7 42 142] Completed';
+            }
+            $lines[] = "$tag $status $text\r\n";
+        });
+        $factory = $this->createStub(ConnectionFactoryInterface::class);
+        $factory->method('create')->willReturn($connection);
+        $client = new Client($factory);
+        $client->connect(new ConnectionConfig('localhost'));
+        $this->commands = [];
+        return $client;
+    }
+
+    private function service(Client $client): RemoteMailService
+    {
+        return new class($this->createStub(Service::class), $client) extends RemoteMailService {
+            public function __construct(Service $service, private readonly Client $client)
+            {
+                parent::__construct($service);
+            }
+
+            public function imapClient(): Client
+            {
+                return $this->client;
+            }
+        };
+    }
+
+    public function testSuccessfulCommandReturnsCompletionText(): void
+    {
+        $result = $this->client()->perform(new CreateCommand('Archive'));
+        $this->assertInstanceOf(CommandCompletion::class, $result);
+        $this->assertSame('OK', $result->status());
+        $this->assertSame('Completed', $result->text());
+    }
+
+    public static function failures(): iterable
+    {
+        foreach (['NO', 'BAD'] as $status) {
+            yield "create $status" => ['collectionCreate', ['Archive'], 'CREATE', false, ['CREATE "Archive"'], $status];
+            yield "rename $status" => ['collectionRename', ['INBOX', 'Archive'], 'RENAME', false, ['RENAME "INBOX" "Archive"'], $status];
+            yield "delete $status" => ['collectionDestroy', ['Archive'], 'DELETE', false, ['DELETE "Archive"'], $status];
+            yield "copy $status" => ['entityCopy', ['Archive', 'INBOX', 42], 'COPY', false, ['SELECT "INBOX"', 'UID COPY 42 "Archive"'], $status];
+            yield "move $status" => ['entityMove', ['Archive', 'INBOX', 42], 'MOVE', true, ['SELECT "INBOX"', 'UID MOVE 42 "Archive"'], $status];
+            yield "fallback copy $status" => ['entityMove', ['Archive', 'INBOX', 42], 'COPY', false, ['SELECT "INBOX"', 'UID COPY 42 "Archive"'], $status];
+            yield "fallback store $status" => ['entityMove', ['Archive', 'INBOX', 42], 'STORE', false, ['SELECT "INBOX"', 'UID COPY 42 "Archive"', 'UID STORE 42 +FLAGS.SILENT (\\Deleted)'], $status];
+        }
+    }
+
+    #[DataProvider('failures')]
+    public function testFailuresPropagateWithoutExecutingLaterCommands(string $method, array $arguments, string $failure, bool $move, array $commands, string $status): void
+    {
+        $service = $this->service($this->client($failure, $move, $status));
+        try {
+            $service->$method(...$arguments);
+            $this->fail('Expected command failure');
+        } catch (ImapException $exception) {
+            $this->assertSame($failure . ' failed: Denied', $exception->getMessage());
+        }
+        $this->assertSame($commands, $this->commands);
+    }
+
+    public function testMoveFallbackDeletesOnlyAfterCopySucceeds(): void
+    {
+        $result = $this->service($this->client())->entityMove('Archive', 'INBOX', 42);
+        $this->assertSame([42 => '142'], $result);
+        $this->assertSame([
+            'SELECT "INBOX"',
+            'UID COPY 42 "Archive"',
+            'UID STORE 42 +FLAGS.SILENT (\\Deleted)',
+            'UID EXPUNGE 42',
+        ], $this->commands);
+    }
+}