Improve IMAP response handling

[Webklex]

Usually every command send to an imap server has one or several responses. Currently they don't always get treated and checked the way they should be.

This was discovered during a discussion on issue #263

For example the command APPEND:
RFC references:

• https://www.rfc-editor.org/rfc/rfc3501#section-6.3.11
• https://datatracker.ietf.org/doc/html/rfc9051#section-6.3.12

Possible responses:

TAG3 OK APPEND completed

..or:

TAG3 OK [APPENDUID 38505 3955] APPEND completed

There are also intermediate responses like:

+ Ready for literal data

..which indicates the server is ready to accept additional data.

Bad commands or server errors are currently only partially handled and should be handled for every command send. Every command can fail in two ways. Either with a NO or BAD alongside some additional and provider specififc error message.

To improve the response handling, every command send to the server, should return an ImapResponse::class instance which holds information such as the send command and received responses / errors. This response class might also include "magic" accessors to retrieve named data from the response (like the Attribute::class class).

[Webklex]

@szymekjanaczek suggested to use something similar to Laravel Resources:

Like Resources in Laravel - but firstly we have to check the compatibility between responses of each operation. In the ImapResponse::class we can parse responses and return them as specific params (like eg. UIDs of newly created messages - in case of copying or appending: OK [COPYUID <some number> <copied uids> <uids in the new folder>] Copied UIDs)

I like the idea. Using a known schema or creating one which behaves similar to a known one has definitely some benefits. But I have to admit, that I haven't knowingly worked with Laravel Resources before.

Regarding potential dependencies

This library already uses third party dependencies:

php-imap/composer.json

Lines 21 to 31 in cc3e72f

	"require": {
	"php": ">=7.0.0",
	"ext-openssl": "*",
	"ext-json": "*",
	"ext-mbstring": "*",
	"ext-iconv": "*",
	"ext-fileinfo": "*",
	"nesbot/carbon": ">=1.0",
	"symfony/http-foundation": ">=2.8.0",
	"illuminate/pagination": ">=5.0.0"
	},

But at the same time I would like to keep them at a minimum.

One additional thought. I'm a bit worried about the overhead we might create if we do integrate an response interface in the first place. How much longer will it take to fetch, lets say 1000 messages?

[szymekjanaczek]

Okey, some thoughts:

Divide and conquer

I think we can (and maybe should) divide this project into two parts:

1. refactor current code base to return raw array response from IMAP command - instead of booleans.
2. prepare universal ImapResponse::class

Why?

1. If we release a version with raw data, probably it would help with gathering more people to work on this feature - more observators of possible IMAP responses. So then, with more experience, it would be easier to create ImapResponse::class
2. Also it could be one of the methods: ->getResponse() and ->getRawResponse() (or sth similar). So (more demanding) users of this lib will be able to work on their own on the response (good for unique individual usages or requirements).

About Laravel Resources and additional packages

There is no need to install any additional lib. We can create our own DTO as well. I mentioned Laravel's resources only as an example of data structure.

---

I'm not sure what do you mean at this:

One additional thought. I'm a bit worried about the overhead we might create if we do integrate an response interface in the first place. How much longer will it take to fetch, lets say 1000 messages?

[Webklex]

Sounds good to me 👍

I've meant that every class / object you create during the runtime will need some additional system resources. At some point this adds up and could slow down the operation.

[szymekjanaczek]

Hmm, you're right - optimization is really important.
Additionally IMAP has its own limits - eg. query length - as I've read somewhere in docs. It's another aspect to think over.

About fetching messages - I think we can leave it at the current version - and instead focus on the following operations (and their responses):

• append
• copy
• move
• delete
• mark

Because the response of fetching messages operation is already done - and works fine (but I'm a bit worried about this: performance of version 4.0.0 #443)

[szymekjanaczek]

So for now, I suggest:

1. Change operations mentioned above return types to array - and as response give back raw data from IMAP.
2. Add custom exceptions for these operations - as the following docs says^[a]:

/**
 * @return array|bool|null tokens if success, false if error, null if bad request
 */

[a] - instead of dividing into "error" or "bad request", we can temporarily treat both of them as errors and throw them as exceptions with eg. imap_last_error(). And more precise error handling build with the new ImapResponse::class.

[szymekjanaczek]

Hi @Webklex
Can you take a look at my PR, please? It's a kinda draft of changes in the ImapProtocol.php. Changes in legacy are still needed. But I'd prefer to consult the current schema/idea with you.

Additionally - do we have any unit/feature tests?

[Webklex]

Hi @szymekjanaczek ,
yes I agree on both 1.) and 2.) however I'm having some trouble with [a]. This would add a php-module dependency in the mix, which is currently optional (php*-imap).

There are currently no unit tests available. I gave it a try a few times, but had trouble to work with faked resources.

Also thanks for your pull request! Ignore my two questions from there - I now understand why you've required the php-imap module. I'm just not sure how to proceed - and if it's worth to try to keep php*-imap out of it.

Best regards,

[ainesophaur]

Is there a reason to keep the legacy protocol around if theres going to be as major of a rewrite as the PR? From what I can see, legacy protocol was to work around not having php*-imap installed.

If a majority of the operations are going to change their return values, then this would jump the project to v5 as its a pretty breaking change. I think it's worth dropping the legacy protocol and the optional uncoupling from php*-imap and just making it a requirement.

If pr #277 does go through, I intend to follow up with some tests if they're not included by either of you two fine individuals.

[Webklex]

@ainesophaur ,
sounds reasonable to me. I wouldn't mind kicking php*-imap out entirely.
What are possible downsides? Folks which rely on the legacy mode are stuck with v4.x, header decoding quality could for some under certain circumstances deteriorate (currently the php*-imap module can be used as a fallback to support header value decoding - for some reason some users had reported problems with my implementation. I never truly found out why, so it could be a "ghost" problem as well). Am I missing something / someone?

Yes we are talking about a v5. I believe the proposed changes are worth to introduce some breaking changes. After all, those changes are there to help us all in the long run :)

[Webklex]

By kicking out php*-imap we also reduce the amount of code that has to be refactored and tested. Win / Win ?

[Webklex]

Hi @szymekjanaczek @ainesophaur ,
I've created a v5 branch: https://github.com/Webklex/php-imap/tree/v5
Please create a pull request to this branch instead of the master / main. By doing so we can play with it isolated from the main branch. Once we are happy with v5 I'm going to merge it into main.

Otherwise the main branch is locked until we've finished v5.
If you think this is a bad idea, please feel free to let me know - I'm happy to learn more :)

Best regards,

[szymekjanaczek]

Hi!

It looks like a good idea to me - gonna start coding off-the-clock ;).

Greetings!

[ainesophaur]

@szymekjanaczek how similar is the work in #277 to what you're working on? @Webklex do you have any preferred way of discussing/planning/communicating regarding v5 work?

[szymekjanaczek]

@ainesophaur it's a kinda draft - as I mentioned. Instead of returning less-informative booleans as a response, I switched it to give back the raw response from the IMAP server after the operation. It's quite connected with this issue - because is focussed on more detailed output. But in v5 we would like to return it like a special class (eg. ImapResponse::class) with an organized structure, similar in each operation (to work with eg. PHPStan).

About the communication way - firstly we have to gain interested people. @Webklex what do you think about the small announcement in the README section? Or in discord (oh, the invitation link expired ;-;) If you fix it, we can use discord for this purpose.

@ainesophaur, can I assume, you would like to help?

[ainesophaur]

Absolutely. I'm working a project that heavily depends on this library so I'd love to contribute back.

…

On Fri, Oct 7, 2022, 2:50 AM Szymon Janaczek ***@***.***> wrote: @ainesophaur <https://github.com/ainesophaur> it's a kinda draft - as I mentioned. Instead of returning less-informative booleans as a response, I switched it to give back the raw response from the IMAP server after the operation. It's quite connected with this issue - because is focussed on more detailed output. But in v5 we would like to return it like a special class (eg. ImapResponse::class) with an organized structure, similar in each operation (to work with eg. PHPStan). About the communication way - firstly we have to gain interested people. @Webklex <https://github.com/Webklex> what do you think about the small announcement in the README section? @ainesophaur <https://github.com/ainesophaur>, can I assume, you would like to help? — Reply to this email directly, view it on GitHub <#270 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABDYC5SUWCJVUYBFU6OP44TWB7BZXANCNFSM57S54HGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[szymekjanaczek]

Okay, @ainesophaur so we have a similar situation. Nice - nothing motivates more than need.
I think we can start on our own discord - just DM me at szymekjanaczek#4390

[Webklex]

Hi @szymekjanaczek ,
I appreciate your enthusiasm and time your are giving this project! I know I'm lagging behind, sorry for that.
You are right, a discussion is probably easier via discord or something similar, compared to this issue section :)

https://discord.gg/rd4cN9h6

For example, should this library stay compatible with php >= v7/v7.2 or is >= v8 enough?
eg.: https://github.com/Webklex/php-imap/pull/277/files#diff-fa4f22d9f79d3e4dd1e5352f5e967291c02a7b3a39e32a0282d9b7d85c707c5aR132 this function came with php 8.

Given all the "new" and great features php 8 provides, it's perhaps a big blocker if this library continues supporting php < v8.