MessageIterator::current() fails when there are no messages

[northys]

Hi,

MessageIterator::current() fails when you perform search in mailbox and no messages. This probably also occurs when you list messages by getMessages and mailbox is empty

Type error: Argument 2 passed to Ddeboer\Imap\Message::__construct() must be of the type integer, null given, called in /home/northys/code/profi-dj/beowulf-crawler/vendor/ddeboer/imap/src/MessageIterator.php on line 34

[Slamdunk]

Hi, can you post the code that reproduce the error? Because this can only happens when you manually instantiate MessageIterator with a non-empty array of empty values as a second argument, like array(0 => null), but this never happens if you natively call Mailbox::getMessages().

I've just pushed a new commit caacd25 to prove this, and as you can see on Travis the build succeeds: https://travis-ci.org/ddeboer/imap/builds/331410487

[northys]

Hi @Slamdunk,

thank you for fast response :)

Here is block of code which caused the error. I use SearchExpression to search messages. When no messages are found the ->current() call fails.

$searchExpression = new SearchExpression();
$searchExpression->addCondition(new From($rule['from']));
$searchExpression->addCondition(new Subject($rule['subject']));
return $mailbox->getMessages($searchExpression, SORTDATE)->current(); // ouch

Maybe I'm doing something wrong. You can see my workaround bellow.

// build search expression, fetch messages into $messages - same code as above
if ($messages->valid()) {
    $message = $messages->current();
} else {
    $this->logger->notice('No message found for this rule.', $rule);
    continue;
}

[Slamdunk]

Hi, I reproduced the bug, you are right.

But I am honestly against changing the current behaviour to fulfil a valid value, because there isn't a unique result neither in the simple case; take for example:

$array = new \ArrayIterator(array());

var_dump(current($array));
var_dump($array->current());

What do you expect? Here the answers: https://3v4l.org/WuFZ0

I don't want to return false nor NULL, instead I'm inclined to throw an exception to tell the user a valid() check must be used, or a native getArrayCopy() or foreach it better suited to cycle the messages.

[northys]

Wow, null and false is wtf... Thank you for fixing it in PR, good job!

What do you think about my workaround? Could you recommend me better way to get the oldest message from search? I do not need more messages, just the oldest one.

[Slamdunk]

Your way looks good, makes sense and its purpose is easy to understand, keep it like that.