#14952 Confirmation emails have no FROM or FROM email address 2.2.4

[sashas777]

Undone commit f2bfdd9 which caused issue

Description

Removed class lib/internal/Magento/Framework/Mail/Template/TransportBuilderByStore and assosiated tests for it.
The class cause the issue #14952 and was introduced because in previous releases default class lib/internal/Magento/Framework/Mail/Template/TransportBuilder didn't have the parameter $store for function public function setFromByStore($from, $store) Line 291.

Fixed Issues (if relevant)

1. 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
2. Remove var Translator = new Translate... from every page #24: Wrong sender name/email in invoice/shipment email in multistore

Manual testing scenarios

1. When Magento has single store view: set values at the admin Store->Configuration->General->Store Email Addresses-> Sales Representative. Make an order from storefront then check in the order success email "name from" and "email from" fields. It supposes to be matched to the admin configuration.

2. When Magento has single store view: set values at the admin Store->Configuration->General->Store Email Addresses-> Sales Representative. Make an order from admin panel then check in the order success email "name from" and "email from" fields. It supposes to be matched to the admin configuration.

3. When Magento has multiple store views: set different values at the admin Store->Configuration->General->Store Email Addresses-> Sales Representative.

4. Create a new order from the storefront for each store view and look at your email to confirm these values in order confirmation email.

5. Create a new order from the admin for each store view and look at your email to confirm these values in order 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)

[DanielRuf]

Undone commit f2bfdd9 which caused issue

A revert does not seem to be the right solution. There was much code added and I guess one method or some lines are just missing.

This is not how it should be fixed imho.

[sashas777]

@DanielRuf

Do you have any ideas what missing? I checked if TransportBuilder class used anywhere else and it doesn't seems to be used.
As far as I can see it was introduced because core code didn't has such method. But now this additional class (transportBuilderByStore) doesn't need anymore.

[gwharton]

I think we need to get some input from @RomaKis on what he intended to do on the original commit. Seems like it was incomplete, but was done for a reason.

[DanielRuf]

Not the right solution imho.

[DanielRuf]

Reverting the changes is not the solution here.

[sashas777]

@RomaKis
Any comments what would be right solution or why is t incorrect?

[RomaKis]

@sashas777, you can check the review comments from this PR #11992.

Correspond comment:
It is not encouraged to add new public methods in classes marked as api

[gwharton]

@RomaKis. The addition of a new class is fine, however in your implementation the new class's $message variable is never used, hence the issue #14952.

[RomaKis]

@gwharton, I can't reproduce this issue.

Steps:

1. Fresh Magento install.
2. Product created.
3. Go to frontend, create customer, add product to cart, place order.
4. Go to admin, find created order, open it and press "Send Email".
5. Email has "From" email.

Do I missed something?

[gwharton]

@RomaKis I'm not sure how your installation is working to be honest. A quick look at the code would indicate.

1. SenderBuilder's constructor creates a copy of transportBuilder and transportBuilderByStore. These are two completely separate objects. each has its own protected member $message.
2. A call is made to configureEmailTemplate() which configures the email template for the transportBuilder object and then sets the from address for the transportBuilderByStore object
3. The to address is set on the transportBuilder object
4. The message is sent using the transportBuilder object

At no point is the transportBuilderByStore object ever used, apart from having its from address set in its own protected member $message.

[RomaKis]

@gwharton and what is $message? it is object \Magento\Framework\Mail\Message injected by ObjectManager. And ObjectManager use get method. So when we inject $message in TransportBuilderByStore we will get the same object from the transportBuilder and set to it "from" email.

[gwharton]

I don't follow. The ObjectManager's get() call in SenderBuilders constructor ensures that multiple instances of the SenderBuilder object all have the same transportBuilderByStore object, but does nothing to link it to the transportBuilder object. Please explain further.

[RomaKis]

@gwharton, we have $message object \Magento\Framework\Mail\Message in TransportBuilder. And we have the same object in TransportBuilderByStore. When we add to constructor \Magento\Framework\Mail\Message $message object manager use get() method, so it doesn't create new object of \Magento\Framework\Mail\Message it return existing.

In TransportBuilderByStore we inject \Magento\Framework\Mail\Message $message and add to it setFrom(), so object $message from TransportBuilder will have "From" email while sending it.

[gwharton]

Whereabouts?

magento2/lib/internal/Magento/Framework/Mail/Template/TransportBuilder.php

Lines 96 to 108 in 32892a4

	public function __construct(
	FactoryInterface $templateFactory,
	MessageInterface $message,
	SenderResolverInterface $senderResolver,
	ObjectManagerInterface $objectManager,
	TransportInterfaceFactory $mailTransportFactory
	) {
	$this->templateFactory = $templateFactory;
	$this->message = $message;
	$this->objectManager = $objectManager;
	$this->_senderResolver = $senderResolver;
	$this->mailTransportFactory = $mailTransportFactory;
	}

magento2/lib/internal/Magento/Framework/Mail/Template/TransportBuilderByStore.php

Lines 31 to 37 in 32892a4

	public function __construct(
	MessageInterface $message,
	SenderResolverInterface $senderResolver
	) {
	$this->message = $message;
	$this->senderResolver = $senderResolver;
	}

magento2/app/code/Magento/Sales/Model/Order/Email/SenderBuilder.php

Lines 42 to 54 in 32892a4

	public function __construct(
	Template $templateContainer,
	IdentityInterface $identityContainer,
	TransportBuilder $transportBuilder,
	TransportBuilderByStore $transportBuilderByStore = null
	) {
	$this->templateContainer = $templateContainer;
	$this->identityContainer = $identityContainer;
	$this->transportBuilder = $transportBuilder;
	$this->transportBuilderByStore = $transportBuilderByStore ?: ObjectManager::getInstance()->get(
	TransportBuilderByStore::class
	);
	}

[RomaKis]

If you will stop here

https://github.com/magento/magento2/blob/32892a46b2682bc7745ca26a9fe3fe8e55e915d2/app/code/Magento/Sales/Model/Order/Email/SenderBuilder.php#L70

you will see that $this->transportBuilder has object $message with all necessary information.
See the screenshots above https://github.com/magento/magento2/pull/15119#issuecomment-390631010

[EmilyPepperman]

Besides the "From"... looks like "Return Path" missing from emails now?

[phoenix128]

I would suggest to avoid adding new public methods when not necessary. Every public method is a kind of contract for backward compatibility.

Changing setFrom and adding an optional parameter could be a better option.

See https://github.com/magento/magento2/pull/16356/files trying to fix the same issue.

[Monalisa222]

I am facing the same issue , when a customer placed an order then customer is getting the mail but not admin.. And the second thing is that it is the taking the different host as my id is socketswitches@socket-switches.com but system is sending from socketswitches@ssdvps-80478.vps.net  . ssdvps-80478.vps.net this is the server ..Any help!!!

[Monalisa222]

I had tried this code but isn't working.

[gopal2k6]

I am also facing same issue.. can you help in this ? My issue in 2.3

[phoenix128]

FYI: Adding a corss reference with another PR trying to fix the same issue: #16356

[gwharton]

I am going to close this PR. There have been several PRs produced that are all trying to fix the same issue. I have collated everyones opinion and generated a new PR with full test coverage, Travis pass and Codacy pass. If you would like to collaborate further on this issue, you are encouraged to do so through PR #16461