coreshop / CoreShop

CoreShop - Pimcore enhanced eCommerce
http://www.coreshop.org
Other
277 stars 157 forks source link

MailProcessor adds Recipient twice #2581

Open solverat opened 5 months ago

solverat commented 5 months ago
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no

Reason:

https://github.com/coreshop/CoreShop/blob/b814c70d1ef36b7f43ace5e877735617d4235464/src/CoreShop/Component/Pimcore/Mail.php#L53-L61

Which is just a duplicate of pimcores internal mail processor:

https://github.com/pimcore/pimcore/blob/3b9af529ec41cc6a9723dde96714df67b65d6078/lib/Mail.php#L378-L382

dpfaffenbauer commented 4 months ago

where does pimcore add the to again?

solverat commented 4 months ago

@dpfaffenbauer: Pimcore doesn't, CoreShop does. Because coreshop uses the $mail->setDocument(), pimcore internally calls setDocumentSettings() which applies all recipients.

After that, coreshop also adds them here:

https://github.com/coreshop/CoreShop/blob/b814c70d1ef36b7f43ace5e877735617d4235464/src/CoreShop/Component/Pimcore/Mail.php#L53-L61

Most of the time, shop mails property $to is empty, because they should be dynamically sent to the customer. But using notification rules with a defined recipient, the mail will be submitted twice.

dpfaffenbauer commented 4 months ago

So we should stop adding that then right? or at least check for duplicates?

solverat commented 4 months ago

Removing this part should be safe, I guess?

https://github.com/coreshop/CoreShop/blob/b814c70d1ef36b7f43ace5e877735617d4235464/src/CoreShop/Component/Pimcore/Mail.php#L51-L57

This part is covered in MailProcessor->setDocument() which triggers the setDocumentSettings() method.