Solving conversion of mails with Swiftmailer

[risototh]

This will allow us to convert msg files to eml file, even if the source uses some invalid email addresses or some of the data are missing at all. Without this, it can fail on any of the headers, and thus prevent the conversion, and it's better to present at least something to user, than nothing.

[hfig]

I ... don't like this. You're swallowing all errors, expected or otherwise.

I think the idea, overall, is reasonable -- MAPI to Mime is an imperfect translation and so the implementer can expect that some fidelity is lost. But by omitting "random" fields the we ordinarily convert, you're kind of breaking the contract.

You can verify if an email address is invalid and will throw using the example in the SwiftMailer docs:

$validator = new EmailValidator();
$validator->isValid("[email protected]", new RFCValidation()); //true

So, I'd suggest:

• Only handle \Swift_RfcComplianceException not \Exception
• It looks like \Swift_RfcComplianceException is only thrown on setting an address (ie a call to addRecipientHeaders()) or setting the message ID, so don't wrap the dates or body in exception handling
• The conversion errors should stored and made available to the caller somehow. To be determined how to do this (throw an exception, add an instance method, add an attachment to the converted email)

[risototh]

I don't like it too, but obviously, we had problems with the conversions and this was the solution. It wasn't only the problem with the emails, but also with the dates, and even with the body. This all is based on real experience with implementation of msg viewer and tested on client files (170k+ files). Throwing and exception in the middle of the conversion does not solve anything, as the conversion stops. Only solution that I can see is maybe adding one more parameter, that enables/disabled the throwing of the exceptions, because I don't need them, I need to show at least something, which is way better, than nothing. What about that solution?

[hfig]

I don't have a problem with it not throwing and with the conversion being lossy, but it needs to indicate to the caller that the conversion completed only partly. And, as noted, it needs to only catch the exceptions we're expecting and intentionally swallowing (\Swift_RfcComplianceException)

[risototh]

OK, I will try to rewrite it to something more useful, that will control the exceptions.

[risototh]

I have implemented the catching of the exceptions on condition. Default behaviour is like previous. The exceptions can be muted, and in that case, all exceptions will be recorded to internal array and can be requested by getter method. Is it OK now?

[risototh]

Hmm, not sure what's wrong again with the formatting... I think, that I will hate the PhpStorm...

[risototh]

OK, the formatting is fixed now. I reverted the previous commit and made the adjustments in Sublime, instead of PhpStorm...

[hfig]

This is OK mostly. Thanks for your work.

But I do have a quibble mucking with the interface by introducing an optional parameter. I think a reasonable fix would be to make the state of swallowing exceptions or not a property of the factory class. Therefore you call toMime() but the factory determines the parsing behaviour (swallow or throw). If you could:

• either modify the Hfig\MAPI\Mime\Swiftmailer\Message constructor to add the muteConversionExceptions option OR add a public setter for it.
• In Hfig\MAPI\Mime\Swiftmailer\Factory add muteConversionExceptions as a parameter to the constructor and a property of the class
• modify Hfig\MAPI\Mime\Swiftmailer\Factory:: parseMessage() to pass through muteConversionExceptions from the factory

Does that make sense/sound reasonable? You'd now write $messageFactory = new MAPI\MapiMessageFactory(new Swiftmailer\Factory(true)); to swallow conversion exceptions when calling toMime()

[risototh]

OK, I will think about it, as I need to pass it somehow to this construction: $wrappedAttachment = \Hfig\MAPI\Mime\Swiftmailer\Attachment::wrap($attachment); $content = $wrappedAttachment->toMimeString();

and this calls the Message::wrap($this->embedded_msg)->toMime() internally, so I think, that I will need to add the public method to the Attachment class, and also to the Message class, and set Message->setMuteConversionExceptions() also in the Attachment->toMime() method.

Will this be acceptable?

[hfig]

hmm it took me a while to understand what you're saying here because your PR doesn't currently include that functionality (passing the flag through so that it would apply to MSG attachments). At the moment your logic in the PR will just ignore/drop the whole attachment if it has an invalidity.

So you've expanded the scope of things a bit!

But yes you can do any of: change the Attachment default constructor, add methods to Message and Attachment classes or change the signature of their ::wrap() constructors (they aren't what I'd consider part of a public contract).

[hfig]

Merged an amended version - PR38