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

Catch all Swift exceptions in Mail::Send() #8763

Merged
merged 2 commits into from Sep 24, 2018

Conversation

Projects
None yet
8 participants
@axometeam
Contributor

axometeam commented Feb 15, 2018

Questions Answers
Branch? 1.6.1.x
Description? Catch all Swift exceptions in Mail::Send() instead of throwing a Fatal Error
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/PSCSX-8913
How to test? The forge ticket is full of details. But you can subscribe to mailalert with a wrong email (e.g.: foo.@bar.com) and create orders with the same product, which will trigger the hook "actionUpdateQuantity", then mailalerts module, then a 'Fatal error : Uncaught exception 'Swift_RfcComplianceException''

This change is Reviewable

@axometeam

This comment has been minimized.

Show comment
Hide comment
@axometeam

axometeam Feb 15, 2018

Contributor

1.7 seems to have the same issue: http://forge.prestashop.com/browse/BOOM-4806

Contributor

axometeam commented Feb 15, 2018

1.7 seems to have the same issue: http://forge.prestashop.com/browse/BOOM-4806

@kpodemski

This comment has been minimized.

Show comment
Hide comment
@kpodemski

kpodemski Feb 15, 2018

Contributor

btw. to someone who will review this, maybe we could create some Wrapper for this function? It's really annoying to use such a long function calls, a while ago i've created some super simple one, code here:

https://gist.github.com/kpodemski/498e1714e3188049932d8a013391d822

Contributor

kpodemski commented Feb 15, 2018

btw. to someone who will review this, maybe we could create some Wrapper for this function? It's really annoying to use such a long function calls, a while ago i've created some super simple one, code here:

https://gist.github.com/kpodemski/498e1714e3188049932d8a013391d822

@nsorosac

This comment has been minimized.

Show comment
Hide comment
@nsorosac

nsorosac Mar 6, 2018

Contributor

UP please :-(

This can be merged in 1.6.1 and 1.7, do you need more info?

Contributor

nsorosac commented Mar 6, 2018

UP please :-(

This can be merged in 1.6.1 and 1.7, do you need more info?

Allow duplicate logs for exceptions in Mail::Send
It's easier to debug Mail deliverability issues when the logs show real-time info.

The use case is "Sending an email > not receiving > going to the logs": if you already encountered the exception once, you won't get a log again, so you won't know if there is a problem or not.
@axometeam

This comment has been minimized.

Show comment
Hide comment
@axometeam

axometeam May 9, 2018

Contributor

Please 😢

Contributor

axometeam commented May 9, 2018

Please 😢

Tools::dieOrLog(Tools::displayError('Error: invalid e-mail address'), $die);
return false;
}
try {

This comment has been minimized.

@PierreRambaud

PierreRambaud May 17, 2018

Contributor

Useless line break :)

@PierreRambaud

PierreRambaud May 17, 2018

Contributor

Useless line break :)

@firespeed

This comment has been minimized.

Show comment
Hide comment
@firespeed

firespeed Aug 7, 2018

UP please !

firespeed commented Aug 7, 2018

UP please !

@eternoendless eternoendless added this to the 1.6.1.21 milestone Sep 3, 2018

@ntiepresta ntiepresta self-assigned this Sep 3, 2018

@ntiepresta ntiepresta removed their assignment Sep 17, 2018

@ntiepresta ntiepresta added QA ✔️ and removed waiting for QA labels Sep 21, 2018

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless
Member

eternoendless commented Sep 24, 2018

Thank you @axometeam

@eternoendless eternoendless merged commit 95e9cfc into PrestaShop:1.6.1.x Sep 24, 2018

1 of 2 checks passed

Codacy/PR Quality Review Codacy was unable to analyse your pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment