Skip to content

Fixed condition with usage "hack" isPostRequest method #12626

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
Jun 7, 2018
Merged

Fixed condition with usage "hack" isPostRequest method #12626

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
13 changes: 1 addition & 12 deletions app/code/Magento/Contact/Controller/Index/Post.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
use Magento\Framework\App\Request\DataPersistorInterface;
use Magento\Framework\Controller\Result\Redirect;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\HTTP\PhpEnvironment\Request;
use Psr\Log\LoggerInterface;
use Magento\Framework\App\ObjectManager;
use Magento\Framework\DataObject;
Expand Down Expand Up @@ -67,7 +66,7 @@ public function __construct(
*/
public function execute()
{
if (!$this->isPostRequest()) {
if (!$this->getRequest()->isPost()) {
return $this->resultRedirectFactory->create()->setPath('*/*/');
}
try {
Expand Down Expand Up @@ -101,16 +100,6 @@ private function sendEmail($post)
);
}

/**
* @return bool
*/
private function isPostRequest()
{
/** @var Request $request */
$request = $this->getRequest();
return !empty($request->getPostValue());
}

/**
* @return array
* @throws \Exception
Expand Down
6 changes: 5 additions & 1 deletion app/code/Magento/Contact/Test/Unit/Controller/Index/PostTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ protected function setUp()
$this->createMock(\Magento\Framework\Message\ManagerInterface::class);
$this->requestStub = $this->createPartialMock(
\Magento\Framework\App\Request\Http::class,
['getPostValue', 'getParams', 'getParam']
['getPostValue', 'getParams', 'getParam', 'isPost']
);
$this->redirectResultMock = $this->createMock(\Magento\Framework\Controller\Result\Redirect::class);
$this->redirectResultMock->method('setPath')->willReturnSelf();
Expand Down Expand Up @@ -174,6 +174,10 @@ public function testExecuteValidPost()
*/
private function stubRequestPostData($post)
{
$this->requestStub
->expects($this->once())
->method('isPost')
->willReturn(!empty($post));
$this->requestStub->method('getPostValue')->willReturn($post);
$this->requestStub->method('getParams')->willReturn($post);
$this->requestStub->method('getParam')->willReturnCallback(
Expand Down
4 changes: 4 additions & 0 deletions dev/tests/integration/testsuite/Magento/Contact/Controller/IndexTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
*/
namespace Magento\Contact\Controller;

use Zend\Http\Request;

/**
* Contact index controller test
*/
Expand All @@ -19,6 +21,7 @@ public function testPostAction()
'hideit' => '',
];
$this->getRequest()->setPostValue($params);
$this->getRequest()->setMethod(Request::METHOD_POST);

$this->dispatch('contact/index/post');
$this->assertRedirect($this->stringContains('contact/index'));
Expand All @@ -38,6 +41,7 @@ public function testPostAction()
public function testInvalidPostAction($params, $expectedMessage)
{
$this->getRequest()->setPostValue($params);
$this->getRequest()->setMethod(Request::METHOD_POST);

$this->dispatch('contact/index/post');
$this->assertRedirect($this->stringContains('contact/index'));
Expand Down
52 changes: 52 additions & 0 deletions lib/internal/Magento/Framework/App/HttpRequestInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php
Copy link
Member

@osrecio osrecio Dec 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

What is the purpose of this Interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This interface extends the basic interface and contains methods for determining which HTTP request used. I did not touch the original interface to not do BC break. But if we can move this "contracts" to original Request interface(https://github.com/magento/magento2/blob/56af1e73ce21867b770a7458ab6e109f4a1eface/lib/internal/Magento/Framework/App/RequestInterface.php) it is even better. Because then it will not be necessary to make checks for implementation or special annotations like "$request instance of HttpRequestInterface" or "/** @var $request HttpRequestInterface $request */"

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! In this case we must create new Interface to not do BC break as you say!

/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Framework\App;

interface HttpRequestInterface
{
/**
* Returned true if POST request
*
* @return boolean
*/
public function isPost();

/**
* Returned true if GET request
*
* @return boolean
*/
public function isGet();

/**
* Returned true if PATCH request
*
* @return boolean
*/
public function isPatch();

/**
* Returned true if DELETE request
*
* @return boolean
*/
public function isDelete();

/**
* Returned true if PUT request
*
* @return boolean
*/
public function isPut();

/**
* Returned true if Ajax request
*
* @return boolean
*/
public function isAjax();
}
3 changes: 2 additions & 1 deletion lib/internal/Magento/Framework/App/Request/Http.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
namespace Magento\Framework\App\Request;

use Magento\Framework\App\HttpRequestInterface;
use Magento\Framework\App\RequestContentInterface;
use Magento\Framework\App\RequestSafetyInterface;
use Magento\Framework\App\Route\ConfigInterface\Proxy as ConfigInterface;
Expand All @@ -16,7 +17,7 @@
/**
* Http request
*/
class Http extends Request implements RequestContentInterface, RequestSafetyInterface
class Http extends Request implements RequestContentInterface, RequestSafetyInterface, HttpRequestInterface
{
/**#@+
* HTTP Ports
Expand Down