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.

Sign up for GitHub

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

Implement bulk-move #306

Merged
merged 8 commits into from
Feb 28, 2018
+134 −19
Merged

Implement bulk-move #306

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4735d31
Implement bulk-move
particleflux Feb 17, 2018
abb0e5c
Throw an exception on error instead of returning success-flag
particleflux Feb 25, 2018
f19d9c7
Refactor bulk message id preparation into a separate method
particleflux Feb 26, 2018
2b8817b
Implement bulk copy
particleflux Feb 26, 2018
d2541fe
Fix doc blocks
particleflux Feb 27, 2018
81624fc
Throw MessageCopyException instead of MessageMoveException on failed …
particleflux Feb 27, 2018
389dea6
Fix wrong annotated exception
particleflux Feb 27, 2018
e333801
Static analysis help
Slamdunk Feb 28, 2018
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
71 changes: 57 additions & 14 deletions src/Mailbox.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

use DateTimeInterface;
use Ddeboer\Imap\Exception\InvalidSearchCriteriaException;
use Ddeboer\Imap\Exception\MessageMoveException;
use Ddeboer\Imap\Search\ConditionInterface;
use Ddeboer\Imap\Search\LogicalOperator\All;

Expand Down Expand Up @@ -118,35 +119,27 @@ public function getStatus(int $flags = null): \stdClass
/**
* Bulk Set Flag for Messages.
*
* @param string $flag \Seen, \Answered, \Flagged, \Deleted, and \Draft
* @param array|string $numbers Message numbers
* @param string $flag \Seen, \Answered, \Flagged, \Deleted, and \Draft
* @param array|MessageIteratorInterface|string $numbers Message numbers
*
* @return bool
*/
public function setFlag(string $flag, $numbers): bool
{
if (\is_array($numbers)) {
$numbers = \implode(',', $numbers);
}

return \imap_setflag_full($this->resource->getStream(), (string) $numbers, $flag, \ST_UID);
return \imap_setflag_full($this->resource->getStream(), $this->prepareMessageIds($numbers), $flag, \ST_UID);
}

/**
* Bulk Clear Flag for Messages.
*
* @param string $flag \Seen, \Answered, \Flagged, \Deleted, and \Draft
* @param array|string $numbers Message numbers
* @param string $flag \Seen, \Answered, \Flagged, \Deleted, and \Draft
* @param array|MessageIteratorInterface|string $numbers Message numbers
*
* @return bool
*/
public function clearFlag(string $flag, $numbers): bool
{
if (\is_array($numbers)) {
$numbers = \implode(',', $numbers);
}

return \imap_clearflag_full($this->resource->getStream(), (string) $numbers, $flag, \ST_UID);
return \imap_clearflag_full($this->resource->getStream(), $this->prepareMessageIds($numbers), $flag, \ST_UID);
}

/**
Expand Down Expand Up @@ -247,4 +240,54 @@ public function getThread(): array

return false !== $tree ? $tree : [];
}

/**
* Bulk move messages.
*
* @param array|MessageIteratorInterface|string $numbers Message numbers
* @param MailboxInterface $mailbox Destination Mailbox to move the messages to
*
* @throws \Ddeboer\Imap\Exception\MessageMoveException
*/
public function move($numbers, MailboxInterface $mailbox)
{
if (!\imap_mail_move($this->resource->getStream(), $this->prepareMessageIds($numbers), $mailbox->getEncodedName(), \CP_UID)) {
throw new MessageMoveException(\sprintf('Messages cannot be moved to "%s"', $mailbox->getName()));
}
}

/**
* Bulk copy messages.
*
* @param array|MessageIteratorInterface|string $numbers Message numbers
* @param MailboxInterface $mailbox Destination Mailbox to copy the messages to
*
* @throws \Ddeboer\Imap\Exception\MessageMoveException
*/
public function copy($numbers, MailboxInterface $mailbox)
{
if (!\imap_mail_copy($this->resource->getStream(), $this->prepareMessageIds($numbers), $mailbox->getEncodedName(), \CP_UID)) {
throw new MessageMoveException(\sprintf('Messages cannot be copied to "%s"', $mailbox->getName()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

MessageCopyException

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed too.

}
}

/**
* Prepare message ids for the use with bulk functions.
*
* @param array|MessageIteratorInterface|string $messageIds Message numbers
Copy link
Collaborator

Choose a reason for hiding this comment

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

getArrayCopy isn't a method of MessageIteratorInterface but only of MessageIterator: please change all the doc hints to the latter class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right. fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or not. This is now breaking the build since the static analyser complains. I could fix this by typehinting the returned variable in the test - but is that such a good idea?

diff --git a/tests/MailboxTest.php b/tests/MailboxTest.php
index 89cd7c5..84a58da 100644
--- a/tests/MailboxTest.php
+++ b/tests/MailboxTest.php
@@ -10,6 +10,7 @@ use Ddeboer\Imap�xception\MessageDoesNotExistException;
 use Ddeboer\Imap�xception\MessageMoveException;
 use Ddeboer\Imap�xception\ReopenMailboxException;
 use Ddeboer\Imap\MailboxInterface;
+use Ddeboer\Imap\MessageIterator;
 
 /**
  * @covers \Ddeboer\Imap�xception\AbstractException
@@ -234,6 +235,7 @@ final class MailboxTest extends AbstractTest
         $this->assertSame(0, $this->mailbox->count());
 
         // move back by iterator
+        /** @var MessageIterator $messages */
         $messages = $anotherMailbox->getMessages();
         $anotherMailbox->move($messages, $this->mailbox);
         $this->getConnection()->expunge();

Copy link
Collaborator

Choose a reason for hiding this comment

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

but is that such a good idea?

The bad thing is getArrayCopy isn't present in any SPL interface, and this complicates everything a lot.
In a future release we could add it to MessageIteratorInterface but at the time being your solution is enough.

*
* @return string
*/
private function prepareMessageIds($messageIds): string
{
if ($messageIds instanceof MessageIterator) {
$messageIds = $messageIds->getArrayCopy();
}

if (\is_array($messageIds)) {
$messageIds = \implode(',', $messageIds);
}

return (string) $messageIds;
}
}
28 changes: 24 additions & 4 deletions src/MailboxInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,8 @@ public function getStatus(int $flags = null): \stdClass;
/**
* Bulk Set Flag for Messages.
*
* @param string $flag \Seen, \Answered, \Flagged, \Deleted, and \Draft
* @param array|string $numbers Message numbers
* @param string $flag \Seen, \Answered, \Flagged, \Deleted, and \Draft
* @param array|MessageIteratorInterface|string $numbers Message numbers
*
* @return bool
*/
Expand All @@ -69,8 +69,8 @@ public function setFlag(string $flag, $numbers): bool;
/**
* Bulk Clear Flag for Messages.
*
* @param string $flag \Seen, \Answered, \Flagged, \Deleted, and \Draft
* @param array|string $numbers Message numbers
* @param string $flag \Seen, \Answered, \Flagged, \Deleted, and \Draft
* @param array|MessageIteratorInterface|string $numbers Message numbers
*
* @return bool
*/
Expand Down Expand Up @@ -118,4 +118,24 @@ public function addMessage(string $message, string $options = null, DateTimeInte
* @return array
*/
public function getThread(): array;

/**
* Bulk move messages.
*
* @param array|MessageIteratorInterface|string $numbers Message numbers
* @param MailboxInterface $mailbox Destination Mailbox to move the messages to
*
* @throws \Ddeboer\Imap\Exception\MessageMoveException
*/
public function move($numbers, self $mailbox);

/**
* Bulk copy messages.
*
* @param array|MessageIteratorInterface|string $numbers Message numbers
* @param MailboxInterface $mailbox Destination Mailbox to copy the messages to
*
* @throws \Ddeboer\Imap\Exception\MessageMoveException
*/
public function copy($numbers, self $mailbox);
}
50 changes: 49 additions & 1 deletion tests/MailboxTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@

use DateTimeImmutable;
use Ddeboer\Imap\Exception\MessageDoesNotExistException;
use Ddeboer\Imap\Exception\MessageMoveException;
use Ddeboer\Imap\Exception\ReopenMailboxException;
use Ddeboer\Imap\Mailbox;
use Ddeboer\Imap\MailboxInterface;

/**
* @covers \Ddeboer\Imap\Exception\AbstractException
Expand All @@ -16,6 +17,7 @@
*/
final class MailboxTest extends AbstractTest
{
/** @var MailboxInterface */
protected $mailbox;

protected function setUp()
Expand Down Expand Up @@ -215,4 +217,50 @@ public function testAppendOptionalArguments()
$this->assertTrue($message->isSeen());
$this->assertSame(' 3-Jan-2012 09:30:03 +0000', $message->getHeaders()->get('maildate'));
}

public function testBulkMove()
{
$anotherMailbox = $this->createMailbox();

// Test move by id
$messages = [1, 2, 3];

$this->assertSame(0, $anotherMailbox->count());
$this->mailbox->move($messages, $anotherMailbox);
$this->getConnection()->expunge();

$this->assertSame(3, $anotherMailbox->count());
$this->assertSame(0, $this->mailbox->count());

// move back by iterator
$messages = $anotherMailbox->getMessages();
$anotherMailbox->move($messages, $this->mailbox);
$this->getConnection()->expunge();

$this->assertSame(0, $anotherMailbox->count());
$this->assertSame(3, $this->mailbox->count());

// test failing bulk move - try to move to a non-existent mailbox
$this->getConnection()->deleteMailbox($anotherMailbox);
$this->expectException(MessageMoveException::class);
$this->mailbox->move($messages, $anotherMailbox);
}

public function testBulkCopy()
{
$anotherMailbox = $this->createMailbox();
$messages = [1, 2, 3];

$this->assertSame(0, $anotherMailbox->count());
$this->assertSame(3, $this->mailbox->count());
$this->mailbox->copy($messages, $anotherMailbox);

$this->assertSame(3, $anotherMailbox->count());
$this->assertSame(3, $this->mailbox->count());

// test failing bulk copy - try to move to a non-existent mailbox
$this->getConnection()->deleteMailbox($anotherMailbox);
$this->expectException(MessageMoveException::class);
$this->mailbox->copy($messages, $anotherMailbox);
}
}