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.

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

Jump to bottom

Refactor contact module #8420

Merged
merged 19 commits into from
Feb 25, 2017
Merged

Refactor contact module #8420

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
47a4681
Remove Zend_Validate calls
schmengler Feb 4, 2017
e1d47cc
Replace Zend_Validate_EmailAddress with 100% correct email validation
schmengler Feb 4, 2017
962c06d
Extract sendEmail method in contact form controller
schmengler Feb 4, 2017
fb902c6
Extract validatedParams method in contact form controller, use redire…
schmengler Feb 4, 2017
352dcef
Show meaningful error message if contact form validation fails
schmengler Feb 4, 2017
c8fbb70
Make validation and sending methods in contacts form controller prote…
schmengler Feb 4, 2017
36261ea
Add docblock comments
schmengler Feb 4, 2017
ec3fad1
Update unit test
schmengler Feb 4, 2017
0c78857
Fix code style violations
schmengler Feb 4, 2017
b4f68b0
Revert "Make validation and sending methods in contacts form controll…
schmengler Feb 16, 2017
5854390
Remove unnecessary dependency
schmengler Feb 16, 2017
854f16d
Moved down unnecessary dependencies from abstract base controller in …
schmengler Feb 16, 2017
22004d7
Move configuration access to config model / API interface
schmengler Feb 16, 2017
cdf8725
Merge remote-tracking branch 'origin/develop' into contact-refactor
schmengler Feb 16, 2017
28d939f
Extract mail sending from contacts controller, introduce mail service…
schmengler Feb 16, 2017
7a01613
Add docblock comments and empty lines
schmengler Feb 16, 2017
0261088
Add missing preference
schmengler Feb 16, 2017
a5c69d0
Move contact form interfaces from Api to Model namespace
schmengler Feb 19, 2017
f5ac9f7
Make method signature of Mail::send() more precise
schmengler Feb 19, 2017
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
117 changes: 66 additions & 51 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 @@ -8,6 +8,8 @@

use Magento\Framework\App\Request\DataPersistorInterface;
use Magento\Framework\App\ObjectManager;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\HTTP\PhpEnvironment\Request;

class Post extends \Magento\Contact\Controller\Index
{
Expand All @@ -19,72 +21,32 @@ class Post extends \Magento\Contact\Controller\Index
/**
* Post user question
*
* @return void
* @throws \Exception
*/
public function execute()
{
$post = $this->getRequest()->getPostValue();
if (!$post) {
$this->_redirect('*/*/');
return;
if (! $this->isPostRequest()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

extra space here after "!"

return $this->resultRedirectFactory->create()->setPath('*/*/');
}

$this->inlineTranslation->suspend();
try {
$postObject = new \Magento\Framework\DataObject();
$postObject->setData($post);

$error = false;

if (!\Zend_Validate::is(trim($post['name']), 'NotEmpty')) {
$error = true;
}
if (!\Zend_Validate::is(trim($post['comment']), 'NotEmpty')) {
$error = true;
}
if (!\Zend_Validate::is(trim($post['email']), 'EmailAddress')) {
$error = true;
}
if (\Zend_Validate::is(trim($post['hideit']), 'NotEmpty')) {
$error = true;
}
if ($error) {
throw new \Exception();
}

$storeScope = \Magento\Store\Model\ScopeInterface::SCOPE_STORE;
$transport = $this->_transportBuilder
->setTemplateIdentifier($this->scopeConfig->getValue(self::XML_PATH_EMAIL_TEMPLATE, $storeScope))
->setTemplateOptions(
[
'area' => \Magento\Backend\App\Area\FrontNameResolver::AREA_CODE,
'store' => \Magento\Store\Model\Store::DEFAULT_STORE_ID,
]
)
->setTemplateVars(['data' => $postObject])
->setFrom($this->scopeConfig->getValue(self::XML_PATH_EMAIL_SENDER, $storeScope))
->addTo($this->scopeConfig->getValue(self::XML_PATH_EMAIL_RECIPIENT, $storeScope))
->setReplyTo($post['email'])
->getTransport();

$transport->sendMessage();
$this->inlineTranslation->resume();
$this->sendEmail($this->validatedParams());
$this->messageManager->addSuccess(
__('Thanks for contacting us with your comments and questions. We\'ll respond to you very soon.')
);
$this->getDataPersistor()->clear('contact_us');
$this->_redirect('contact/index');
return;
} catch (LocalizedException $e) {
$this->messageManager->addErrorMessage($e->getMessage());
$this->getDataPersistor()->set('contact_us', $this->getRequest()->getParams());
} catch (\Exception $e) {
$this->inlineTranslation->resume();
$this->messageManager->addError(
$this->messageManager->addErrorMessage(
__('We can\'t process your request right now. Sorry, that\'s all we know.')
);
$this->getDataPersistor()->set('contact_us', $post);
$this->_redirect('contact/index');
return;
$this->getDataPersistor()->set('contact_us', $this->getRequest()->getParams());
} finally {
$this->inlineTranslation->resume();
}
return $this->resultRedirectFactory->create()->setPath('contact/index');
}

/**
Expand All @@ -101,4 +63,57 @@ private function getDataPersistor()

return $this->dataPersistor;
}

/**
* @param array $post Post data from contact form
*/
protected function sendEmail($post)
Copy link
Contributor

Choose a reason for hiding this comment

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

Magento discourages the introduction of any new "protected" properties or methods. The reason is that we are trying to get rid of inheritance-based APIS and promote composition ver inheritance. Only public or private modifiers should be used.

Please change this to "private"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's good to hear. Would you mind if I removed the abstract base controller then and moved the email sending out of the controller? I hesitated because it looked like inheritance was intended here and extensions might already use it (although that's improbable)

Copy link
Contributor

Choose a reason for hiding this comment

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

Abstract base controllers (and similar framework pieces such as Blocks, Models, etc) still remain places where legacy inheritance-based APIs haven't been refactored yet. I would suggest keeping abstract base controller for consistency with other controllers (and you are right, for backward compatibility)

However the policy for the new methods/properties to be public or private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be clear, I was referring to this one, not the framework base controller: https://github.com/magento/magento2/blob/develop/app/code/Magento/Contact/Controller/Index.php

{
$storeScope = \Magento\Store\Model\ScopeInterface::SCOPE_STORE;
$transport = $this->_transportBuilder
->setTemplateIdentifier($this->scopeConfig->getValue(self::XML_PATH_EMAIL_TEMPLATE, $storeScope))
->setTemplateOptions(
[
'area' => \Magento\Backend\App\Area\FrontNameResolver::AREA_CODE,
'store' => \Magento\Store\Model\Store::DEFAULT_STORE_ID,
Copy link
Contributor

Choose a reason for hiding this comment

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

our static test failing with this message:

Module Magento\Contact has undeclared dependencies: hard [Magento\Backend]

do we really need the dependency on Backend here? If yes, please add the dependency to the composer.json otherwise.

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 is the same code as before, just moved to a new method, but good that the static tests catched it :) I'll check if we can get rid of the dependency!

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, noticed that, though the fact you touched that classed triggered static test failure :) I'd appreciate if you ensure that the dependency is needed and add it to the composer.

]
)
->setTemplateVars(['data' => new \Magento\Framework\DataObject($post)])
->setFrom($this->scopeConfig->getValue(self::XML_PATH_EMAIL_SENDER, $storeScope))
->addTo($this->scopeConfig->getValue(self::XML_PATH_EMAIL_RECIPIENT, $storeScope))
->setReplyTo($post['email'])
->getTransport();

$transport->sendMessage();
}

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

/**
* @return array
* @throws \Exception
*/
protected function validatedParams()
{
$request = $this->getRequest();
if (trim($request->getParam('name')) === '') {
throw new LocalizedException(__('Name is missing'));
}
if (trim($request->getParam('comment')) === '') {
throw new LocalizedException(__('Comment is missing'));
}
if (false === \strpos($request->getParam('email'), '@')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You are losing quite a bit of functionality here vs Zend_Validate_EmailAddress. Maybe the better approach would be to switch to the ZF2 component https://github.com/zendframework/zend-validator/blob/master/src/EmailAddress.php#L492

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did that on purpose because I don't believe in email address validation via regular expressions (relevant: https://hackernoon.com/the-100-correct-way-to-validate-email-addresses-7c4818f24643). If there's a decision to use the MX check, the ZF2 email validator would be useful again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great article - you got me convinced.

Copy link
Member

@joe-vortex joe-vortex Apr 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Why are we not using php's native filter_var function? Surely this would be better as:

if (!filter_var($request->getParam('email'), FILTER_VALIDATE_EMAIL)) {
    throw new LocalizedException(__('Invalid email address'));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@joe-vortex same reason that I dropped Zend_Validate_EmailAddress. The top comment on http://php.net/manual/en/function.filter-var.php already gives an example

throw new LocalizedException(__('Invalid email address'));
}
if (trim($request->getParam('hideit')) !== '') {
throw new \Exception();
}

return $request->getParams();
}
}
3 changes: 3 additions & 0 deletions app/code/Magento/Contact/i18n/en_US.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,6 @@ Contacts,Contacts
"Email Sender","Email Sender"
"Email Template","Email Template"
"Email template chosen based on theme fallback when ""Default"" option is selected.","Email template chosen based on theme fallback when ""Default"" option is selected."
"Comment is missing","Comment is missing"
"Name is missing","Name is missing"
"Invalid email address","Invalid email address"
50 changes: 50 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 @@ -21,11 +21,61 @@ public function testPostAction()
$this->getRequest()->setPostValue($params);

$this->dispatch('contact/index/post');
$this->assertRedirect($this->stringContains('contact/index'));
$this->assertSessionMessages(
$this->contains(
"Thanks for contacting us with your comments and questions. We'll respond to you very soon."
),
\Magento\Framework\Message\MessageInterface::TYPE_SUCCESS
);
}

/**
* @dataProvider dataInvalidPostAction
* @param $params
* @param $expectedMessage
*/
public function testInvalidPostAction($params, $expectedMessage)
{
$this->getRequest()->setPostValue($params);

$this->dispatch('contact/index/post');
$this->assertRedirect($this->stringContains('contact/index'));
$this->assertSessionMessages(
$this->contains($expectedMessage),
\Magento\Framework\Message\MessageInterface::TYPE_ERROR
);
}
public static function dataInvalidPostAction()
{
return [
'missing_comment' => [
'params' => [
'name' => 'customer name',
'comment' => '',
'email' => 'user@example.com',
'hideit' => '',
],
'expectedMessage' => "Comment is missing",
],
'missing_name' => [
'params' => [
'name' => '',
'comment' => 'customer comment',
'email' => 'user@example.com',
'hideit' => '',
],
'expectedMessage' => "Name is missing",
],
'invalid_email' => [
'params' => [
'name' => 'customer name',
'comment' => 'customer comment',
'email' => 'invalidemail',
'hideit' => '',
],
'expectedMessage' => "Invalid email address",
],
];
}
}