Improve code quality for Subscriber/NewAction

+ Refactor `else` block for returning success message to private function
+ Use import statements for classes
+ Because imports are being used an `if` statement can be written on one line
+ Use `addExceptionMessage` and `addSuccessMessage` instead of their deprecated counterparts
+ Include with DI the `Magento\Framework\Validator\EmailAddress` class
which can be used to validate the email address. Therefore the static
function call to the `is` method on the `Zend_Validate` class is not
needed anymore.

Author: arnoudhgz

app/code/Magento/Newsletter/Controller/Subscriber/NewAction.php

@@ -10,19 +10,32 @@
 use Magento\Customer\Model\Session;
 use Magento\Customer\Model\Url as CustomerUrl;
 use Magento\Framework\App\Action\Context;
+use Magento\Framework\App\Config\ScopeConfigInterface;
+use Magento\Framework\App\ObjectManager;
+use Magento\Framework\Exception\LocalizedException;
+use Magento\Framework\Phrase;
+use Magento\Framework\Validator\EmailAddress as EmailValidator;
+use Magento\Newsletter\Controller\Subscriber as SubscriberController;
+use Magento\Newsletter\Model\Subscriber;
+use Magento\Store\Model\ScopeInterface;
 use Magento\Store\Model\StoreManagerInterface;
 use Magento\Newsletter\Model\SubscriberFactory;
 
 /**
  * @SuppressWarnings(PHPMD.CouplingBetweenObjects)
  */
-class NewAction extends \Magento\Newsletter\Controller\Subscriber
+class NewAction extends SubscriberController
 {
     /**
      * @var CustomerAccountManagement
      */
     protected $customerAccountManagement;
 
+    /**
+     * @var EmailValidator
+     */
+    private $emailValidator;
+
     /**
      * Initialize dependencies.
      *
@@ -32,16 +45,19 @@ class NewAction extends \Magento\Newsletter\Controller\Subscriber
      * @param StoreManagerInterface $storeManager
      * @param CustomerUrl $customerUrl
      * @param CustomerAccountManagement $customerAccountManagement
+     * @param EmailValidator $emailValidator
      */
     public function __construct(
         Context $context,
         SubscriberFactory $subscriberFactory,
         Session $customerSession,
         StoreManagerInterface $storeManager,
         CustomerUrl $customerUrl,
-        CustomerAccountManagement $customerAccountManagement
+        CustomerAccountManagement $customerAccountManagement,
+        EmailValidator $emailValidator = null
     ) {
         $this->customerAccountManagement = $customerAccountManagement;
+        $this->emailValidator = $emailValidator ?: ObjectManager::getInstance()->get(EmailValidator::class);
         parent::__construct(
             $context,
             $subscriberFactory,
@@ -55,7 +71,7 @@ public function __construct(
      * Validates that the email address isn't being used by a different account.
      *
      * @param string $email
-     * @throws \Magento\Framework\Exception\LocalizedException
+     * @throws LocalizedException
      * @return void
      */
     protected function validateEmailAvailable($email)
@@ -64,7 +80,7 @@ protected function validateEmailAvailable($email)
         if ($this->_customerSession->getCustomerDataObject()->getEmail() !== $email
             && !$this->customerAccountManagement->isEmailAvailable($email, $websiteId)
         ) {
-            throw new \Magento\Framework\Exception\LocalizedException(
+            throw new LocalizedException(
                 __('This email address is already assigned to another user.')
             );
         }
@@ -73,19 +89,19 @@ protected function validateEmailAvailable($email)
     /**
      * Validates that if the current user is a guest, that they can subscribe to a newsletter.
      *
-     * @throws \Magento\Framework\Exception\LocalizedException
+     * @throws LocalizedException
      * @return void
      */
     protected function validateGuestSubscription()
     {
-        if ($this->_objectManager->get(\Magento\Framework\App\Config\ScopeConfigInterface::class)
+        if ($this->_objectManager->get(ScopeConfigInterface::class)
                 ->getValue(
-                    \Magento\Newsletter\Model\Subscriber::XML_PATH_ALLOW_GUEST_SUBSCRIBE_FLAG,
-                    \Magento\Store\Model\ScopeInterface::SCOPE_STORE
+                    Subscriber::XML_PATH_ALLOW_GUEST_SUBSCRIBE_FLAG,
+                    ScopeInterface::SCOPE_STORE
                 ) != 1
             && !$this->_customerSession->isLoggedIn()
         ) {
-            throw new \Magento\Framework\Exception\LocalizedException(
+            throw new LocalizedException(
                 __(
                     'Sorry, but the administrator denied subscription for guests. Please <a href="%1">register</a>.',
                     $this->_customerUrl->getRegisterUrl()
@@ -98,20 +114,19 @@ protected function validateGuestSubscription()
      * Validates the format of the email address
      *
      * @param string $email
-     * @throws \Magento\Framework\Exception\LocalizedException
+     * @throws LocalizedException
      * @return void
      */
     protected function validateEmailFormat($email)
     {
-        if (!\Zend_Validate::is($email, \Magento\Framework\Validator\EmailAddress::class)) {
-            throw new \Magento\Framework\Exception\LocalizedException(__('Please enter a valid email address.'));
+        if (!$this->emailValidator->isValid($email)) {
+            throw new LocalizedException(__('Please enter a valid email address.'));
         }
     }
 
     /**
      * New subscription action
      *
-     * @throws \Magento\Framework\Exception\LocalizedException
      * @return void
      */
     public function execute()
@@ -126,28 +141,37 @@ public function execute()
 
                 $subscriber = $this->_subscriberFactory->create()->loadByEmail($email);
                 if ($subscriber->getId()
-                    && $subscriber->getSubscriberStatus() == \Magento\Newsletter\Model\Subscriber::STATUS_SUBSCRIBED
+                    && (int) $subscriber->getSubscriberStatus() === Subscriber::STATUS_SUBSCRIBED
                 ) {
-                    throw new \Magento\Framework\Exception\LocalizedException(
+                    throw new LocalizedException(
                         __('This email address is already subscribed.')
                     );
                 }
 
-                $status = $this->_subscriberFactory->create()->subscribe($email);
-                if ($status == \Magento\Newsletter\Model\Subscriber::STATUS_NOT_ACTIVE) {
-                    $this->messageManager->addSuccess(__('The confirmation request has been sent.'));
-                } else {
-                    $this->messageManager->addSuccess(__('Thank you for your subscription.'));
-                }
-            } catch (\Magento\Framework\Exception\LocalizedException $e) {
-                $this->messageManager->addException(
+                $status = (int) $this->_subscriberFactory->create()->subscribe($email);
+                $this->messageManager->addSuccessMessage($this->getSuccessMessage($status));
+            } catch (LocalizedException $e) {
+                $this->messageManager->addExceptionMessage(
                     $e,
                     __('There was a problem with the subscription: %1', $e->getMessage())
                 );
             } catch (\Exception $e) {
-                $this->messageManager->addException($e, __('Something went wrong with the subscription.'));
+                $this->messageManager->addExceptionMessage($e, __('Something went wrong with the subscription.'));
             }
         }
         $this->getResponse()->setRedirect($this->_redirect->getRedirectUrl());
     }
+
+    /**
+     * @param int $status
+     * @return Phrase
+     */
+    private function getSuccessMessage(int $status): Phrase
+    {
+        if ($status === Subscriber::STATUS_NOT_ACTIVE) {
+            return __('The confirmation request has been sent.');
+        }
+
+        return __('Thank you for your subscription.');
+    }
 }