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.3 contact form not sending emails( No log entry). #32730

Merged
merged 1 commit into from
Apr 22, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions app/code/Magento/Email/Model/Transport.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,15 @@
namespace Magento\Email\Model;

use Magento\Framework\App\Config\ScopeConfigInterface;
use Magento\Framework\App\ObjectManager;
use Magento\Framework\Exception\MailException;
use Magento\Framework\Mail\MessageInterface;
use Magento\Framework\Mail\TransportInterface;
use Magento\Framework\Phrase;
use Magento\Store\Model\ScopeInterface;
use Laminas\Mail\Message;
use Laminas\Mail\Transport\Sendmail;
use Psr\Log\LoggerInterface;

/**
* Class that responsible for filling some message data before transporting it.
Expand Down Expand Up @@ -60,15 +62,22 @@ class Transport implements TransportInterface
*/
private $message;

/**
* @var LoggerInterface|null
*/
private $logger;

/**
* @param MessageInterface $message Email message object
* @param ScopeConfigInterface $scopeConfig Core store config
* @param null|string|array|\Traversable $parameters Config options for sendmail parameters
* @param LoggerInterface|null $logger
*/
public function __construct(
MessageInterface $message,
ScopeConfigInterface $scopeConfig,
$parameters = null
$parameters = null,
LoggerInterface $logger = null
) {
$this->isSetReturnPath = (int) $scopeConfig->getValue(
self::XML_PATH_SENDING_SET_RETURN_PATH,
Expand All @@ -81,6 +90,7 @@ public function __construct(

$this->laminasTransport = new Sendmail($parameters);
$this->message = $message;
$this->logger = $logger ?: ObjectManager::getInstance()->get(LoggerInterface::class);
}

/**
Expand All @@ -100,7 +110,8 @@ public function sendMessage()

$this->laminasTransport->send($laminasMessage);
} catch (\Exception $e) {
throw new MailException(new Phrase($e->getMessage()), $e);
$this->logger->error($e);
throw new MailException(new Phrase('Unable to send mail. Please try again later.'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@engcom-Kilo @zakdma, we have regression here. The original exception should be passed as a 2nd param here to understand why it failed, as it was in the past.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI here is a PR that fixes this regression #33419

}
}

Expand Down
72 changes: 72 additions & 0 deletions app/code/Magento/Email/Test/Unit/Model/TransportTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\Email\Test\Unit\Model;

use Laminas\Mail\Transport\Exception\RuntimeException;
use Magento\Email\Model\Transport;
use Magento\Framework\App\Config\ScopeConfigInterface;
use Magento\Framework\Mail\Message;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use Psr\Log\LoggerInterface;

/**
* Tests for email transport functionality.
*/
class TransportTest extends TestCase
{
/**
* @var MockObject|LoggerInterface
*/
private $loggerMock;

/**
* @var Transport
*/
private $transport;

/**
* @var ScopeConfigInterface|MockObject
*/
private $scopeConfigMock;

/**
* @inheridoc
*/
protected function setUp(): void
{
$this->loggerMock = $this->getMockBuilder(LoggerInterface::class)
->disableOriginalConstructor()
->onlyMethods(['error'])
->getMockForAbstractClass();
$this->scopeConfigMock = $this->getMockBuilder(ScopeConfigInterface::class)
->disableOriginalConstructor()
->getMockForAbstractClass();
$this->transport = new Transport(
new Message(),
$this->scopeConfigMock,
null,
$this->loggerMock
);
}

/**
* Verify exception is properly handled in case one occurred when message sent.
*
* @return void
*/
public function testSendMessageBrokenMessage(): void
{
$exception = new RuntimeException('Invalid email; contains no at least one of "To", "Cc", and "Bcc" header');
$this->loggerMock->expects(self::once())->method('error')->with($exception);
$this->expectException('Magento\Framework\Exception\MailException');
$this->expectExceptionMessage('Unable to send mail. Please try again later.');

$this->transport->sendMessage();
}
}
1 change: 1 addition & 0 deletions app/code/Magento/Email/i18n/en_US.csv
Original file line number Diff line number Diff line change
Expand Up @@ -98,3 +98,4 @@ Action,Action
"Email template chosen based on theme fallback, when the ""Default"" option is selected.","Email template chosen based on theme fallback, when the ""Default"" option is selected."
"Header Template","Header Template"
"Footer Template","Footer Template"
"Unable to send mail. Please try again later.","Unable to send mail. Please try again later."
46 changes: 40 additions & 6 deletions lib/internal/Magento/Framework/Mail/Test/Unit/TransportTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,57 @@

namespace Magento\Framework\Mail\Test\Unit;

use Laminas\Mail\Transport\Exception\RuntimeException;
use Magento\Framework\Mail\Message;
use Magento\Framework\Mail\Transport;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;
use Psr\Log\LoggerInterface;

/**
* Provides tests for framework email transport functionality.
*/
class TransportTest extends TestCase
{
/**
* @var MockObject|LoggerInterface
*/
private $loggerMock;

/**
* @var Transport
*/
private $transport;

/**
* @inheridoc
*/
protected function setUp(): void
{
$this->loggerMock = $this->getMockBuilder(LoggerInterface::class)
->disableOriginalConstructor()
->onlyMethods(['error'])
->getMockForAbstractClass();
$this->transport = new Transport(
new Message(),
null,
$this->loggerMock
);
}

/**
* Verify exception is properly handled in case one occurred when message sent.
*
* @covers \Magento\Framework\Mail\Transport::sendMessage
* @return void
*/
public function testSendMessageBrokenMessage()
public function testSendMessageBrokenMessage(): void
{
$exception = new RuntimeException('Invalid email; contains no at least one of "To", "Cc", and "Bcc" header');
$this->loggerMock->expects(self::once())->method('error')->with($exception);
$this->expectException('Magento\Framework\Exception\MailException');
$this->expectExceptionMessage('Invalid email; contains no at least one of "To", "Cc", and "Bcc" header');
$transport = new Transport(
new Message()
);
$this->expectExceptionMessage('Unable to send mail. Please try again later.');

$transport->sendMessage();
$this->transport->sendMessage();
}
}
18 changes: 15 additions & 3 deletions lib/internal/Magento/Framework/Mail/Transport.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,21 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\Framework\Mail;

use Magento\Framework\App\ObjectManager;
use Magento\Framework\Exception\MailException;
use Magento\Framework\Phrase;
use Laminas\Mail\Message as LaminasMessage;
use Laminas\Mail\Transport\Sendmail;
use Psr\Log\LoggerInterface;

/**
* Mail transport
*/
class Transport implements \Magento\Framework\Mail\TransportInterface
class Transport implements TransportInterface
{
/**
* @var Sendmail
Expand All @@ -25,14 +29,21 @@ class Transport implements \Magento\Framework\Mail\TransportInterface
*/
private $message;

/**
* @var LoggerInterface|null
*/
private $logger;

/**
* @param MessageInterface $message
* @param null|string|array|\Traversable $parameters
* @param LoggerInterface|null $logger
*/
public function __construct(MessageInterface $message, $parameters = null)
public function __construct(MessageInterface $message, $parameters = null, LoggerInterface $logger = null)
{
$this->laminasTransport = new Sendmail($parameters);
$this->message = $message;
$this->logger = $logger ?: ObjectManager::getInstance()->get(LoggerInterface::class);
}

/**
Expand All @@ -45,7 +56,8 @@ public function sendMessage()
LaminasMessage::fromString($this->message->getRawMessage())
);
} catch (\Exception $e) {
throw new MailException(new Phrase($e->getMessage()), $e);
$this->logger->error($e);
throw new MailException(new Phrase('Unable to send mail. Please try again later.'));
}
}

Expand Down