SPLIT #239:::Skip attachments

[nickl-]

Split PR of #239 all original commits by @commanddotcom only cherry picked, branched, squashed.

This fixes #150 #167 #122 provides a way to skip processing of attachments when it is not required.

[nickl-]

I totally agree we should avoid processing parts that we don't need but I am not sure this is the way.

That sure is a lot of ifs are we certain we have covered all the basis? Just looks like something that can cause major headaches constantly having to patch for edge cases slipping through or getting ignored, isn't this a wheel we already have? Since we are checking now is it still necessary to do isAttachment again?

php-imap/src/PhpImap/Mailbox.php

Line 652 in 56ed03e

$isAttachment = $partStructure->ifid || isset($params['filename']) || isset($params['name']);

Instead of something that is supposed to optimise performance we've added extra checks and balances, being punished to get the reward.

[nickl-]

Lets have a look and see what is going on here at initMailPart.

What do we have here

Message structure

We retrieve the structure at getMail so we already have the parts information.

php-imap/src/PhpImap/Mailbox.php

Lines 585 to 594 in 56ed03e

	$mailStructure = $this->imap('fetchstructure', [$mailId, FT_UID]);
	if(empty($mailStructure->parts)) {
	$this->initMailPart($mail, $mailStructure, 0, $markAsSeen);
	}
	else {
	foreach($mailStructure->parts as $partNum => $partStructure) {
	$this->initMailPart($mail, $partStructure, $partNum + 1, $markAsSeen);
	}
	}

Data retrieve and decode

Fetching the data for the part is what we want to avoid.

php-imap/src/PhpImap/Mailbox.php

Lines 613 to 618 in 56ed03e

	if($partNum) { // don't use ternary operator to optimize memory usage / parsing speed (see http://fabien.potencier.org/the-php-ternary-operator-fast-or-not.html)
	$data = $this->imap('fetchbody', [$mail->id, $partNum, $options]);
	}
	else {
	$data = $this->imap('body', [$mail->id, $options]);
	}

Without the data there is no need for expensive decodes.

php-imap/src/PhpImap/Mailbox.php

Lines 620 to 632 in 56ed03e

	if($partStructure->encoding == 1) {
	$data = imap_utf8($data);
	}
	elseif($partStructure->encoding == 2) {
	$data = imap_binary($data);
	}
	elseif($partStructure->encoding == 3) {
	$data = preg_replace('~[^a-zA-Z0-9+=/]+~s', '', $data); // https://github.com/barbushin/php-imap/issues/88
	$data = imap_base64($data);
	}
	elseif($partStructure->encoding == 4) {
	$data = quoted_printable_decode($data);
	}

No discrimination yet, we have the data long before we even know what we are looking at, now that's equal opportunity greedy loading right there.

Parameter collection

Next up is parameter collection, now this is perhaps a bit cumbersome but we already have the structure data so we might as well process it here.

php-imap/src/PhpImap/Mailbox.php

Lines 634 to 650 in 56ed03e

	$params = [];
	if(!empty($partStructure->parameters)) {
	foreach($partStructure->parameters as $param) {
	$params[strtolower($param->attribute)] = $this->decodeMimeStr($param->value);
	}
	}
	if(!empty($partStructure->dparameters)) {
	foreach($partStructure->dparameters as $param) {
	$paramName = strtolower(preg_match('~^(.*?)\*~', $param->attribute, $matches) ? $matches[1] : $param->attribute);
	if(isset($params[$paramName])) {
	$params[$paramName] .= $param->value;
	}
	else {
	$params[$paramName] = $param->value;
	}
	}
	}

By the looks of it, and I might be wrong, we are really only concerned with 3 parameters before those pretty params go to garbage:

filename and name

php-imap/src/PhpImap/Mailbox.php

Lines 662 to 666 in 56ed03e

	if(empty($params['filename']) && empty($params['name'])) {
	$fileName = $attachmentId . '.' . strtolower($partStructure->subtype);
	}
	else {
	$fileName = !empty($params['filename']) ? $params['filename'] : $params['name'];

and charset

php-imap/src/PhpImap/Mailbox.php

Lines 696 to 697 in 56ed03e

	if(!empty($params['charset'])) {
	$data = $this->convertStringEncoding($data, $params['charset'], $this->serverEncoding);

Not sure how many parameters we are collecting but based on this knowledge does it still make sense to decode everyone we find?

php-imap/src/PhpImap/Mailbox.php

Line 637 in 56ed03e

$params[strtolower($param->attribute)] = $this->decodeMimeStr($param->value);

Probably a tad over eager here but still it is based on information:

• we already have
• is required knowledge about the message
• might as well collect everything we might need so we don't have to look it up again

Type negotiation

We already have all the information we need to decide whether this is an attachment or not.

php-imap/src/PhpImap/Mailbox.php

Lines 652 to 659 in 56ed03e

	$isAttachment = $partStructure->ifid || isset($params['filename']) || isset($params['name']);
	// ignore contentId on body when mail isn't multipart (https://github.com/barbushin/php-imap/issues/71)
	if(!$partNum && TYPETEXT === $partStructure->type) {
	$isAttachment = false;
	}
	if($isAttachment) {

If it is an attachment there is a whole lot of boiler plating to be done but it turns out the only time we actually need the data is when/if we are going to write it to file.

php-imap/src/PhpImap/Mailbox.php

Lines 676 to 692 in 56ed03e

	if($this->attachmentsDir) {
	$replace = [
	'/\s/' => '_',
	'/[^0-9a-zа-яіїє_\.]/iu' => '',
	'/_+/' => '_',
	'/(^_)|(_$)/' => '',
	];
	$fileSysName = preg_replace('~[\\\\/]~', '', $mail->id . '_' . $attachmentId . '_' . preg_replace(array_keys($replace), $replace, $fileName));
	$attachment->filePath = $this->attachmentsDir . DIRECTORY_SEPARATOR . $fileSysName;
	if(strlen($attachment->filePath) > 255) {
	$ext = pathinfo($attachment->filePath, PATHINFO_EXTENSION);
	$attachment->filePath = substr($attachment->filePath, 0, 255 - 1 - strlen($ext)) . "." . $ext;
	}
	file_put_contents($attachment->filePath, $data);
	}

For everything else, that is apparently not an attachment we immediately start processing data again

php-imap/src/PhpImap/Mailbox.php

Lines 696 to 698 in 56ed03e

	if(!empty($params['charset'])) {
	$data = $this->convertStringEncoding($data, $params['charset'], $this->serverEncoding);
	}

Even before, assuming we have data after all that effort, we considered if we actually have the appropriate types to identify textPlain or textHtml.

php-imap/src/PhpImap/Mailbox.php

Lines 699 to 708 in 56ed03e

	if($partStructure->type == 0 && $data) {
	if(strtolower($partStructure->subtype) == 'plain') {
	$mail->textPlain .= $data;
	}
	else {
	$mail->textHtml .= $data;
	}
	}
	elseif($partStructure->type == 2 && $data) {
	$mail->textPlain .= trim($data);

The magic numbers should actually be replaced by the constants 0 = TYPETEXT and 2 = TYPEMESSAGE to clarify but not to mention there are a total of 9 possible types.

After this we continue with the recursive processing of additional sub-parts. It is clear that the data is hardly the most important thing but yet takes the highest priority. Even without getting fancy there is a lot that can be optimised already by simply only fetching data that we might actually need.

Lazy loading

Of course it makes sense not to retrieve every attachment if we are not even going to look at any attachments, that is just cray cray. Doesn't the same count for the message text as well? If we are only going to look at plain text then why do we bother with collecting html data and visa versa. The same goes for all the eager decodes rather wait until we actually ask for it.

From the look of things it should be simple enough to just collect the structure data. We have everything we need to retrieve the data so instead lets just capture the ingredients for now. Then only if you actually call getAttachments, getTextPlain or getTextHtml do we finally retrieve the data and do all the decode niceness.

Only pay for what you actually use.

[commanddotcom]

@nickl- Lazy loading sounds good in theory but what about backward compatibility?

[nickl-]

@commanddotcom I am sure we can remain on the no breaking side of the fence, what concerns you?

[barbushin]

Please check this #286

[Sebbo94BY]

Reference: https://stackoverflow.com/questions/11285430/imap-get-attached-file

[Sebbo94BY]

Tested this change: It works fine and backward compatibility is given, so nobody needs to update his / her code with this change.

I'll merge it and add some PHPUnit tests.