Added bulk set / clear flags functionality for mailbox messages

[wujku]

No description provided.

[Slamdunk]

It should be:

@param string $flag   \Seen, \Answered, \Flagged, \Deleted, and \Draft
@param array $numbers Message numbers

Same rule goes for clearFlag

[Slamdunk]

You can directly return the result.

[Slamdunk]

Return type of implode is already a string, no need for the type casting

[Slamdunk]

Return type of implode is already a string, no need for the type casting

[Slamdunk]

$flag param misses the string type

[Slamdunk]

Hi, thank you for this PR.
Before merging I have only one doubt about the numbers parameters to be an array.
The manual says:

A sequence of message numbers. You can enumerate desired messages with the X,Y syntax, or retrieve all messages within an interval with the X:Y syntax

The user can seamlessly write:

$mailbox->setFlag('\\Seen', [
    '1',
    '2',
    '3:10',
    '11,12,15',
]);

And the code would work as it's expected.

But if I can write sequences by hand, what is the added benefit of requesting the parameter to be an array?
I can write

$mailbox->setFlag('\\Seen', ['1,2,3:10,11,12,15']);

Bypassing the check.

The only benefit I see is we can save an implode call to the user if he/she actually give the array filled as you wrote in the test.

[Slamdunk]

I've tested that creating the messages in different mailboxes and using the setFlag method on a random mailbox, the flags are changed to all the UIDs specificed because, as the name said, the flags are changed by UIDs regardless on the mailbox the mails are in.

As such I find misleading to have these two new methods in the Mailbox class since they don't depend on it.

What about moving the code (and the test) to the Connection?

[wujku]

Check 189815a, UIDs are related with specific mailbox. Moving functionality to Connection would require a mailboxName parameter.

[wujku]

@Slamdunk check 9f9f665

[Slamdunk]

Can you rebase the PR please?

[wujku]

@Slamdunk Done

[Slamdunk]

Hi, I have another API doubt. In the test you write:

$message = $mailbox->getMessage('1');
var_dump($message->isSeen()); // True

$mailbox->clearFlag('\\Seen', '1');

$message = $mailbox->getMessage('1');
var_dump($message->isSeen()); // False

And this works correctly because each getMessage call creates a new Message instance, so without cache. But if I do this:

$message = $mailbox->getMessage('1');
var_dump($message->isSeen()); // True

$mailbox->clearFlag('\\Seen', '1');

var_dump($message->isSeen()); // True

The user can be fooled by reusing the same previous message, and this is because the internal Message cache was not cleared.

As far as I can see, the only solution to this would be to couple Message instances to the Mailbox, calculate the sequence number provided to the new two methods and clear related caches: too complex, doesn't worth the effort.

So lets go for an easier path: @wujku plese update the README.md Mailboxes chapter showing the new functionalities and add a warning about this edge behaviour that we are not going to address: the user must be aware of it and retrieve new Message instances if they want to set bulk flags and refresh the single Messages flags.

[Slamdunk]

Another thing: please don't squash the commits, only rebase it. Squash makes review code changes harder. I'll squash anyway the PR before merge.

[wujku]

@Slamdunk What to do with the Scrutinizer error? I did nothing with composer packages.

[Slamdunk]

Just ignore it: look only at Travis, your PR is fine 👍

[Slamdunk]

@wujku I'll mere this PR as soon as you commit and push the changes to README.md I underlined:

So lets go for an easier path: @wujku plese update the README.md Mailboxes chapter showing the new functionalities and add a warning about this edge behaviour that we are not going to address: the user must be aware of it and retrieve new Message instances if they want to set bulk flags and refresh the single Messages flags.

[Slamdunk]

Thank you @wujku