[2.3] Alternative fix for Multi Store Emails issue, Fix Async Emails issues, Fix Multiple Email issues

[gwharton]

Description

This PR removes the previously introduced transportBuilderByStore class that was introduced in 2.2.4 to fix issue #11740 as this implementation caused unwanted regression/bug when sending multiple emails/async emails.

Fixed Issues (if relevant)

1. Sending emails from Admin in Multi-Store Environment defaults to Primary Store #11740:Sending emails from Admin in Multi-Store Environment defaults to Primary Store
2. Confirmation emails have no FROM or FROM email address 2.2.4 #14952:Confirmation emails have no FROM or FROM email address 2.2.4
3. Store Email Addresses are not used anymore #14945:Store Email Addresses are not used anymore
4. Magento 2.2.4 not sending from sales sender #16355:Magento 2.2.4 not sending from sales sender
5. Store emails have correct from address but do not have a from name #18470:Store emails have correct from address but do not have a from name

Manual testing scenarios

Scenareo 1

• Single store
• Setup store email addresses
• Complete checkout
• Verify from address in confirmation email

Scenareo 2

• Multiple store
• Setup store email addresses for Store 1
• Setup store email addresses for Store 2
• Complete checkout on Store 1- Verify from address in confirmation email
• Complete checkout on Store 2- Verify from address in confirmation email

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[sreichel]

Can you please reopen?

[gwharton]

Note, in testing 2.3-develop, emails will not have a from name set. This is due to bug #18470.

[ihor-sviziev]

Hi @gwharton,
Let me know once you'll add all needed changes. I'll review them all together

[gwharton]

Just waiting on travis to pass and I should be done. I know there are lots of usages of the now deprecated function within the magento codebase, but I'm not proposing to update any of them in these PRs. Seems a little beyond scope and also a testing nightmare to change all in one go. I can audit the codebase and compile a list of areas that could do with looking at in the future if that would help.

[ihor-sviziev]

Hi @gwharton,

Unfortunately this change will not be included in 2.3.0, so as part of 2.3.x release line - it's not backward change. Could you do the same changes as I requested for 2.2.x release line?

Sorry for confusing you.

Thank you

[gwharton]

@ihor-sviziev All done.

[ghost]

Hi @gwharton, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

[magento-engcom-team]

Hi @gwharton. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.

[JoostWan]

You can use cweagans/composer-patches and below patches for Magento 2.3.0

"patches": {
            "magento/module-sales": {
                "PR-18471: Alternative fix for Multi Store Emails issue, Fix Async Emails issues, Fix Multiple Email issues": "patches/composer/PR-18471-Sales.diff"
            },
            "magento/framework": {
                "PR-18471: Alternative fix for Multi Store Emails issue, Fix Async Emails issues, Fix Multiple Email issues": "patches/composer/PR-18471-Framework.diff"
            }
        }

patches.zip

[rishabhchd19]

You can use cweagans/composer-patches and below patches for Magento 2.3.0

"patches": {
            "magento/module-sales": {
                "PR-18471: Alternative fix for Multi Store Emails issue, Fix Async Emails issues, Fix Multiple Email issues": "patches/composer/PR-18471-Sales.diff"
            },
            "magento/framework": {
                "PR-18471: Alternative fix for Multi Store Emails issue, Fix Async Emails issues, Fix Multiple Email issues": "patches/composer/PR-18471-Framework.diff"
            }
        }

patches.zip

This isn't working to my system as no file is being updated when I put the zip files in folder patches/composer/ . I ran setup:di:compile & setup:static-content:deploy -f after setup:upgrade but neither it is patching up files with http://mydomain.com/patches/composer/PR-18471-Sales.diff nor with the path patches/composer/PR-18471-Sales.diff. Problem with both the patches.

[hostep]

@rishabhchd19 : you need to put the patches & composer-exit-on-patch-failure sections in your extra section in your composer.json file, make sure you read the documentation very carefully :)

After that, you can run composer install which should apply the patches. After they are applied, you need to put them in your composer.lock file as well, which you can automatically update using composer update --lock

There's also some information over here ("Apply a patch" section) which more or less tells you the same.

[rishabhchd19]

yeah...silly mistake. Got your point. Thanks @hostep

[ladle3000]

I apply this patch to 2.3 and then mageplaza smtp gives this error on test send

Zend\Mail\Transport\Smtp transport expects either a Sender or at least one From address in the Message; none provided

[ihor-sviziev]

Hi @snoroozi,
I don't think it's caused by these changes, or you applied patch incorrectly somehow.
You can create an issue for https://github.com/mageplaza/magento-2-smtp

[Lukas-G21]

Does this patch work for 2.3? Because on my 2.3 version I applied these patches correctly. On composer -v install it changed the code with diff. Ran setup:upgrade, cache:clean, setup:di:compile, setup:static:content:deploy. Still emails sender is www-data. I checked Magento 2.3-develop brach and code is different than on patch. It uses setFromByScope https://github.com/magento/magento2/blob/2.3-develop/app/code/Magento/Sales/Model/Order/Email/SenderBuilder.php Does anyone have working patch for 2.3?

[gwharton]

The change to setfrombyscope was just a wording change.

Are you using any third party email modules that could be overriding core functions?

[Lukas-G21]

None of 3rd party modules override these core functions. Also cleared these : rm -rf var/cache/* var/page_cache/* var/di/* var/generation/* var/view_preprocessed/* pub/static/* generated/code/*
and redid whole process. Still same sender www-data. I used PR-18471.zip

[gwharton]

I don't know what else to suggest.

I've just redeployed 2.3-develop and tested sending newsletter emails, sending sales email in synchronous mode, and sending sales emails in asynchronous mode. All set the correct sender details.

Either the patches aren't applied correctly, or something is overriding core where you arent expecting it.

try as a double check.

grep -r 'Sales\\Model\\Order\\Email\\SenderBuilder\|Framework\\Mail\\Message\|Framework\\Mail\\Template\\TransportBuilder' * | grep di.xml

Are you able to reproduce in a dev environment. Can you debug using xdebug? How about disabling all modules and see if issue is still there. If not reintroduce modules a few at a time. All ways in which I've tracked down unexpected behaviour in the past.

[Lukas-G21]

Heres how I applied patch:

in project directory patches/composer/ added:
PR-18471-Framework.diff
PR-18471-Sales.diff
then composer require cweagans/composer-patches and in composer.json in "extra" added:
"composer-exit-on-patch-failure": true, "patches": { "magento/module-sales": { "PR-18471: Alternative fix for Multi Store Emails issue, Fix Async Emails issues, Fix Multiple Email issues": "patches/composer/PR-18471-Sales.diff" }, "magento/framework": { "PR-18471: Alternative fix for Multi Store Emails issue, Fix Async Emails issues, Fix Multiple Email issues": "patches/composer/PR-18471-Framework.diff" } }

Then ran composer -v install and composer update --lock. Then
rm -rf var/cache/* var/page_cache/* var/di/* var/generation/* var/view_preprocessed/* pub/static/* generated/code/* and cli commands for cache clean, setup upgrade, di:compile and static content compilation.

Also none of the 3rd party modules extend patch function and
grep -r 'Sales\\Model\\Order\\Email\\SenderBuilder\|Framework\\Mail\\Message\|Framework\\Mail\\Template\\TransportBuilder' * | grep di.xml
returns
vendor/magento/module-email/etc/di.xml: <preference for="Magento\Framework\Mail\MessageInterface" type="Magento\Framework\Mail\Message" /> vendor/dotmailer/dotmailer-magento2-extension/etc/di.xml: <type name="Magento\Framework\Mail\Template\TransportBuilder"> vendor/dotmailer/dotmailer-magento2-extension/etc/di.xml: <type name="Magento\Framework\Mail\MessageInterface">
this module is turned off though.
Patch does not work locally nor on staging server on 2.3

UPDATE:
When debugging /vendor/zendframework/zend-mail/src/Transport/Sendmail::mailHandler() $result headers has correct parameters, but when email arrives it is www-data

[ladle3000]

@trenox2121 I never got this patch to work either and also correctly applied. Would love to see someone package it up more simply

[Lukas-G21]

@snoroozi, @gwharton after debugging I didn't find any problems in code. Then reconfigured sendmail locally, email headers are correct now. So this patch does work.

[lgrassini]

Magento 2.3.1 still implementing setFromByScope instead of setFromByStore in Framework/Mail/Template/TransportBuilder.php causing some other emails, like Recover Password email, not being sent.

   public function setFrom($from)
    {
        return $this->setFromByScope($from, null);
    }

See: https://github.com/magento/magento2/blob/2.3-develop/lib/internal/Magento/Framework/Mail/Template/TransportBuilder.php

@gwharton Any idea why the Magento team didn't fully implemented the proposed PR ?

[gwharton]

SetFrom and setFrombyStore are deprecated and call the framework function setFrombyScope. Anything that used setFrom or setfrombystore should still work. New code shoukd use setfrombyscope.

The initial fix of setfrombystore was renamed to setfrombyscope as framework functions should not be store aware. (Renamed via deprecation)

[gwharton]

Oh hang on. I see. The deprecated function setfrombystore has been removed. Yes i imagine this would break things!!!

[helefa]

is there a way to backport this to 2.2.4?

[ihor-sviziev]

is there a way to backport this to 2.2.4?

You can look at #18472

[LiamKarlMitchell]

Will this fix Magento 2.3.0 seemingly just exiting the script when trying to notify Invoice & Shipping emails?