Location of thrown exceptions make them impossible to catch properly.

[RickKukiela]

laminas-mail/src/Header/GenericHeader.php

Lines 49 to 67 in 0516586

	public static function splitHeaderLine($headerLine)
	{
	$parts = explode(':', $headerLine, 2);
	if (count($parts) !== 2) {
	throw new InvalidArgumentException('Header must match with the format "name:value"');
	}
	if (! HeaderName::isValid($parts[0])) {
	throw new InvalidArgumentException('Invalid header name detected');
	}
	if (! HeaderValue::isValid($parts[1])) {
	throw new InvalidArgumentException('Invalid header value detected');
	}
	$parts[1] = ltrim($parts[1]);
	return $parts;
	}

Hello. The above reference method throws exceptions while "loading" the mailbox information meaning you have to wrap your message loop in a try/catch block to catch them but doing that offers no way to "continue" processing the loop after encountering an email with "invalid headers".

Often times it does not matter to what I or others are doing if the email has invalid headers and would prefer not to put the "kabash" on whole routine just because of something like this.

I recommend, instead of checking the headers as it loads the mailbox data as it apparently does right now, instead have them checked when the message is accessed for the first time, e.g.:

foreach ($mail as $messageNum => $message) {

(The above line should not trigger an exception.)

Instead:

foreach ($mail as $messageNum => $message) {
    $message->method();
    // ANY method call could do all the checks the first time.
    // Mark the message object clean so further calls to methods do not trigger re-checks.
    // The above line could be wrapped in try/catch and allow the loop to continue to the next message without failing.
}

[Ocramius]

I disagree here: any kind of iterator that loads data can potentially fail, and the mail component is the same.

Moving the crash from the loading of an item to the loading of a specific detail of a message later on only moves the "mine".

This could have been foreach ($images as $image) { echo $image->size(); }: if $image is not a valid image, it should never be instantiated at all, of possible (with a crash being a valid outcome).

If I/O requires asynchronous/lazy loading, you shift the exception, making the user more involved in it.

I agree that it isn't nice to get a crash mid-iteration, but I also think that shifting the exception further inside the loop causes even worse problems.

[Slamdunk]

All \Laminas\Mail\Storage\AbstractStorages implement the \SeekableIterator interface, that means that you are not bound to the foreach statement to iterate them.
The following example is a legit alternative to read all the messages:

/** @var \Laminas\Mail\Storage\AbstractStorage $mail */
$mail->rewind();
while ($mail->valid()) {
    try {
        $message = $mail->current();
    } catch (\Throwable) {
        // Do stuff
    }
    
    $mail->next();
}

[RickKukiela]

@Slamdunk I like that solution, its much more elegant that what I came up with. I feel that this should be mentioned in the docs at the least though, as https://github.com/laminas/laminas-mail/blob/2.22.x/docs/book/read.md (only documentation I could find here that show how to check a mailbox and read the mail) only uses the foreach() example in all cases and makes no effort to discuss exception handling. Given that there are apparently issues with header validation here that need to be addressed (read: no other mail clients had a problem reading the message that this library was failing to load due to "invalid headers"), it would have been nice to know how to properly iterate the mailbox in a way that allows me to handle issues.

On a related note, I'm not sure "invalid" headers are worthy of an exception in the first place. By taking that approach, I believe you are preventing the caller from reading an email body just because your validator didn't like the format or the way certain characters are encoded in the header (whether or not the headers are valid and this is a bug not withstanding). That is to say, given the code above (I haven not actually tried it yet), it seems that handling the issue that I ran into in this manner would still not allow me to actually parse the body of the message that had "invalid headers". If I'm wrong here please feel free to correct me. It just seems to me that's how this scenario would play out given the sample code above - I would be able to continue past the "broken" message in this case but I still do not think I could have read the body, right?

Maybe a feature could be added where header validation could be disabled ahead of time and/or instead set an errors array on the message object where any non-fatal errors could be logged. That way the message body could still be parsed and the caller could still be made aware of any issues that were detected via something like if(!empty($message->errors)) { or if(!$message->hasErrors) { you catch my drift.

Food for thought :)

[RickKukiela]

@Ocramius I think you make a valid point but your example is misrepresenting the issue at had in a way. In your example I fully agree, if the image is invalid it should not be be loaded, at face value, but in this case the the "header" of the email was invalid but the other parts of it were not and there is a use-case for wanting to still have access to the non-invalid parts of the message. The entire message is not invalid in this case, only a single, non-consequential header (a spam header in this case) was what it didn't like. I don't think its warranted to treat the entire email message as invalid just because of an issue with a single header.