From 9cd871c32e4c8bc3da6e5b736d9f9ffbd7cebe42 Mon Sep 17 00:00:00 2001 From: Jan Jakes Date: Thu, 31 Aug 2023 15:34:23 +0200 Subject: [PATCH] Remove unused step runner interface [MAILPOET-5568] --- .../Automation/Engine/Control/StepHandler.php | 85 +++++++------------ .../Automation/Engine/Control/StepRunner.php | 10 --- .../Engine/Control/Steps/ActionStepRunner.php | 36 -------- mailpoet/lib/Automation/Engine/Hooks.php | 1 - mailpoet/lib/DI/ContainerConfigurator.php | 1 - .../Engine/Control/StepHandlerTest.php | 50 ++++++----- 6 files changed, 57 insertions(+), 126 deletions(-) delete mode 100644 mailpoet/lib/Automation/Engine/Control/StepRunner.php delete mode 100644 mailpoet/lib/Automation/Engine/Control/Steps/ActionStepRunner.php diff --git a/mailpoet/lib/Automation/Engine/Control/StepHandler.php b/mailpoet/lib/Automation/Engine/Control/StepHandler.php index d6b1281ff5..7b3429f498 100644 --- a/mailpoet/lib/Automation/Engine/Control/StepHandler.php +++ b/mailpoet/lib/Automation/Engine/Control/StepHandler.php @@ -3,11 +3,9 @@ namespace MailPoet\Automation\Engine\Control; use Exception; -use MailPoet\Automation\Engine\Control\Steps\ActionStepRunner; use MailPoet\Automation\Engine\Data\Automation; use MailPoet\Automation\Engine\Data\AutomationRun; use MailPoet\Automation\Engine\Data\AutomationRunLog; -use MailPoet\Automation\Engine\Data\Step; use MailPoet\Automation\Engine\Data\StepRunArgs; use MailPoet\Automation\Engine\Data\StepValidationArgs; use MailPoet\Automation\Engine\Data\SubjectEntry; @@ -25,9 +23,6 @@ use MailPoet\Automation\Engine\WordPress; use Throwable; class StepHandler { - /** @var ActionStepRunner */ - private $actionStepRunner; - /** @var SubjectLoader */ private $subjectLoader; @@ -40,9 +35,6 @@ class StepHandler { /** @var AutomationStorage */ private $automationStorage; - /** @var array */ - private $stepRunners = []; - /** @var AutomationRunLogStorage */ private $automationRunLogStorage; @@ -52,11 +44,13 @@ class StepHandler { /** @var Registry */ private $registry; + /** @var StepRunControllerFactory */ + private $stepRunControllerFactory; + /** @var StepScheduler */ private $stepScheduler; public function __construct( - ActionStepRunner $actionStepRunner, Hooks $hooks, SubjectLoader $subjectLoader, WordPress $wordPress, @@ -64,9 +58,9 @@ class StepHandler { AutomationRunLogStorage $automationRunLogStorage, AutomationStorage $automationStorage, Registry $registry, + StepRunControllerFactory $stepRunControllerFactory, StepScheduler $stepScheduler ) { - $this->actionStepRunner = $actionStepRunner; $this->hooks = $hooks; $this->subjectLoader = $subjectLoader; $this->wordPress = $wordPress; @@ -74,28 +68,12 @@ class StepHandler { $this->automationRunLogStorage = $automationRunLogStorage; $this->automationStorage = $automationStorage; $this->registry = $registry; + $this->stepRunControllerFactory = $stepRunControllerFactory; $this->stepScheduler = $stepScheduler; } public function initialize(): void { $this->wordPress->addAction(Hooks::AUTOMATION_STEP, [$this, 'handle']); - $this->addStepRunner(Step::TYPE_ACTION, $this->actionStepRunner); - $this->wordPress->doAction(Hooks::STEP_RUNNER_INITIALIZE, [$this]); - } - - public function addStepRunner(string $stepType, StepRunner $stepRunner): void { - $this->stepRunners[$stepType] = $stepRunner; - } - - public function getStepRunners(): array { - return $this->stepRunners; - } - - /** - * @param array $stepRunners - */ - public function setStepRunners(array $stepRunners): void { - $this->stepRunners = $stepRunners; } /** @param mixed $args */ @@ -153,35 +131,38 @@ class StepHandler { if (!$stepData) { throw Exceptions::automationStepNotFound($stepId); } + $step = $this->registry->getStep($stepData->getKey()); - $stepType = $stepData->getType(); - if (isset($this->stepRunners[$stepType])) { - $log = new AutomationRunLog($automationRun->getId(), $stepData->getId()); - try { - $requiredSubjects = $step instanceof Action ? $step->getSubjectKeys() : []; - $subjectEntries = $this->getSubjectEntries($automationRun, $requiredSubjects); - $args = new StepRunArgs($automation, $automationRun, $stepData, $subjectEntries, $stepRunNumber); - $validationArgs = new StepValidationArgs($automation, $stepData, array_map(function (SubjectEntry $entry) { - return $entry->getSubject(); - }, $subjectEntries)); - $this->stepRunners[$stepType]->run($args, $validationArgs); - $log->markCompletedSuccessfully(); - } catch (Throwable $e) { - $log->markFailed(); - $log->setError($e); - throw $e; - } finally { - try { - $this->hooks->doAutomationStepAfterRun($log); - } catch (Throwable $e) { - // Ignore integration errors - } - $this->automationRunLogStorage->createAutomationRunLog($log); - } - } else { + if (!$step instanceof Action) { throw new InvalidStateException(); } + $log = new AutomationRunLog($automationRun->getId(), $stepData->getId()); + try { + $requiredSubjects = $step->getSubjectKeys(); + $subjectEntries = $this->getSubjectEntries($automationRun, $requiredSubjects); + $args = new StepRunArgs($automation, $automationRun, $stepData, $subjectEntries, $stepRunNumber); + $validationArgs = new StepValidationArgs($automation, $stepData, array_map(function (SubjectEntry $entry) { + return $entry->getSubject(); + }, $subjectEntries)); + + $step->validate($validationArgs); + $step->run($args, $this->stepRunControllerFactory->createController($args)); + + $log->markCompletedSuccessfully(); + } catch (Throwable $e) { + $log->markFailed(); + $log->setError($e); + throw $e; + } finally { + try { + $this->hooks->doAutomationStepAfterRun($log); + } catch (Throwable $e) { + // Ignore integration errors + } + $this->automationRunLogStorage->createAutomationRunLog($log); + } + // schedule next step if not scheduled by action if (!$this->stepScheduler->hasScheduledStep($args)) { $this->stepScheduler->scheduleNextStep($args); diff --git a/mailpoet/lib/Automation/Engine/Control/StepRunner.php b/mailpoet/lib/Automation/Engine/Control/StepRunner.php deleted file mode 100644 index 4b118f6d02..0000000000 --- a/mailpoet/lib/Automation/Engine/Control/StepRunner.php +++ /dev/null @@ -1,10 +0,0 @@ -registry = $registry; - $this->stepRunControllerFactory = $stepRunControllerFactory; - } - - public function run(StepRunArgs $runArgs, StepValidationArgs $validationArgs): void { - $action = $this->registry->getAction($runArgs->getStep()->getKey()); - if (!$action) { - throw new InvalidStateException(); - } - - $action->validate($validationArgs); - $action->run($runArgs, $this->stepRunControllerFactory->createController($runArgs)); - } -} diff --git a/mailpoet/lib/Automation/Engine/Hooks.php b/mailpoet/lib/Automation/Engine/Hooks.php index d8d8f7232a..9fcb71bd5f 100644 --- a/mailpoet/lib/Automation/Engine/Hooks.php +++ b/mailpoet/lib/Automation/Engine/Hooks.php @@ -18,7 +18,6 @@ class Hooks { public const INITIALIZE = 'mailpoet/automation/initialize'; public const API_INITIALIZE = 'mailpoet/automation/api/initialize'; - public const STEP_RUNNER_INITIALIZE = 'mailpoet/automation/step_runner/initialize'; public const TRIGGER = 'mailpoet/automation/trigger'; public const AUTOMATION_STEP = 'mailpoet/automation/step'; diff --git a/mailpoet/lib/DI/ContainerConfigurator.php b/mailpoet/lib/DI/ContainerConfigurator.php index fc18764421..7a84f3046f 100644 --- a/mailpoet/lib/DI/ContainerConfigurator.php +++ b/mailpoet/lib/DI/ContainerConfigurator.php @@ -130,7 +130,6 @@ class ContainerConfigurator implements IContainerConfigurator { $container->autowire(\MailPoet\Automation\Engine\Control\StepScheduler::class)->setPublic(true); $container->autowire(\MailPoet\Automation\Engine\Control\SubjectTransformerHandler::class)->setPublic(true)->setShared(false); $container->autowire(\MailPoet\Automation\Engine\Control\SubjectLoader::class)->setPublic(true); - $container->autowire(\MailPoet\Automation\Engine\Control\Steps\ActionStepRunner::class)->setPublic(true); $container->autowire(\MailPoet\Automation\Engine\Control\TriggerHandler::class)->setPublic(true); $container->autowire(\MailPoet\Automation\Engine\Engine::class)->setPublic(true); $container->autowire(\MailPoet\Automation\Engine\Hooks::class)->setPublic(true); diff --git a/mailpoet/tests/integration/Automation/Engine/Control/StepHandlerTest.php b/mailpoet/tests/integration/Automation/Engine/Control/StepHandlerTest.php index eb65f04769..be00e533f6 100644 --- a/mailpoet/tests/integration/Automation/Engine/Control/StepHandlerTest.php +++ b/mailpoet/tests/integration/Automation/Engine/Control/StepHandlerTest.php @@ -3,39 +3,40 @@ namespace MailPoet\Test\Automation\Engine\Control; use MailPoet\Automation\Engine\Control\StepHandler; -use MailPoet\Automation\Engine\Control\StepRunner; use MailPoet\Automation\Engine\Data\Automation; use MailPoet\Automation\Engine\Data\AutomationRun; use MailPoet\Automation\Engine\Data\NextStep; use MailPoet\Automation\Engine\Data\Step; use MailPoet\Automation\Engine\Exceptions\InvalidStateException; +use MailPoet\Automation\Engine\Integration\Action; +use MailPoet\Automation\Engine\Registry; use MailPoet\Automation\Engine\Storage\AutomationRunStorage; use MailPoet\Automation\Engine\Storage\AutomationStorage; use MailPoet\Automation\Integrations\Core\Actions\DelayAction; use MailPoet\Automation\Integrations\MailPoet\Triggers\SomeoneSubscribesTrigger; class StepHandlerTest extends \MailPoetTest { - /** @var AutomationStorage */ private $automationStorage; /** @var AutomationRunStorage */ private $automationRunStorage; - /** @var StepHandler */ - private $testee; - - /** @var array */ - private $originalRunners = []; - public function _before() { - $this->testee = $this->diContainer->get(StepHandler::class); $this->automationStorage = $this->diContainer->get(AutomationStorage::class); $this->automationRunStorage = $this->diContainer->get(AutomationRunStorage::class); - $this->originalRunners = $this->testee->getStepRunners(); } public function testItDoesOnlyProcessActiveAndDeactivatingAutomations() { + // The run method will be called twice: Once for the active automation and once for the deactivating automation. + $step = $this->createMock(Action::class); + $step->expects(self::exactly(2))->method('run'); + $registry = $this->createMock(Registry::class); + $registry->expects(self::exactly(2))->method('getStep')->willReturn($step); + $stepHandler = $this->getServiceWithOverrides(StepHandler::class, [ + 'registry' => $registry, + ]); + $automation = $this->createAutomation(); $this->assertInstanceOf(Automation::class, $automation); $steps = $automation->getSteps(); @@ -44,13 +45,9 @@ class StepHandlerTest extends \MailPoetTest { $currentStep = current($steps); $this->assertInstanceOf(Step::class, $currentStep); - $runner = $this->createMock(StepRunner::class); - $runner->expects(self::exactly(2))->method('run'); // The run method will be called twice: Once for the active automation and once for the deactivating automation. - - $this->testee->addStepRunner($currentStep->getType(), $runner); $this->assertSame(Automation::STATUS_ACTIVE, $automation->getStatus()); - $this->testee->handle(['automation_run_id' => $automationRun->getId(), 'step_id' => $currentStep->getId()]); + $stepHandler->handle(['automation_run_id' => $automationRun->getId(), 'step_id' => $currentStep->getId()]); // no exception thrown. $newAutomationRun = $this->automationRunStorage->getAutomationRun($automationRun->getId()); $this->assertInstanceOf(AutomationRun::class, $newAutomationRun); @@ -58,7 +55,7 @@ class StepHandlerTest extends \MailPoetTest { $automation->setStatus(Automation::STATUS_DEACTIVATING); $this->automationStorage->updateAutomation($automation); - $this->testee->handle(['automation_run_id' => $automationRun->getId(), 'step_id' => $currentStep->getId()]); + $stepHandler->handle(['automation_run_id' => $automationRun->getId(), 'step_id' => $currentStep->getId()]); // no exception thrown. $newAutomationRun = $this->automationRunStorage->getAutomationRun($automationRun->getId()); $this->assertInstanceOf(AutomationRun::class, $newAutomationRun); @@ -78,7 +75,7 @@ class StepHandlerTest extends \MailPoetTest { $this->assertInstanceOf(AutomationRun::class, $automationRun); $error = null; try { - $this->testee->handle(['automation_run_id' => $automationRun->getId(), 'step_id' => $currentStep->getId()]); + $stepHandler->handle(['automation_run_id' => $automationRun->getId(), 'step_id' => $currentStep->getId()]); } catch (InvalidStateException $error) { $this->assertSame('mailpoet_automation_not_active', $error->getErrorCode(), "Automation with '$status' did not return expected error code."); } @@ -92,6 +89,14 @@ class StepHandlerTest extends \MailPoetTest { } public function testAnDeactivatingAutomationBecomesDraftAfterLastRunIsExecuted() { + $step = $this->createMock(Action::class); + $step->expects(self::exactly(2))->method('run'); + $registry = $this->createMock(Registry::class); + $registry->expects(self::exactly(2))->method('getStep')->willReturn($step); + $stepHandler = $this->getServiceWithOverrides(StepHandler::class, [ + 'registry' => $registry, + ]); + $automation = $this->createAutomation(); $this->assertInstanceOf(Automation::class, $automation); $automationRun1 = $this->tester->createAutomationRun($automation); @@ -104,10 +109,8 @@ class StepHandlerTest extends \MailPoetTest { $steps = $automation->getSteps(); $lastStep = end($steps); $this->assertInstanceOf(Step::class, $lastStep); - $runner = $this->createMock(StepRunner::class); - $this->testee->addStepRunner($lastStep->getType(), $runner); - $this->testee->handle(['automation_run_id' => $automationRun1->getId(), 'step_id' => $lastStep->getId()]); + $stepHandler->handle(['automation_run_id' => $automationRun1->getId(), 'step_id' => $lastStep->getId()]); /** @var Automation $updatedAutomation */ $updatedAutomation = $this->automationStorage->getAutomation($automation->getId()); /** @var AutomationRun $updatedautomationRun */ @@ -115,7 +118,7 @@ class StepHandlerTest extends \MailPoetTest { $this->assertSame(Automation::STATUS_DEACTIVATING, $updatedAutomation->getStatus()); $this->assertSame(AutomationRun::STATUS_COMPLETE, $updatedautomationRun->getStatus()); - $this->testee->handle(['automation_run_id' => $automationRun2->getId(), 'step_id' => $lastStep->getId()]); + $stepHandler->handle(['automation_run_id' => $automationRun2->getId(), 'step_id' => $lastStep->getId()]); /** @var Automation $updatedAutomation */ $updatedAutomation = $this->automationStorage->getAutomation($automation->getId()); /** @var AutomationRun $updatedautomationRun */ @@ -133,9 +136,4 @@ class StepHandlerTest extends \MailPoetTest { new Step('delay', Step::TYPE_ACTION, $delay->getKey(), [], []) ); } - - public function _after() { - parent::_after(); - $this->testee->setStepRunners($this->originalRunners); - } }