moveMessage(): Return value must be of type bool, array returned

[davidwhicker]

Webklex\PHPIMAP\Connection\Protocols\ImapProtocol::moveMessage(): Return value must be of type bool, array returned

When moving message the above error is displayed.

The move is successful but error is logged.

[Webklex]

Hi @FortuneV13 , I believe this was fixed by this commit: https://github.com/Webklex/php-imap/commit/33ee4d2eb8bc1645489c0715c0412c42746c90ac

Best regards,

[davidwhicker]

Sorry I have just realised I created this on the wrong repo. It should be the laravel imap repoKind regardsDavid WhickerOn 23 Aug 2022, at 17:21, Webklex @.***> wrote: Hi @FortuneV13 , I believe this was fixed by this commit: 33ee4d2 Best regards,

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[Webklex]

Hi @FortuneV13 , the error originated in this repository. But a similar issue was reported with issue #261 and git fixed in the commit mentioned above. It's just not released yet. I'm still working on some fixes which I would like to include in the next release.

Best regards & happy coding,

[davidwhicker]

That’s great, I’ll leave it in your hands. Thank you for your work on this :)Kind regardsDavid WhickerOn 23 Aug 2022, at 17:44, Webklex @.***> wrote: Hi @FortuneV13 , the error originated in this repository. But a similar issue was reported with issue #261 and git fixed in the commit mentioned above. It's just not released yet. I'm still working on some fixes which I would like to include in the next release. Best regards & happy coding,

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[Webklex]

If you don't mid, I would like to hear your thought regarding the release frequency: #265

Best regards,

[davidwhicker]

If possible known bugs like this to be released asap rather than waiting for a collective amount of bug fixes. That said what ever is easiest for you. Kind regardsDavid WhickerOn 23 Aug 2022, at 18:08, Webklex @.***> wrote: If you don't mid, I would like to hear your thought regarding the release frequency: #265 Best regards,

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[szymekjanaczek]

Hi, @Webklex I'm not sure about force casting bool here:

return (bool)$this->requestAndResponse($command, [$set, $this->escapeString($folder)], true);

Why? Because in the previously returned array were some useful info - like tokens. I was using them to get data of moved/copied/created messages. What do you think about getting back the array and instead of returning less-detailed true/false, throwing an exception in case of error?

[Webklex]

Hi @szymekjanaczek , that sounds like a good idea to me! Do you like to push a pull request? I'll patch it otherwise later today.

Best regards,

[szymekjanaczek]

Glad to hear that!

I'm thinking about sth like this:

    /**
     * Append a new message to given folder
     * @param string $folder name of target folder
     * @param string $message full message content
     * @param array|null $flags flags for new message
     * @param string $date date for new message
     *
-    * @return bool success
+    * @return bool success
     * @throws RuntimeException
     */
-   public function appendMessage(string $folder, string $message, $flags = null, $date = null): bool {
+   public function appendMessage(string $folder, string $message, $flags = null, $date = null): array {
        $tokens = [];
        $tokens[] = $this->escapeString($folder);
        if ($flags !== null) {
            $tokens[] = $this->escapeList($flags);
        }
        if ($date !== null) {
            $tokens[] = $this->escapeString($date);
        }
        $tokens[] = $this->escapeString($message);

-         return (bool) $this->requestAndResponse('APPEND', $tokens, true);
+        $response = $this->requestAndResponse('APPEND', $tokens, true);
+
+        if (!is_array($response)) {
+           throw new MessageAppendingException();
+        }
+
+        return $response;
    }

Basing on doc of readResponse method:

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

I can also add more detailed exceptions - one for imap error, one for bad request.

What do you think?

[Webklex]

I just looked at some rfcs:

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

The response could look like this:

TAG3 OK APPEND completed

..or:

TAG3 OK [APPENDUID 38505 3955] APPEND completed

(TAG3 always consist of TAG plus a number)

Looking at the example, I'm kind of wondering how / why it is currently working..

C: A003 APPEND saved-messages (\Seen) {326}
S: + Ready for literal data
C: Date: Mon, 7 Feb 1994 21:52:25 -0800 (PST)
C: From: Fred Foobar <[email protected]>
C: Subject: afternoon meeting
C: To: [email protected]
C: Message-Id: <[email protected]>
C: MIME-Version: 1.0
C: Content-Type: TEXT/PLAIN; CHARSET=US-ASCII
C:
C: Hello Joe, do you think we can meet at 3:30 tomorrow?
C:
S: A003 OK APPEND completed

(S = Server, C = Client)

The code should wait until a + Ready for literal data has been received from the server and push the message content afterwords. Also why is a $date attribute required? Its not getting used. And the size (in this case 326) isn't send. In theory this command should be invalid - but it somehow isn't and does work?

[Webklex]

Figured out why $date is required - is required for the legacy protocol.

\imap_append($this->stream, $folder, $message, $flags, $date);

In order to use the same interface, this method has to have a $date attribute - this could be used to set the message date header, if it hasn't been set before.

But this would also mean that the message has to be parsed - so this will add quite some complexity to this method.

[Webklex]

I start to realize, that a bool cast might not be a good idea at all. Usually all commands can return something (if a newer imap protocol version is used). Similar to the above mentioned APPEND responses. Therefor MOVE, COPY, etc shouldn't be a boolean either but an array instead. But how do we handle those responses where there is no additional context provided? Returning an empty array isn't great. Throwing an exception could be one way, but that would add additional complexity.

Perhaps its time to think about general response handling - always return an ImapResponse::class instance, which contains all available request and response information? This could also help if you are debugging something.

@szymekjanaczek we should move this discussion into a new issue, to reduce the amount of messages we are sending to @FortuneV13 and also to separate the topic :)

[szymekjanaczek]

Yeah, separate universal response sounds nice - sth like Resources in Laravel - but firstly we have to check the compatibility between responses of each operation.

Then - 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)

And I also agree about having mercy on @FortuneV13's mailbox ;) Are you going to create a new issue or should I do it?

[davidwhicker]

Yeah, separate universal response sounds nice - sth like Resources in Laravel. There 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)

And I also agree about having mercy on @FortuneV13's mailbox ;) Are you going to create a new issue or should I do it?

haha thanks chaps. And thank you for your work.

[szymekjanaczek]

Figured out why $date is required - is required for the legacy protocol.

\imap_append($this->stream, $folder, $message, $flags, $date);

In order to use the same interface, this method has to have a $date attribute - this could be used to set the message date header, if it hasn't been set before.

But this would also mean that the message has to be parsed - so this will add quite some complexity to this method.

Hmmm, in PHP docs $date looks like optional/nullable param:

imap_append(
    IMAP\Connection $imap,
    string $folder,
    string $message,
    ?string $options = null,
    ?string $internal_date = null
): bool

[Webklex]

@szymekjanaczek Yes it's currently optional as well: https://github.com/Webklex/php-imap/blob/cc3e72f8517b0bf169a91d9fe5831ebd5740874d/src/Connection/Protocols/LegacyProtocol.php#L388-L397

But this means it has to be there - otherwise you wont be able to set it. Or am I missing something? If you are wondering about the if statement - well the given date can't be null. It has to be something or not set at all.

[davidwhicker]

Thank you for coming back to me. I’m already on the latest version. Install today. Kind regardsDavid WhickerOn 23 Aug 2022, at 17:21, Webklex @.***> wrote: Hi @FortuneV13 , I believe this was fixed by this commit: 33ee4d2 Best regards,

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>