Refactor contact module #8420
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| <?php | ||
| /** | ||
| * Contact module base controller | ||
| */ | ||
| namespace Magento\Contact\Api; | ||
|
|
||
| /** | ||
| * Contact module configuration | ||
| */ | ||
| interface ConfigInterface | ||
| { | ||
| /** | ||
| * Recipient email config path | ||
| */ | ||
| const XML_PATH_EMAIL_RECIPIENT = 'contact/email/recipient_email'; | ||
|
|
||
| /** | ||
| * Sender email config path | ||
| */ | ||
| const XML_PATH_EMAIL_SENDER = 'contact/email/sender_email_identity'; | ||
|
|
||
| /** | ||
| * Email template config path | ||
| */ | ||
| const XML_PATH_EMAIL_TEMPLATE = 'contact/email/email_template'; | ||
|
|
||
| /** | ||
| * Enabled config path | ||
| */ | ||
| const XML_PATH_ENABLED = 'contact/contact/enabled'; | ||
|
|
||
| /** | ||
| * Check if contacts module is enabled | ||
| * | ||
| * @return bool | ||
| */ | ||
| public function isEnabled(); | ||
|
|
||
| /** | ||
| * Return email template identifier | ||
| * | ||
| * @return string | ||
| */ | ||
| public function emailTemplate(); | ||
|
|
||
| /** | ||
| * Return email sender address | ||
| * | ||
| * @return string | ||
| */ | ||
| public function emailSender(); | ||
|
|
||
| /** | ||
| * Return email recipient address | ||
| * | ||
| * @return mixed | ||
|
There was a problem hiding this comment. Service Contracts does not allow usage of "mixed" type There was a problem hiding this comment. good catch, should have been |
||
| */ | ||
| public function emailRecipient(); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| <?php | ||
| /** | ||
| * Contact module base controller | ||
| */ | ||
| namespace Magento\Contact\Api; | ||
|
|
||
| interface MailInterface | ||
| { | ||
| /** | ||
| * Send email from contact form | ||
| * | ||
| * @param string $replyTo Reply-to email address | ||
| * @param array $variables Email template variables | ||
|
There was a problem hiding this comment. Service Contracts does not allow usage of "array" type, it should be more specific, i.e. "String[]" There was a problem hiding this comment. but email template variables can be anything, from strings and integers to objects. How would you deal with it? There was a problem hiding this comment. I now understand that these rules only apply to service contracts in the sense of interfaces that can be exposed as web API, so I assume that I can leave the signature as it is, after moving the interfaces out of |
||
| * @return void | ||
| */ | ||
| public function send($replyTo, $variables); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,8 +6,16 @@ | |
| */ | ||
| namespace Magento\Contact\Controller\Index; | ||
|
|
||
| use Magento\Contact\Api\ConfigInterface; | ||
| use Magento\Contact\Api\MailInterface; | ||
| use Magento\Framework\App\Action\Context; | ||
| use Magento\Framework\App\ObjectManager; | ||
| use Magento\Framework\App\Request\DataPersistorInterface; | ||
| use Magento\Framework\Controller\Result\Redirect; | ||
| use Magento\Framework\Exception\LocalizedException; | ||
| use Magento\Framework\HTTP\PhpEnvironment\Request; | ||
| use Magento\Framework\Mail\Template\TransportBuilder; | ||
| use Magento\Framework\Translate\Inline\StateInterface; | ||
|
|
||
| class Post extends \Magento\Contact\Controller\Index | ||
| { | ||
|
|
@@ -16,75 +24,60 @@ class Post extends \Magento\Contact\Controller\Index | |
| */ | ||
| private $dataPersistor; | ||
|
|
||
| /** | ||
| * @var Context | ||
| */ | ||
| private $context; | ||
|
|
||
| /** | ||
| * @var MailInterface | ||
| */ | ||
| private $mail; | ||
|
|
||
| /** | ||
| * @param Context $context | ||
| * @param MailInterface $mail | ||
| * @param ConfigInterface $contactsConfig | ||
| * @param DataPersistorInterface $dataPersistor | ||
| */ | ||
| public function __construct( | ||
| Context $context, | ||
| MailInterface $mail, | ||
| ConfigInterface $contactsConfig, | ||
| DataPersistorInterface $dataPersistor | ||
| ) { | ||
| parent::__construct($context, $contactsConfig); | ||
| $this->context = $context; | ||
| $this->mail = $mail; | ||
| $this->dataPersistor = $dataPersistor; | ||
| } | ||
|
|
||
| /** | ||
| * Post user question | ||
| * | ||
| * @return Redirect | ||
| */ | ||
| public function execute() | ||
| { | ||
| if (! $this->isPostRequest()) { | ||
|
|
||
| return $this->resultRedirectFactory->create()->setPath('*/*/'); | ||
| } | ||
| try { | ||
| $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'); | ||
| } catch (LocalizedException $e) { | ||
| $this->messageManager->addErrorMessage($e->getMessage()); | ||
| $this->getDataPersistor()->set('contact_us', $this->getRequest()->getParams()); | ||
| } catch (\Exception $e) { | ||
| $this->messageManager->addErrorMessage( | ||
| __('We can\'t process your request right now. Sorry, that\'s all we know.') | ||
| ); | ||
| $this->getDataPersistor()->set('contact_us', $this->getRequest()->getParams()); | ||
| } | ||
| return $this->resultRedirectFactory->create()->setPath('contact/index'); | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -101,4 +94,46 @@ private function getDataPersistor() | |
|
|
||
| return $this->dataPersistor; | ||
| } | ||
|
|
||
| /** | ||
| * @param array $post Post data from contact form | ||
| * @return void | ||
| */ | ||
| private function sendEmail($post) | ||
| { | ||
| $this->mail->send($post['email'], ['data' => new \Magento\Framework\DataObject($post)]); | ||
| } | ||
|
|
||
| /** | ||
| * @return bool | ||
| */ | ||
| private function isPostRequest() | ||
| { | ||
| /** @var Request $request */ | ||
| $request = $this->getRequest(); | ||
| return !empty($request->getPostValue()); | ||
| } | ||
|
|
||
| /** | ||
| * @return array | ||
| * @throws \Exception | ||
| */ | ||
| private 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'), '@')) { | ||
|
There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. Great article - you got me convinced. There was a problem hiding this comment. Why are we not using php's native There was a problem hiding this comment. @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(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface is neither Service Interface (does not describe operations on data) nor Data Interface (it does not consist only of getters and setters for the fields). Also, it does not looks like it makes sense to expose it as Web API.
Let's move these two interfaces out of Api namespace to the Model. Please add @api annotation to the interface to indicate that this is backward compatible module's interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review. I did not intend to expose the interfaces to the web api, they were only meant as service contracts. I was not aware of the convention to distinguish between
Apinamespace and@apiannotation, will change it as suggested