Suggestion: Change DataPartInfo::fetch() functionality

[AntonioDiPassio-AppSys]

Hi there

I'll quickly describe my situation: I am using Laravel to process emails with Laravel's Job Queue. This requires me to serialize and unserialize the messages, which also renders the imapStream unusable. The issue I am having is when I try to access the contents of an attachment using IncomingMailAttachment::getContents(). This uses DataPartInfo::fetch(), which in turn always tries to fetch from the imap server. This will fail when the message has been serialized/unserialized.

My suggestion is changing the functionality of DataPartInfo::fetch() to just fetching the datapart, and creating another function getData() to return the contents. Then you can change all the fetch() calls to getData() calls (this is only used in 2 places)

public function getData(){
        if(null === $this->data){
            $this->fetch();
        }
        return (null === $this->data) ? '' : $this->data;
    }

DataPartInfo::fetch() could then be changed into:

public function fetch()
    {
        if (0 === $this->part) {
            $this->data = Imap::body($this->mail->getImapStream(), $this->id, $this->options);
        } else {
            $this->data = Imap::fetchbody($this->mail->getImapStream(), $this->id, $this->part, $this->options);
        }

        //decode
        switch ($this->encoding) {
            case ENC8BIT:
                $this->data = \imap_utf8((string)$this->data);
                break;
            case ENCBINARY:
                $this->data = \imap_binary((string)$this->data);
                break;
            case ENCBASE64:
                $this->data = \base64_decode((string)$this->data, false);
                break;
            case ENCQUOTEDPRINTABLE:
                $this->data = \quoted_printable_decode((string)$this->data);
                break;
        }

        //convert encoding
        if (isset($this->charset) and !empty(\trim($this->charset))) {
            $this->data = $this->mail->decodeMimeStr(
                (string)$this->data // Data to convert
            );
        }
    }

[Sebbo94BY]

Might be related to #585.

[Sebbo94BY]

@AntonioDiPassio-AppSys is this still required? I believe not.

I also don't see any advantage in changing this.

The code of your getData() also doesn't make any sense to me as it contains two identical if-conditions, which return different values when it's true.

[AntonioDiPassio-AppSys]

@Sebi94nbg well, I have since moved on to a different package, but I feel it could still be usefull. The issue with your fetch() function is that it always reads from the Imap stream, even if $this->data is already set, which causes a problem if you serialize/unserialize the object.

My getData() returns the data if it's set, but uses the fetch() if not. This way, the imap stream is only read if the data is null.

The first if-condition doesn't actually return a value, it calls $this->fetch() which in turn populates $this->data. So what it does is: First check if data is set. If not, run $this->fetch() to populate it. Then check again if data has been set by running the fetch, and return '' or data.

I also merged your decodeAfterFetch() and convertEncodingAfterFetch() into the fetch() because I saw no reason to create separate function calls for it, as they are always called in succession.

[Sebbo94BY]

Ok, but when I understood you right, then the version 4.5.0 of this package is now also doing your suggested logic: https://github.com/barbushin/php-imap/compare/4.4.0...4.5.0

It's not splitted into an own function, but it should do the same as you've described. Do you agree with that or are you still expecting a different change?

Seperating or merging the last two mentioned functions is a different topic, but doesn't really affect the performance of the code.

[AntonioDiPassio-AppSys]

Version 4.5.0 does my logic, but only partially. The whole purpose of the fetch() function is to return this->data. Why not return it immediately, even if $this->part === 0?

` if (null !== $this->data) { return $this->data; }

if (0 === $this->part) { $this->data = Imap::body($this->mail->getImapStream(), $this->id, $this->options); } else { $this->data = Imap::fetchbody($this->mail->getImapStream(), $this->id, $this->part, $this->options); }

... `

This would be the same as my code then. The only reason I split it into 2 functions (getData and fetch), was just to have a single responsibility per function. getData for returning the data, and fetch for fetching the data via the imap stream.