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

Magento 2.2.4 not sending from sales sender #16355

Closed
versdivers opened this issue Jun 23, 2018 · 19 comments
Closed

Magento 2.2.4 not sending from sales sender #16355

versdivers opened this issue Jun 23, 2018 · 19 comments
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed

Comments

@versdivers
Copy link

versdivers commented Jun 23, 2018

Preconditions

  1. Magento 2.2.4
  2. PhP 7.1
  3. Apache

Steps to reproduce

  1. Set general store email addresses
  2. Set store specific email senders in Sales Emails
  3. Send order/shipment/creditnota/invoice mail to customer

Expected result

Email is sent from store email address

Actual result

It sends from the mail server default mailadress

Why

In vendor \magento\module-sales\Model\Order\Email\SenderBuilder.php the configureEmailTemplate() function as been changed.

You can see that
$this->transportBuilder->setFrom($this->identityContainer->getEmailIdentity());
has been changed to

        $this->transportBuilderByStore->setFromByStore(
            $this->identityContainer->getEmailIdentity(),
            $this->identityContainer->getStore()->getId()
        );

With further inspection it on first glance it returns the same result .

Eventually the from gets formatted within the _formatAddress function within Zend/Mail.php and used within the same class at setFrom function which has
$this->_storeHeader('From', $this->_formatAddress($email, $name), true);

The 'From' header can have something like Test Mail test@mail.com or just test@mail.com. But the function itself returns just Test Mail with no email.

And that is because the sprintf function appearently seems to have issues with <> signs. Note sure why.

So i changed

            $encodedName = $this->_encodeHeader($name);
            if ($encodedName === $name  &&  strcspn($name, '()<>[]:;@\\,.') != strlen($name)) {
                $format = '"%s" <%s>';
            } else {
                $format = '%s <%s>';
            }
            return sprintf($format, $encodedName, $email);

to

            $encodedName = $this->_encodeHeader($name);
            if ($encodedName === $name  &&  strcspn($name, '()<>[]:;@\\,.') != strlen($name)) {
                $format = '"%1$s" %3$s%2$s%4$s';
            } else {
                $format = '%1$s %3$s%2$s%4$s';
            }
            return sprintf($format, $encodedName, $email , '<' , '>');

and now it does return Test Mail test@mail.com. So that is fixed;

And now i am stuck.

this->transportBuilder->setFrom($this->identityContainer->getEmailIdentity());

-->sends what i need.

      $this->transportBuilderByStore->setFromByStore(
            $this->identityContainer->getEmailIdentity(),
            $this->identityContainer->getStore()->getId()
        );

-->sends from my SMTP servers default emailaddress.

It seems like the headers are reset somewhere.

@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Jun 23, 2018
@magento-engcom-team
Copy link
Contributor

magento-engcom-team commented Jun 23, 2018

Hi @versdivers. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento-engcom-team give me {$VERSION} instance

where {$VERSION} is version tags (starting from 2.2.0+) or develop branches (2.2-develop +).
For more details, please, review the Magento Contributor Assistant documentation.

@versdivers do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?

  • yes
  • no

@versdivers
Copy link
Author

versdivers commented Jun 23, 2018

@magento-engcom-team yes

@versdivers
Copy link
Author

It seems that TransportBuilder and transportBuilderByStore work in different instances of the messageinterface

@versdivers
Copy link
Author

i also do not get why transportBuilderByStore as been created. I see this just as bad coding tbh. We are back to creating new classes for simple adjustments.

Fix:
vendor/magento/module-sales/Model/Order/Email/SenderBuilder.php

replace

        $this->transportBuilderByStore->setFromByStore(
            $this->identityContainer->getEmailIdentity(),
            $this->identityContainer->getStore()->getId()
        );

with

$this->transportBuilder->setFrom($this->identityContainer->getEmailIdentity(), $this->identityContainer->getStore()->getId());
vendor/magento/framework/Mail/Template/TransportBuilder.php
replace

    /**
     * Set mail from address
     *
     * @param string|array $from
     * @return $this
     */
    public function setFrom($from)
    {
        $result = $this->_senderResolver->resolve($from);
        $this->message->setFrom($result['email'], $result['name']);
        return $this;
    }

with

    /**
     * Set mail from address
     *
     * @param string|array $from
     * @return $this
     */
    public function setFrom($from, $store = null)
    {
        $result = $this->_senderResolver->resolve($from, $store);
        $this->message->setFrom($result['email'], $result['name']);
        return $this;
    }

and just like that we do not need the TransportBuilderByStore anymore and it works as expected.

versdivers added a commit to versdivers/magento2 that referenced this issue Jun 23, 2018
@kingyond
Copy link

kingyond commented Jun 23, 2018

@versdivers this issue same as #16231.
The bug cause by amazon payment module. It disable Magento\Framework\Mail\MessageInterface as singleton object. And the make TransportBuilderByStore create a new MessageInterface which different from the on create by TransportBuilder.
open this file
vendor/amzn/amazon-pay-and-login-magento-2-module/src/Payment/etc/di.xml
find this line #63
<type name="Magento\Framework\Mail\MessageInterface" shared="false" />
remove the shared string
<type name="Magento\Framework\Mail\MessageInterface" />
save it.
after that, remove generated data and compile the di again. this should fix the bug.

@gwharton
Copy link
Contributor

@versdivers Your comment above should have been the right solution in my opinion. But was turned down on review as it meant making a change to a class marked as an API.

Instead we have the bodged solution that is currently in place, which still has bugs, e.g you can no longer send multiple emails at the same time, only the first will succeed, the rest will exception and fail.

@versdivers
Copy link
Author

I will keep my solution in place. On update i hope Magento found a way to remove all the bugs with this but for those who run into this problem i can comfirm that this solution is working great on a live website with multistore.

@gwharton
Copy link
Contributor

Duplicate of #14952

@rishabhchd19
Copy link

Resolved Problem Disabling AMAZON PAYMENT.

@mjoraid
Copy link

mjoraid commented Jul 9, 2018

1- I agree with others. Disabling Amazon Payment does fix the issue. I disabled payment, login, and core just for the sake of it and then enabled them again.
2- +kingyond solutions did fix the issue for me. Now order email is sent From as the I specified in my Admin - Sales - Sales Emails

vendor/amzn/amazon-pay-and-login-magento-2-module/src/Payment/etc/di.xml
find this line #63
<type name="Magento\Framework\Mail\MessageInterface" shared="false" />
remove the shared string
<type name="Magento\Framework\Mail\MessageInterface" />
save it.

Now I'm not sure what are the implications of removing

share="false"

@ihor-sviziev
Copy link
Contributor

Related issue: #14952

@magento-engcom-team magento-engcom-team added the Fixed in 2.3.x The issue has been fixed in 2.3 release line label Jan 7, 2019
@pratikhmehta
Copy link
Contributor

i also do not get why transportBuilderByStore as been created. I see this just as bad coding tbh. We are back to creating new classes for simple adjustments.

Fix:
vendor/magento/module-sales/Model/Order/Email/SenderBuilder.php

replace

        $this->transportBuilderByStore->setFromByStore(
            $this->identityContainer->getEmailIdentity(),
            $this->identityContainer->getStore()->getId()
        );

with

$this->transportBuilder->setFrom($this->identityContainer->getEmailIdentity(), $this->identityContainer->getStore()->getId());
vendor/magento/framework/Mail/Template/TransportBuilder.php
replace

    /**
     * Set mail from address
     *
     * @param string|array $from
     * @return $this
     */
    public function setFrom($from)
    {
        $result = $this->_senderResolver->resolve($from);
        $this->message->setFrom($result['email'], $result['name']);
        return $this;
    }

with

    /**
     * Set mail from address
     *
     * @param string|array $from
     * @return $this
     */
    public function setFrom($from, $store = null)
    {
        $result = $this->_senderResolver->resolve($from, $store);
        $this->message->setFrom($result['email'], $result['name']);
        return $this;
    }

and just like that we do not need the TransportBuilderByStore anymore and it works as expected.

Thank You so much.
Also Working in Magento 2.2.7.

@gwharton
Copy link
Contributor

gwharton commented Jan 9, 2019

@pratikhmehta totally agree. The transportbuilderbystore class has already been deprecated in 2.3 and soon will be in 2.2.

@pratikhmehta
Copy link
Contributor

pratikhmehta commented Jan 9, 2019

@pratikhmehta totally agree. The transportbuilderbystore class has already been deprecated in 2.3 and soon will be in 2.2.

But early I used Magento 2.2.4 that time i did not get any issue like this. After upgrade version Magento 2.2.7 Issue is coming.
But after used solution above solved issue.

@gwharton
Copy link
Contributor

gwharton commented Jan 9, 2019

Yes, the problem was introduced to fix a separate issue in 2.2.5, however the fix had unexpected consequences, including this one.

You will see in PR #18472 that a fix very much along the lines of your comments above, is going to be merged very soon into 2.2 and hopefully this will be available in time for release 2.2.8.

Like I said, the fix has already been applied to 2.3 in PR #18471

@magento-engcom-team magento-engcom-team added the Fixed in 2.2.x The issue has been fixed in 2.2 release line label Mar 5, 2019
@CompactCodeEU
Copy link

@magento-engcom-team You already said that this would be fixed in 2.3. I do not think that there is an upcoming 2.2.9 release anymore. That version has already been released. Do you mean there is a patch for 2.2.9? We still changed it in 2.3.0 though for our customers since the fix was not implemented.

@ihor-sviziev
Copy link
Contributor

Hi @CompactCodeEU,
Releases in 2.3.x and 2.2.x lines are going together, so this fix should be included in upcoming 2.3.1 and 2.2.9 releases, as @magento-engcom-team said

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in 2.2.x The issue has been fixed in 2.2 release line Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed
Projects
None yet
Development

No branches or pull requests

8 participants