Ensure there are no emails without recipient in email queue

[lemundo-team]

Additional check of email recipients in order to ensure there are no invalid emails without recipient in queue

Description (*)

It appeared several times that there are emails without recipients in the emails queue - even if the email address is mandatory for checkout. This pull request is not fixing the origin issue but it ensures that this one wrongly added email is not blocking all queued emails which is leading to unsatisfied customers.
Consider it more as a security improvement to keep the shop running.

Manual testing scenarios (*)

There is no known test scenario but it is solving issues that happens once every 2000 orders.

Contribution checklist (*)

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All automated tests passed successfully (all builds are green)

[kiatng]

LGTM

[colinmollenhour]

Why move the unset?

[kkrieger85]

I think the unset comes from line 245 https://github.com/OpenMage/magento-lts/pull/1109/files#diff-e408845a99067b7da8df6e1c971d74dd81e8d02f15b4b6111ce1d4389ae45e49L245

[kkrieger85]

I think the unset comes from line 245 https://github.com/OpenMage/magento-lts/pull/1109/files#diff-e408845a99067b7da8df6e1c971d74dd81e8d02f15b4b6111ce1d4389ae45e49L245

[github-actions]

Unit Test Results

1 files  1 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0 ❌
7 runs  5 ✔️ 2 💤 0 ❌

Results for commit e286008.

[luigifab]

The move of the unset is wrong no?

magento-lts/app/code/core/Mage/Core/Model/Email/Queue.php

Lines 239 to 247 in 3005c60

	try {
	$mailer->send();
	unset($mailer);
	$message->setProcessedAt(Varien_Date::formatDate(true));
	$message->save(); // save() is throwing exception when recipient is not set
	} catch (Exception $e) {
	Mage::logException($e);
	}

On exception, variable is not unset.

[addison74]

@luigifab - the author can tell us why he moved in exception. Maybe it was better to leave it where it was before.

[colinmollenhour]

Agreed with the change overall. Not sure if the location of the unset matters much, the PHP garbage collector should be able to clean it up either way I think.

[leissbua]

i think the whole solution does not work nor is well tested - remove an recipient and the queue processing is brought to an end:

---EXCEPTION---
Mage_Core_Exception: Message recipients data must be set. in /home/cloudpanel/htdocs/www.domain.com/app/Mage.php:656
Stack trace:
#0 /home/cloudpanel/htdocs/www.domain.com/app/code/core/Mage/Core/Model/Email/Queue.php(99): Mage::throwException('Message recipie...')
#1 /home/cloudpanel/htdocs/www.domain.com/app/code/core/Mage/Core/Model/Abstract.php(329): Mage_Core_Model_Email_Queue->_beforeSave()

This was happening first time on a store running since 10 years with 1k plus oders a day. We never had problems until today.

We should better quality test code changes, because i have the feeling quality is going down.

[kiatng]

@leissbua Sorry for the problem you face. How do you remove a recipient? Was it the first recipient that was removed? Does it throw the exception when the second or third recipient is removed?