Skip to content

Move additional dependencies from private getters to constructor - Magento_Captcha #26398

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 20, 2020
Merged

Move additional dependencies from private getters to constructor - Magento_Captcha #26398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
65 changes: 30 additions & 35 deletions app/code/Magento/Captcha/Observer/CheckContactUsFormObserver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,34 +3,41 @@
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\Captcha\Observer;

use Magento\Captcha\Helper\Data;
use Magento\Framework\App\Action\Action;
use Magento\Framework\App\ActionFlag;
use Magento\Framework\App\Response\RedirectInterface;
use Magento\Framework\Event\Observer;
use Magento\Framework\Event\ObserverInterface;
use Magento\Framework\App\Request\DataPersistorInterface;
use Magento\Framework\App\ObjectManager;
use Magento\Framework\Message\ManagerInterface;

/**
* Class CheckContactUsFormObserver
* Check captcha on contact us form submit observer.
*/
class CheckContactUsFormObserver implements ObserverInterface
{
/**
* @var \Magento\Captcha\Helper\Data
* @var Data
*/
protected $_helper;

/**
* @var \Magento\Framework\App\ActionFlag
* @var ActionFlag
*/
protected $_actionFlag;

/**
* @var \Magento\Framework\Message\ManagerInterface
* @var ManagerInterface
*/
protected $messageManager;

/**
* @var \Magento\Framework\App\Response\RedirectInterface
* @var RedirectInterface
*/
protected $redirect;

Expand All @@ -45,60 +52,48 @@ class CheckContactUsFormObserver implements ObserverInterface
private $dataPersistor;

/**
* @param \Magento\Captcha\Helper\Data $helper
* @param \Magento\Framework\App\ActionFlag $actionFlag
* @param \Magento\Framework\Message\ManagerInterface $messageManager
* @param \Magento\Framework\App\Response\RedirectInterface $redirect
* @param Data $helper
* @param ActionFlag $actionFlag
* @param ManagerInterface $messageManager
* @param RedirectInterface $redirect
* @param CaptchaStringResolver $captchaStringResolver
* @param DataPersistorInterface $dataPersistor
*/
public function __construct(
\Magento\Captcha\Helper\Data $helper,
\Magento\Framework\App\ActionFlag $actionFlag,
\Magento\Framework\Message\ManagerInterface $messageManager,
\Magento\Framework\App\Response\RedirectInterface $redirect,
CaptchaStringResolver $captchaStringResolver
Data $helper,
ActionFlag $actionFlag,
ManagerInterface $messageManager,
RedirectInterface $redirect,
CaptchaStringResolver $captchaStringResolver,
DataPersistorInterface $dataPersistor
) {
$this->_helper = $helper;
$this->_actionFlag = $actionFlag;
$this->messageManager = $messageManager;
$this->redirect = $redirect;
$this->captchaStringResolver = $captchaStringResolver;
$this->dataPersistor = $dataPersistor;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @Bartlomiejsz, thank you for your contribution!

Even if this change doesn't affect a public API, I would recommend to make it as backward compatible as possible, adding the new constructor parameter as last and optional (DataPersistorInterface $dataPersisto = null), with property initialized with object manager if the parameter is not passed.

e.g.

$this->dataPersistor = $dataPersistor ?: ObjectManager::getInstance()->get(DataPeristorInterface::class);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @aleron75, I can change it if it is required, sure, but I believe this would be great to specify final requirements, because almost each PR is processed differently :)
I.e. #26684 was accepted after modifying constructor to be not backward compatible, and in #26269 I was even asked to modify it and remove object manager usage.
What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me check and get back to you soon, thank you for pointing it out

Copy link
Contributor

Choose a reason for hiding this comment

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

Hello @Bartlomiejsz here I am; I verified and I apologize, you are right: we can accept backward-incompatible changes for non-api classes, so we could add new arguments without fallback to Object Manager.

}

/**
* Check CAPTCHA on Contact Us page
*
* @param \Magento\Framework\Event\Observer $observer
* @param Observer $observer
* @return void
*/
public function execute(\Magento\Framework\Event\Observer $observer)
public function execute(Observer $observer)
{
$formId = 'contact_us';
$captcha = $this->_helper->getCaptcha($formId);
if ($captcha->isRequired()) {
/** @var \Magento\Framework\App\Action\Action $controller */
/** @var Action $controller */
$controller = $observer->getControllerAction();
if (!$captcha->isCorrect($this->captchaStringResolver->resolve($controller->getRequest(), $formId))) {
$this->messageManager->addErrorMessage(__('Incorrect CAPTCHA.'));
$this->getDataPersistor()->set($formId, $controller->getRequest()->getPostValue());
$this->_actionFlag->set('', \Magento\Framework\App\Action\Action::FLAG_NO_DISPATCH, true);
$this->dataPersistor->set($formId, $controller->getRequest()->getPostValue());
$this->_actionFlag->set('', Action::FLAG_NO_DISPATCH, true);
$this->redirect->redirect($controller->getResponse(), 'contact/index/index');
}
}
}

/**
* Get Data Persistor
*
* @return DataPersistorInterface
*/
private function getDataPersistor()
{
if ($this->dataPersistor === null) {
$this->dataPersistor = ObjectManager::getInstance()
->get(DataPersistorInterface::class);
}

return $this->dataPersistor;
}
}
148 changes: 80 additions & 68 deletions app/code/Magento/Captcha/Test/Unit/Observer/CheckContactUsFormObserverTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,109 +5,122 @@
*/
namespace Magento\Captcha\Test\Unit\Observer;

use Magento\Captcha\Helper\Data;
use Magento\Captcha\Model\DefaultModel;
use Magento\Captcha\Observer\CaptchaStringResolver;
use Magento\Captcha\Observer\CheckContactUsFormObserver;
use Magento\Framework\App\Action\Action;
use Magento\Framework\App\ActionFlag;
use Magento\Framework\App\Request\DataPersistorInterface;
use Magento\Framework\App\Request\Http;
use Magento\Framework\App\Response\RedirectInterface;
use Magento\Framework\Event\Observer;
use Magento\Framework\Message\ManagerInterface;
use Magento\Framework\Session\SessionManager;
use Magento\Framework\TestFramework\Unit\Helper\ObjectManager;
use PHPUnit\Framework\MockObject\MockObject;
use PHPUnit\Framework\TestCase;

/**
* Test class for \Magento\Captcha\Observer\CheckContactUsFormObserver
*
* @SuppressWarnings(PHPMD.CouplingBetweenObjects)
*/
class CheckContactUsFormObserverTest extends \PHPUnit\Framework\TestCase
class CheckContactUsFormObserverTest extends TestCase
{
/**
* @var \Magento\Captcha\Observer\CheckContactUsFormObserver
* @var ObjectManager
*/
protected $checkContactUsFormObserver;
private $objectManagerHelper;

/**
* @var \Magento\Captcha\Helper\Data|\PHPUnit_Framework_MockObject_MockObject
* @var CheckContactUsFormObserver
*/
protected $helperMock;
private $checkContactUsFormObserver;

/**
* @var \Magento\Framework\App\ActionFlag|\PHPUnit_Framework_MockObject_MockObject
* @var Data|MockObject
*/
protected $actionFlagMock;
private $helperMock;

/*
* @var \Magento\Framework\Message\ManagerInterface|\PHPUnit_Framework_MockObject_MockObject
/**
* @var ActionFlag|MockObject
*/
protected $messageManagerMock;
private $actionFlagMock;

/**
* @var \Magento\Framework\App\Response\RedirectInterface|\PHPUnit_Framework_MockObject_MockObject
* @var ManagerInterface|MockObject
*/
protected $redirectMock;
private $messageManagerMock;

/**
* @var \Magento\Framework\TestFramework\Unit\Helper\ObjectManager
* @var RedirectInterface|MockObject
*/
protected $objectManagerHelper;
private $redirectMock;

/**
* @var \Magento\Captcha\Observer\CaptchaStringResolver|\PHPUnit_Framework_MockObject_MockObject
* @var CaptchaStringResolver|MockObject
*/
protected $captchaStringResolverMock;
private $captchaStringResolverMock;

/**
* @var \Magento\Framework\Session\SessionManager|\PHPUnit_Framework_MockObject_MockObject
* @var DataPersistorInterface|MockObject
*/
protected $sessionMock;
private $dataPersistorMock;

/**
* @var \Magento\Captcha\Model\DefaultModel|\PHPUnit_Framework_MockObject_MockObject
* @var SessionManager|MockObject
*/
protected $captchaMock;
private $sessionMock;

/**
* @var \Magento\Framework\App\Request\DataPersistorInterface|\PHPUnit_Framework_MockObject_MockObject
* @var DefaultModel|MockObject
*/
protected $dataPersistorMock;
private $captchaMock;

protected function setUp()
{
$this->objectManagerHelper = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this);
$this->objectManagerHelper = new ObjectManager($this);

$this->helperMock = $this->createMock(Data::class);
$this->actionFlagMock = $this->createMock(ActionFlag::class);
$this->messageManagerMock = $this->createMock(ManagerInterface::class);
$this->redirectMock = $this->createMock(RedirectInterface::class);
$this->captchaStringResolverMock = $this->createMock(CaptchaStringResolver::class);
$this->dataPersistorMock = $this->getMockBuilder(DataPersistorInterface::class)
->getMockForAbstractClass();

$this->helperMock = $this->createMock(\Magento\Captcha\Helper\Data::class);
$this->actionFlagMock = $this->createMock(\Magento\Framework\App\ActionFlag::class);
$this->messageManagerMock = $this->createMock(\Magento\Framework\Message\ManagerInterface::class);
$this->redirectMock = $this->createMock(\Magento\Framework\App\Response\RedirectInterface::class);
$this->captchaStringResolverMock = $this->createMock(\Magento\Captcha\Observer\CaptchaStringResolver::class);
$this->sessionMock = $this->createPartialMock(
\Magento\Framework\Session\SessionManager::class,
SessionManager::class,
['addErrorMessage']
);
$this->dataPersistorMock = $this->getMockBuilder(\Magento\Framework\App\Request\DataPersistorInterface::class)
->getMockForAbstractClass();
$this->captchaMock = $this->createMock(DefaultModel::class);

$this->checkContactUsFormObserver = $this->objectManagerHelper->getObject(
\Magento\Captcha\Observer\CheckContactUsFormObserver::class,
CheckContactUsFormObserver::class,
[
'helper' => $this->helperMock,
'actionFlag' => $this->actionFlagMock,
'messageManager' => $this->messageManagerMock,
'redirect' => $this->redirectMock,
'captchaStringResolver' => $this->captchaStringResolverMock
'captchaStringResolver' => $this->captchaStringResolverMock,
'dataPersistor' => $this->dataPersistorMock
]
);
$this->objectManagerHelper->setBackwardCompatibleProperty(
$this->checkContactUsFormObserver,
'dataPersistor',
$this->dataPersistorMock
);

$this->captchaMock = $this->createMock(\Magento\Captcha\Model\DefaultModel::class);
}

public function testCheckContactUsFormWhenCaptchaIsRequiredAndValid()
{
$formId = 'contact_us';
$captchaValue = 'some-value';

$controller = $this->createMock(\Magento\Framework\App\Action\Action::class);
$request = $this->createMock(\Magento\Framework\App\Request\Http::class);
$request->expects($this->any())
->method('getPost')
->with(\Magento\Captcha\Helper\Data::INPUT_NAME_FIELD_VALUE, null)
$controller = $this->createMock(Action::class);
$request = $this->createMock(Http::class);
$request->method('getPost')
->with(Data::INPUT_NAME_FIELD_VALUE, null)
->willReturn([$formId => $captchaValue]);
$controller->expects($this->any())->method('getRequest')->willReturn($request);
$this->captchaMock->expects($this->any())->method('isRequired')->willReturn(true);
$controller->method('getRequest')->willReturn($request);
$this->captchaMock->method('isRequired')->willReturn(true);
$this->captchaMock->expects($this->once())
->method('isCorrect')
->with($captchaValue)
Expand All @@ -116,13 +129,13 @@ public function testCheckContactUsFormWhenCaptchaIsRequiredAndValid()
->method('resolve')
->with($request, $formId)
->willReturn($captchaValue);
$this->helperMock->expects($this->any())
->method('getCaptcha')
->with($formId)->willReturn($this->captchaMock);
$this->helperMock->method('getCaptcha')
->with($formId)
->willReturn($this->captchaMock);
$this->sessionMock->expects($this->never())->method('addErrorMessage');

$this->checkContactUsFormObserver->execute(
new \Magento\Framework\Event\Observer(['controller_action' => $controller])
new Observer(['controller_action' => $controller])
);
}

Expand All @@ -135,11 +148,10 @@ public function testCheckContactUsFormRedirectsCustomerWithWarningMessageWhenCap
$redirectUrl = 'http://magento.com/contacts/';
$postData = ['name' => 'Some Name'];

$request = $this->createMock(\Magento\Framework\App\Request\Http::class);
$request = $this->createMock(Http::class);
$response = $this->createMock(\Magento\Framework\App\Response\Http::class);
$request->expects($this->any())
->method('getPost')
->with(\Magento\Captcha\Helper\Data::INPUT_NAME_FIELD_VALUE, null)
$request->method('getPost')
->with(Data::INPUT_NAME_FIELD_VALUE, null)
->willReturn([$formId => $captchaValue]);
$request->expects($this->once())
->method('getPostValue')
Expand All @@ -150,10 +162,10 @@ public function testCheckContactUsFormRedirectsCustomerWithWarningMessageWhenCap
->with($response, $redirectRoutePath, [])
->willReturn($redirectUrl);

$controller = $this->createMock(\Magento\Framework\App\Action\Action::class);
$controller->expects($this->any())->method('getRequest')->willReturn($request);
$controller->expects($this->any())->method('getResponse')->willReturn($response);
$this->captchaMock->expects($this->any())->method('isRequired')->willReturn(true);
$controller = $this->createMock(Action::class);
$controller->method('getRequest')->willReturn($request);
$controller->method('getResponse')->willReturn($response);
$this->captchaMock->method('isRequired')->willReturn(true);
$this->captchaMock->expects($this->once())
->method('isCorrect')
->with($captchaValue)
Expand All @@ -162,32 +174,32 @@ public function testCheckContactUsFormRedirectsCustomerWithWarningMessageWhenCap
->method('resolve')
->with($request, $formId)
->willReturn($captchaValue);
$this->helperMock->expects($this->any())
->method('getCaptcha')
$this->helperMock->method('getCaptcha')
->with($formId)
->willReturn($this->captchaMock);
$this->messageManagerMock->expects($this->once())->method('addErrorMessage')->with($warningMessage);
$this->messageManagerMock->expects($this->once())
->method('addErrorMessage')
->with($warningMessage);
$this->actionFlagMock->expects($this->once())
->method('set')
->with('', \Magento\Framework\App\Action\Action::FLAG_NO_DISPATCH, true);
->with('', Action::FLAG_NO_DISPATCH, true);
$this->dataPersistorMock->expects($this->once())
->method('set')
->with($formId, $postData);

$this->checkContactUsFormObserver->execute(
new \Magento\Framework\Event\Observer(['controller_action' => $controller])
new Observer(['controller_action' => $controller])
);
}

public function testCheckContactUsFormDoesNotCheckCaptchaWhenItIsNotRequired()
{
$this->helperMock->expects($this->any())
->method('getCaptcha')
$this->helperMock->method('getCaptcha')
->with('contact_us')
->willReturn($this->captchaMock);
$this->captchaMock->expects($this->any())->method('isRequired')->willReturn(false);
$this->captchaMock->method('isRequired')->willReturn(false);
$this->captchaMock->expects($this->never())->method('isCorrect');

$this->checkContactUsFormObserver->execute(new \Magento\Framework\Event\Observer());
$this->checkContactUsFormObserver->execute(new Observer());
}
}