Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Newsletter] Improved error handling while sending newsletter #4349

Merged
merged 5 commits into from May 20, 2019
Merged

[Newsletter] Improved error handling while sending newsletter #4349

merged 5 commits into from May 20, 2019

Conversation

AlexanderWeckmer
Copy link
Contributor

If the domain of a recipient is misspelled or not existing, the existing process causes:

Expected response code 354 but got code "554", with message "554 5.5.0 No valid recipients have been specified

Improvement:
First Step: Avoiding any email domains with no MX entries.
Second Step: If any error occurs cancel the jamming smtp-process via $mailer->getTransport()->stop();

  • Changed Logger:info to Logge r:warn otherwise no output in the “newsletter-sending-output.log”
  • Added counter to see the exact progress of the sending process in the .log file, helps later in finding any further errors
  • Added ReturnPath for hard bounces

Log will show the exact progress of the send process
* Checks if domain of email-address is valid.
* Adds return path for hard-bounces
If error while sending occurs, swift transportation will be stopped and kills the jamed smtp-process.
$data['progress'] = round($offset / $totalCount * 100, 2);
$tmpStore->setData($data);
$tmpStore->update();

$sendingParamContainers = $addressAdapter->getParamsForSingleSending($limit, $offset);
foreach ($sendingParamContainers as $sendingParamContainer) {
Logger::warn('Sending newsletter ' . $index . ' / ' . $totalCount. ' [' . $document->getId(). ']');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why severity warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm super bussy this week.
The issue with the warning is, that an Logger::info isn't logging to the newsletter-sending-output.log.
But it is essential for error tracking, especially related to customers, that you know exactly which customers got the newsletter and which ones not. But maybe I'm missing the point how to partial change the logging to info-Level..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for clarifying!
Please add a comment there explaining why we're using warning so that it doesn't get changed in the future.

lib/Mail.php Outdated
$mailer->send($this, $emailaddress);
} catch (\Exception $e){
$mailer->getTransport()->stop();
sleep(10);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why sleep()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make sure the transportation connection is really closed, otherwise we will run in occupied errors, Sleep() happen only in an error case. But I read a bit more about swiftmailer in the meantime, maybe the is a better solution, but this have to wait for the weekend.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks, then I'll wait until you come back on this topic.

@brusch brusch self-assigned this May 13, 2019
@brusch
Copy link
Member

brusch commented May 15, 2019

@Lexipowder ping 😊

} catch (\Exception $e){
$mailer->getTransport()->stop();
throw new \Exception($emailaddress[0].' - '.$e->getMessage());
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work well, without sleep(), according to swiftmailer docs:

/**
* Check if this Transport mechanism is alive.
*
* If a Transport mechanism session is no longer functional, the method
* returns FALSE. It is the responsibility of the developer to handle this
* case and restart the Transport mechanism manually.
*
* @example
*
* if (!$transport->ping()) {
* $transport->stop();
* $transport->start();
* }
*
* The Transport mechanism will be started, if it is not already.
*
* It is undefined if the Transport mechanism attempts to restart as long as
* the return value reflects whether the mechanism is now functional.
*
* @return bool TRUE if the transport is alive
*/

@brusch brusch changed the title Improves error handling while sending newsletter [Newsletter] Improved error handling while sending newsletter May 20, 2019
@brusch brusch added this to the 6.0.0 ("Ale") milestone May 20, 2019
@brusch brusch merged commit af9c669 into pimcore:master May 20, 2019
@brusch
Copy link
Member

brusch commented May 20, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants