Merged PR 58314: Add / update try catch when publishing to queues

## What's being changed In all places where we publish to queues, we are ensuring such calls are wrapped in try/catch statements. I've gone a little beyond this and made sure that all code in critical storefront observers (anything relating to customer interactions, or sales) is wrapped in try/catch. ## Why it's being changed It's important that observers and plugins in modules must never derail active processes in Magento. The publish call does throw a LocalizedException if the name of the topic is not recognised. ## How to review / test this change - Smoke test each updated observer, prioritising the ones where errors would impact the storefront - Pick a couple of critical ones (like OrderSaveAfter) and make them throw an exception by setting a bad topic name in the publisher->publish call Related work items: #265574
dotmailer · Oct 3, 2024 · 1196bb4 · 1196bb4
1 parent e6e241c
commit 1196bb4
Show file tree

Hide file tree

Showing 14 changed files with 394 additions and 282 deletions.
diff --git a/Controller/Email/Accountcallback.php b/Controller/Email/Accountcallback.php
@@ -3,6 +3,7 @@
 namespace Dotdigitalgroup\Email\Controller\Email;
 
 use Dotdigitalgroup\Email\Helper\Data;
+use Dotdigitalgroup\Email\Logger\Logger;
 use Dotdigitalgroup\Email\Model\Integration\IntegrationSetup;
 use Dotdigitalgroup\Email\Model\Integration\IntegrationSetupFactory;
 use Dotdigitalgroup\Email\Model\Sync\Integration\IntegrationInsights;
@@ -12,7 +13,6 @@
 use Magento\Framework\App\RequestInterface;
 use Magento\Framework\Controller\ResultFactory;
 use Magento\Framework\Controller\ResultInterface;
-use Magento\Framework\Exception\LocalizedException;
 use Magento\Framework\MessageQueue\PublisherInterface;
 
 class Accountcallback implements HttpPostActionInterface
@@ -27,6 +27,11 @@ class Accountcallback implements HttpPostActionInterface
      */
     private $helper;
 
+    /**
+     * @var Logger
+     */
+    private $logger;
+
     /**
      * @var ReinitableConfigInterface
      */
@@ -53,6 +58,7 @@ class Accountcallback implements HttpPostActionInterface
      * @param Context $context
      * @param IntegrationSetupFactory $integrationSetupFactory
      * @param Data $helper
+     * @param Logger $logger
      * @param ReinitableConfigInterface $reinitableConfig
      * @param ResultFactory $resultFactory
      * @param PublisherInterface $publisher
@@ -61,12 +67,14 @@ public function __construct(
         Context $context,
         IntegrationSetupFactory $integrationSetupFactory,
         Data $helper,
+        Logger $logger,
         ReinitableConfigInterface $reinitableConfig,
         ResultFactory $resultFactory,
         PublisherInterface $publisher
     ) {
         $this->integrationSetup = $integrationSetupFactory->create();
         $this->helper = $helper;
+        $this->logger = $logger;
         $this->reinitableConfig = $reinitableConfig;
         $this->request = $context->getRequest();
         $this->resultFactory = $resultFactory;
@@ -77,49 +85,52 @@ public function __construct(
      * Process the callback
      *
      * @return ResultInterface
-     * @throws LocalizedException
      */
     public function execute()
     {
         $params = $this->request->getParams();
         $website = $this->helper->getWebsiteForSelectedScopeInAdmin();
 
-        $this->helper->debug('Account callback request', $params);
+        $this->logger->debug('Account callback request', $params);
 
         if (!isset($params['code']) || !$this->integrationSetup->isCodeValid($params['code'])) {
             return $this->sendErrorResponse();
         }
 
-        // save credentials and reinit cache
-        $this->helper->saveApiCredentials(
-            $params['apiusername'],
-            $params['apipassword'],
-            $params['apiendpoint'] ?? null,
-            $website
-        );
-
-        // enable EC in Magento
-        $this->helper->enableEngagementCloud($website);
-        $this->reinitableConfig->reinit();
-
-        // set up EC account
-        $dataFieldsStatus = $this->integrationSetup->setupDataFields();
-        $addressBookStatus = $this->integrationSetup->createAddressBooks();
-        $syncStatus = $this->integrationSetup->enableSyncs();
-
-        //Clear config cache.
-        $this->reinitableConfig->reinit();
-
-        $this->helper->log('Dotdigital account creation', [
-            'api_username' => $params['apiusername'],
-            'api_endpoint' => $params['apiendpoint'],
-            'data_field_set_up' => $dataFieldsStatus,
-            'address_books_set_up' => $addressBookStatus,
-            'syncs_enabled_for_trial' => $syncStatus,
-        ]);
-
-        $this->helper->log('----PUBLISHING INTEGRATION INSIGHTS---');
-        $this->publisher->publish(IntegrationInsights::TOPIC_SYNC_INTEGRATION, '');
+        try {
+            // save credentials and reinit cache
+            $this->helper->saveApiCredentials(
+                $params['apiusername'],
+                $params['apipassword'],
+                $params['apiendpoint'] ?? null,
+                $website
+            );
+
+            // enable EC in Magento
+            $this->helper->enableEngagementCloud($website);
+            $this->reinitableConfig->reinit();
+
+            // set up EC account
+            $dataFieldsStatus = $this->integrationSetup->setupDataFields();
+            $addressBookStatus = $this->integrationSetup->createAddressBooks();
+            $syncStatus = $this->integrationSetup->enableSyncs();
+
+            //Clear config cache.
+            $this->reinitableConfig->reinit();
+
+            $this->logger->info('Dotdigital account creation', [
+                'api_username' => $params['apiusername'],
+                'api_endpoint' => $params['apiendpoint'],
+                'data_field_set_up' => $dataFieldsStatus,
+                'address_books_set_up' => $addressBookStatus,
+                'syncs_enabled_for_trial' => $syncStatus,
+            ]);
+
+            $this->logger->info('----PUBLISHING INTEGRATION INSIGHTS---');
+            $this->publisher->publish(IntegrationInsights::TOPIC_SYNC_INTEGRATION, '');
+        } catch (\Exception $e) {
+            $this->logger->error('Error in account callback controller', [(string) $e]);
+        }
 
         return $this->resultFactory->create(ResultFactory::TYPE_RAW)
             ->setHttpResponseCode(201);

diff --git a/Helper/Data.php b/Helper/Data.php
@@ -518,6 +518,9 @@ public function getLastOrderId()
      * @param array $extra
      *
      * @return void
+     *
+     * @deprecated Use dedicated Logger.
+     * @see \Dotdigitalgroup\Email\Logger\Logger
      */
     public function log($data, $extra = [])
     {
@@ -533,6 +536,9 @@ public function log($data, $extra = [])
      * @param array $extra
      *
      * @return void
+     *
+     * @deprecated Use dedicated Logger.
+     * @see \Dotdigitalgroup\Email\Logger\Logger
      */
     public function debug($message, $extra = [])
     {
@@ -546,6 +552,9 @@ public function debug($message, $extra = [])
      *
      * @param string $message
      * @param array $extra
+     *
+     * @deprecated Use dedicated Logger.
+     * @see \Dotdigitalgroup\Email\Logger\Logger
      */
     public function error($message, $extra = [])
     {

diff --git a/Model/AbandonedCart/ProgramEnrolment/Saver.php b/Model/AbandonedCart/ProgramEnrolment/Saver.php
@@ -4,7 +4,7 @@
 
 namespace Dotdigitalgroup\Email\Model\AbandonedCart\ProgramEnrolment;
 
-use Dotdigitalgroup\Email\Helper\Data;
+use Dotdigitalgroup\Email\Logger\Logger;
 use Dotdigitalgroup\Email\Model\AutomationFactory;
 use Dotdigitalgroup\Email\Model\Queue\Sync\Automation\AutomationPublisher;
 use Dotdigitalgroup\Email\Model\ResourceModel\Automation;
@@ -17,9 +17,9 @@
 class Saver
 {
     /**
-     * @var Data
+     * @var Logger
      */
-    private $helper;
+    private $logger;
 
     /**
      * @var AutomationFactory
@@ -39,21 +39,21 @@ class Saver
     /**
      * Saver constructor.
      *
+     * @param Logger $logger
      * @param AutomationFactory $automationFactory
      * @param AutomationPublisher $publisher
      * @param Automation $automationResource
-     * @param Data $data
      */
     public function __construct(
+        Logger $logger,
         AutomationFactory $automationFactory,
         AutomationPublisher $publisher,
-        Automation $automationResource,
-        Data $data
+        Automation $automationResource
     ) {
+        $this->logger = $logger;
         $this->automationFactory = $automationFactory;
         $this->publisher = $publisher;
         $this->automationResource = $automationResource;
-        $this->helper = $data;
     }
 
     /**
@@ -64,7 +64,6 @@ public function __construct(
      * @param int $programId
      *
      * @return void
-     * @throws Exception
      */
     public function save($quote, $store, $programId)
     {
@@ -82,7 +81,7 @@ public function save($quote, $store, $programId)
 
             $this->publisher->publish($automation);
         } catch (Exception $e) {
-            $this->helper->debug((string)$e, []);
+            $this->logger->error('Error in automation saver', [(string) $e]);
         }
     }
 }
diff --git a/Model/Automation.php b/Model/Automation.php
@@ -5,7 +5,7 @@
 namespace Dotdigitalgroup\Email\Model;
 
 use Dotdigitalgroup\Email\Helper\Config;
-use Dotdigitalgroup\Email\Helper\Data;
+use Dotdigitalgroup\Email\Logger\Logger;
 use Dotdigitalgroup\Email\Model\Queue\Sync\Automation\AutomationPublisher;
 use Dotdigitalgroup\Email\Model\Sync\Automation\AutomationTypeHandler;
 use Exception;
@@ -33,9 +33,9 @@ class Automation extends AbstractModel
     private $dateTime;
 
     /**
-     * @var Data
+     * @var Logger
      */
-    private $helper;
+    private $logger;
 
     /**
      * @var StoreManagerInterface
@@ -55,10 +55,10 @@ class Automation extends AbstractModel
     /**
      * Automation constructor.
      *
+     * @param Logger $logger
      * @param Context $context
      * @param Registry $registry
      * @param DateTime $dateTime
-     * @param Data $helper
      * @param ResourceModel\Automation $automationResource
      * @param StoreManagerInterface $storeManagerInterface
      * @param ScopeConfigInterface $scopeConfig
@@ -68,10 +68,10 @@ class Automation extends AbstractModel
      * @param array $data
      */
     public function __construct(
+        Logger $logger,
         Context $context,
         Registry $registry,
         DateTime $dateTime,
-        Data $helper,
         ResourceModel\Automation $automationResource,
         StoreManagerInterface $storeManagerInterface,
         ScopeConfigInterface $scopeConfig,
@@ -80,9 +80,9 @@ public function __construct(
         AbstractDb $resourceCollection = null,
         array $data = []
     ) {
+        $this->logger = $logger;
         $this->dateTime = $dateTime;
         $this->automationResource = $automationResource;
-        $this->helper = $helper;
         $this->scopeConfig = $scopeConfig;
         $this->publisher = $publisher;
         $this->storeManager = $storeManagerInterface;
@@ -129,13 +129,14 @@ public function beforeSave()
      */
     public function newCustomerAutomation($customer)
     {
-        $email = $customer->getEmail();
-        $websiteId = $customer->getWebsiteId();
-        $storeId = $customer->getStoreId();
-        $customerId = $customer->getId();
-        $store = $this->storeManager->getStore($storeId);
-        $storeName = $store->getName();
         try {
+            $email = $customer->getEmail();
+            $websiteId = $customer->getWebsiteId();
+            $storeId = $customer->getStoreId();
+            $customerId = $customer->getId();
+            $store = $this->storeManager->getStore($storeId);
+            $storeName = $store->getName();
+
             $apiEnabled = $this->scopeConfig->getValue(
                 Config::XML_PATH_CONNECTOR_API_ENABLED,
                 ScopeInterface::SCOPE_WEBSITE,
@@ -165,7 +166,7 @@ public function newCustomerAutomation($customer)
                 $this->publisher->publish($this);
             }
         } catch (Exception $e) {
-            $this->helper->debug((string)$e, []);
+            $this->logger->error('Error creating new customer automation', [(string) $e]);
         }
     }
 }
diff --git a/Model/Queue/Sync/Automation/AutomationPublisher.php b/Model/Queue/Sync/Automation/AutomationPublisher.php
@@ -4,6 +4,7 @@
 
 namespace Dotdigitalgroup\Email\Model\Queue\Sync\Automation;
 
+use Dotdigitalgroup\Email\Logger\Logger;
 use Dotdigitalgroup\Email\Model\Automation;
 use Dotdigitalgroup\Email\Model\Queue\Data\AutomationDataFactory;
 use Magento\Framework\MessageQueue\PublisherInterface;
@@ -12,6 +13,11 @@ class AutomationPublisher
 {
     public const TOPIC_SYNC_AUTOMATION = 'ddg.sync.automation';
 
+    /**
+     * @var Logger
+     */
+    private $logger;
+
     /**
      * @var AutomationDataFactory
      */
@@ -23,13 +29,16 @@ class AutomationPublisher
     private $publisher;
 
     /**
+     * @param Logger $logger
      * @param AutomationDataFactory $automationDataFactory
      * @param PublisherInterface $publisher
      */
     public function __construct(
+        Logger $logger,
         AutomationDataFactory $automationDataFactory,
         PublisherInterface $publisher
     ) {
+        $this->logger = $logger;
         $this->automationDataFactory = $automationDataFactory;
         $this->publisher = $publisher;
     }
@@ -47,6 +56,10 @@ public function publish(Automation $automation)
         $message->setId((int) $automation->getId());
         $message->setType($automation->getAutomationType());
 
-        $this->publisher->publish(self::TOPIC_SYNC_AUTOMATION, $message);
+        try {
+            $this->publisher->publish(self::TOPIC_SYNC_AUTOMATION, $message);
+        } catch (\Exception $e) {
+            $this->logger->error('Automation publish failed', [(string) $e]);
+        }
     }
 }