Add sizes() methods to protocol interface and implmentations

[didi1357]

Implemented as teased in Issue #378.

Relates to Issue #278.

[didi1357]

Damn, I expected this to be a quick/easy fix. Up to now I was only using/exploring the library via composer. Sorry to have bothered you with this already. I'll set myself a proper testing environment up. :)

[Webklex]

Hi @didi1357 ,
thank you for your pull request as well as your time and effort spend to implement a new feature.

I'm happily pressing the test button as often as it takes :)

Best regards & happy coding,

[didi1357]

Hey, no need to press the button yet. :D

I ran into an issue where you might have some valuable hint(s):

>> TAG6 FETCH 1 (RFC822.SIZE)

<< * 1 FETCH (RFC822.SIZE 9715)
<< TAG6 OK Fetch completed (0.001 + 0.000 secs).
<br />
<b>Fatal error</b>:  Uncaught Webklex\PHPIMAP\Exceptions\ResponseException: Command failed to process:
Causes:
	- Empty response
Commands send:
	TAG6 FETCH 1 (RFC822.SIZE)\r\n
Responses received:
	* 1 FETCH (RFC822.SIZE 9715)\r\n
	TAG6 OK Fetch completed (0.001 + 0.000 secs).\r\n
Error occurred in /home/didi1357/git/php-imap/src/Exceptions/ResponseException.php:53
Stack trace:
#0 /home/didi1357/git/php-imap/src/Connection/Protocols/Response.php(310): Webklex\PHPIMAP\Exceptions\ResponseException::make()
#1 /home/didi1357/git/php-imap/src/Connection/Protocols/Response.php(301): Webklex\PHPIMAP\Connection\Protocols\Response-&gt;validate()
#2 /home/didi1357/git/php-imap/src/Message.php(611): Webklex\PHPIMAP\Connection\Protocols\Response-&gt;validatedData()
#3 /home/didi1357/git/php-imap/src/Message.php(238): Webklex\PHPIMAP\Message-&gt;fetchSize()
#4 /home/didi1357/git/php-imap/src/Query/Query.php(462): Webklex\PHPIMAP\Message-&gt;__construct()
#5 /home/didi1357/git/php-imap/src/Query/Query.php(484): Webklex\PHPIMAP\Query\Query-&gt;getMessage()
#6 /home/didi1357/git/php-imap/tests/SizeTest.php(75): Webklex\PHPIMAP\Query\Query-&gt;getMessageByMsgn()
#7 {main}
  thrown in <b>/home/didi1357/git/php-imap/src/Exceptions/ResponseException.php</b> on line <b>53</b><br />
>> TAG7 LOGOUT

<< * BYE Logging out

The response seems to be fine, but the dissection in ImapProtocol->fetch() seems to fail somehow.

Another thing also in that function which seems a bit fishy imho is this construct:

...
            if ($to === null && !is_array($from) && ($uid ? $tokens[2][$uidKey] == $from : $tokens[0] == $from)) {
                // we still need to read all lines
                while (!$this->readLine($response, $tokens, $tag))
                    return $response->setResult($data);
            }
...

Without understanding the idea of that code: Are you sure, that the return inside the while is correct? It should either be an if, or no return inside, shouldn't it?

[didi1357]

Hey,

I think I'm done with this issue. The problem wasn't my implementation of the feature, but the broken MSGN implementation. No clue why I used that one for my test at first.

I added a test using UID which worked against my server. As it needs a real server, it's no unit test but just a file which should give anyone also trying that a head start.

Other than that, I tried to adapt your coding style as good as possible. Let me know if there is something you'd still like to have changed to get this merged. :)

[didi1357]

My SizeTest.php seems to break the Unit Test system. I will either need to remove it or move it to another folder. Any preference from your side?

[Webklex]

Hi @didi1357,
good observation:

php-imap/src/Connection/Protocols/ImapProtocol.php

Lines 698 to 703 in 46ef2a6

	// if we want only one message we can ignore everything else and just return
	if ($to === null && !is_array($from) && ($uid ? $tokens[2][$uidKey] == $from : $tokens[0] == $from)) {
	// we still need to read all lines
	while (!$this->readLine($response, $tokens, $tag))
	return $response->setResult($data);
	}

Looking at the git history, it always has been a while loop instead of an if. Since it behaves like an if
statement and it hasn't caused any issues, it managed to survive until you've managed to find it :) Good job 👍

Regarding the "Empty response" error:
There is an issue regarding the same error: #362

My current theory:
If you search for messages that don't exist. Such as searching for messages with the subject "fooBar" in a folder which
doesn't have any messages matching the search criteria, will result in en "empty" response - which is
technically correct because no messages were found and no data returned, but obviously false and not expected behavior.

Testing the size method might be a bit complicated. I'm not sure how to actually implement such an test environment.
I believe we'll need a temporary test environment which boots and provides a temporary imap / mail server. But maybe they
could also be provided via hidden environment variables? I really don't know.. If you like to explore this topic please feel
invited to do so and welcome to share your knowledge :)

If you want to dive in, checkout barbushin's amazing work:
https://github.com/barbushin/php-imap/tree/master/tests/unit
https://github.com/barbushin/php-imap/blob/master/.github/workflows/php_unit_tests.yml

I hope I could answer your questions.

Best regards & happy coding,

Edit:
https://github.com/ddeboer/imap/tree/master/.github looks also promising.

[didi1357]

Since it behaves like an if statement and it hasn't caused any issues, it managed to survive

How do we proceed with that? Do you implement that on master? Do I fix it on this branch? I'd suggest we go one by one => No fix on this branch. I'd even say it was my fault to tell you this within this issue. However I'd appreciate a decision of you.

My current theory:

Please take a look closer at my log.
The line with
* 1 FETCH (RFC822.SIZE 9715)\r\n
suggests, that a message with size 9715 was found, but wasn't dissected correctly. I suspect that the problem was introduced with the change in MSGN handling as discussed in issue #356.

However, I'd also suggest to go one by one with this problem and leave that out of the scope of this PR.

Testing the size method might be a bit complicated.

Definitely. You need a working 24/7 IMAP server as test infrastructure to properly do this with a unit test. Also seems out of scope of this PR to me. To me the question is more about whether the file should be somewhere outside of the unit tests in this repository or just completely dropped. Both solutions are okay to me. I'd also just appreciate a decision of you about this. I'll unfortunately not have the time to implement a whole test infrastructure :S

Best regards and especially: A BIG THANK YOU for your fast response. This is definitely not to be taken as granted for an open source project like this one!

[Webklex]

I don't expect a test case at all. I'm glad you took on the task to implement the method - I don't expect from you to do something I don't even have an idea of :)

I managed to verify and fix my suspected search issue - so it looks like we were describing two different bugs. The exception "Empty response" isn't really descriptive and can certainly be improved. However similar to my fix (allowing the response to be empty (https://github.com/Webklex/php-imap/blob/master/src/Connection/Protocols/ImapProtocol.php#L1199)) there might be similar occasions were such a behavior is actually wanted.
Your case does indeed look different. You were expecting a response but didn't get any.
I just merged #356, do you still get this exception?

I'm not so sure if a 24/7 server is actually required. If I understand this correctly, you can "boot" a docker container as a step inside the github actions and keep it active during the unit test.

Yes we are going all over the place inside this pr, but that's alright. If you want to fix something else or tackle a certain feature, I agree and recommend to create a separate issue / pr in order to give others a chance to join or discover it later on.

Best regards,

[Webklex]

P.s.: regarding

php-imap/src/Connection/Protocols/ImapProtocol.php

Lines 698 to 703 in 46ef2a6

	// if we want only one message we can ignore everything else and just return
	if ($to === null && !is_array($from) && ($uid ? $tokens[2][$uidKey] == $from : $tokens[0] == $from)) {
	// we still need to read all lines
	while (!$this->readLine($response, $tokens, $tag))
	return $response->setResult($data);
	}

If you like to convert that while into an if, feel free to do so within this pr or create a new pr. However you like - I'm happy to merge it any way :)

[didi1357]

Hey, I

• squashed my previous nonsense to one new commit
• rebased on your current master state
• replaced the while with an if as suggested by you
• moved the SizeTest.php to size_test.php in examples folder => Your unit tests should pass again

Let me know if there is anything still to be done to get this merged.

Br, Didi

[Webklex]

Hi @didi1357,
thanks for the update :)

One question regarding: https://github.com/didi1357/php-imap/blob/master/src/Message.php#L238

This will cause one additional request for every message to be fetched. How about moving the fetch logic to
https://github.com/didi1357/php-imap/blob/master/src/Message.php#L436-L443 and treat it as a message attribute like uid and msgn.
I'm not sure if your initial commit had this as well - if so, I'm sorry that I've missed this earlier.

Something like this:

case "size":
    if (!isset($this->attributes[$name])) {
        $this->fetchSize();
    }
    return $this->attributes[$name];

https://github.com/didi1357/php-imap/blob/master/src/Message.php#L609-L616

private function fetchSize(): void {
    $sequence_id = $this->getSequenceId();
    $sizes = $this->client->getConnection()->sizes([$sequence_id], $this->sequence)->validatedData();
    if (!isset($sizes[$sequence_id])) {
        throw new MessageSizeFetchingException("sizes did not set an array entry for the supplied sequence_id", 0);
    }
    $this->attributes["size"] = $sizes[$sequence_id];
}

Doing so will spare the request and only fetch the size if its requested. This will probably save a couple of
seconds if you fetch upwards of a few 100 or 1000 messages.

Btw: did you meant to include the local test case under examples?
https://github.com/didi1357/php-imap/blob/master/examples/size_test.php

Best regards & happy coding,

[didi1357]

Hey, awesome. I didn't look deep enough into this logic, but this definitely makes sense. I'll implement accordingly.

Yes. I moved it there, as all files in the tests folder seem to be run by the unit test system and unfortunately my test isn't compatible to that as it needs an IMAP server.

[Webklex]

Awesome, thank you :)