Collect all step error types instead of terminating on the first one

[MAILPOET-4659]

mailpoet/lib/Automation/Engine/Exceptions.php

@@ -28,6 +28,7 @@ class Exceptions {
   private const WORKFLOW_STRUCTURE_NOT_VALID = 'mailpoet_automation_workflow_structure_not_valid';
   private const WORKFLOW_STEP_MODIFIED_WHEN_UNKNOWN = 'mailpoet_automation_workflow_step_modified_when_unknown';
   private const WORKFLOW_NOT_VALID = 'mailpoet_automation_workflow_not_valid';
+  private const MISSING_REQUIRED_SUBJECTS = 'mailpoet_automation_missing_required_subjects';
 
   public function __construct() {
     throw new InvalidStateException(
@@ -190,4 +191,17 @@ class Exceptions {
       ->withMessage(sprintf(__("Workflow validation failed: %s", 'mailpoet'), $detail))
       ->withErrors($errors);
   }
+
+  public static function missingRequiredSubjects(Step $step, array $missingSubjectKeys): UnexpectedValueException {
+    return UnexpectedValueException::create()
+      ->withErrorCode(self::MISSING_REQUIRED_SUBJECTS)
+      // translators: %1$s is the key of the step, %2$s are the missing subject keys.
+      ->withMessage(
+        sprintf(
+          __("Step with ID '%1\$s' is missing required subjects with keys: %2\$s", 'mailpoet'),
+          $step->getId(),
+          implode(', ', $missingSubjectKeys)
+        )
+      );
+  }
 }

mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepArgsRule.php

@@ -3,7 +3,6 @@
 namespace MailPoet\Automation\Engine\Validation\WorkflowRules;
 
 use MailPoet\Automation\Engine\Data\Workflow;
-use MailPoet\Automation\Engine\Exceptions;
 use MailPoet\Automation\Engine\Registry;
 use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowNode;
 use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowNodeVisitor;
@@ -16,9 +15,6 @@ class ValidStepArgsRule implements WorkflowNodeVisitor {
   /** @var Validator */
   private $validator;
 
-  /** @var array<string, array{step_id: string, message: string}> */
-  private $errors = [];
-
   public function __construct(
     Registry $registry,
     Validator $validator
@@ -28,7 +24,6 @@ class ValidStepArgsRule implements WorkflowNodeVisitor {
   }
 
   public function initialize(Workflow $workflow): void {
-    $this->errors = [];
   }
 
   public function visitNode(Workflow $workflow, WorkflowNode $node): void {
@@ -39,25 +34,11 @@ class ValidStepArgsRule implements WorkflowNodeVisitor {
     }
 
     // validate args schema only for active workflows
-    if ($workflow->getStatus() !== Workflow::STATUS_ACTIVE) {
+    if ($workflow->getStatus() === Workflow::STATUS_ACTIVE) {
-      return;
-    }
-
-    try {
       $this->validator->validate($registryStep->getArgsSchema(), $step->getArgs());
-    } catch (Throwable $e) {
-      $this->errors[$step->getId()] = [
-        'step_id' => $step->getId(),
-        'message' => $e instanceof ValidationException
-          ? $e->getWpError()->get_error_message()
-          : __('Unknown error.', 'mailpoet'),
-      ];
     }
   }
 
   public function complete(Workflow $workflow): void {
-    if ($this->errors) {
-      throw Exceptions::workflowNotValid(__('Some steps have invalid arguments', 'mailpoet'), $this->errors);
-    }
   }
 }

mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepOrderRule.php

@@ -13,9 +13,6 @@ class ValidStepOrderRule implements WorkflowNodeVisitor {
   /** @var Registry */
   private $registry;
 
-  /** @var array<string, array{step_id: string, message: string}> */
-  private $errors = [];
-
   public function __construct(
     Registry $registry
   ) {
@@ -23,7 +20,6 @@ class ValidStepOrderRule implements WorkflowNodeVisitor {
   }
 
   public function initialize(Workflow $workflow): void {
-    $this->errors = [];
   }
 
   public function visitNode(Workflow $workflow, WorkflowNode $node): void {
@@ -51,22 +47,11 @@ class ValidStepOrderRule implements WorkflowNodeVisitor {
     $subjectKeys = $this->collectSubjectKeys($workflow, $node->getParents());
     $missingSubjectKeys = array_diff($requiredSubjectKeys, $subjectKeys);
     if (count($missingSubjectKeys) > 0) {
-      $missingKeysString = implode(', ', $missingSubjectKeys);
+      throw Exceptions::missingRequiredSubjects($step, $missingSubjectKeys);
-      $this->errors[$step->getId()] = [
-        'step_id' => $step->getId(),
-        'message' => sprintf(
-          // translators: %s are the missing subject keys.
-          __("Missing required subjects with keys: %s", 'mailpoet'),
-          $missingKeysString
-        ),
-      ];
     }
   }
 
   public function complete(Workflow $workflow): void {
-    if ($this->errors) {
-      throw Exceptions::workflowNotValid(__('Some steps are not valid', 'mailpoet'), $this->errors);
-    }
   }
 
   /**

mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepRule.php

@@ -0,0 +1,54 @@
+<?php declare(strict_types = 1);
+
+namespace MailPoet\Automation\Engine\Validation\WorkflowRules;
+
+use MailPoet\Automation\Engine\Data\Workflow;
+use MailPoet\Automation\Engine\Exceptions;
+use MailPoet\Automation\Engine\Exceptions\UnexpectedValueException;
+use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowNode;
+use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowNodeVisitor;
+use Throwable;
+
+class ValidStepRule implements WorkflowNodeVisitor {
+  /** @var WorkflowNodeVisitor[] */
+  private $rules;
+
+  /** @var array<string, array{step_id: string, message: string}> */
+  private $errors = [];
+
+  /** @param WorkflowNodeVisitor[] $rules */
+  public function __construct(
+    array $rules
+  ) {
+    $this->rules = $rules;
+  }
+
+  public function initialize(Workflow $workflow): void {
+    foreach ($this->rules as $rule) {
+      $rule->initialize($workflow);
+    }
+  }
+
+  public function visitNode(Workflow $workflow, WorkflowNode $node): void {
+    foreach ($this->rules as $rule) {
+      $stepId = $node->getStep()->getId();
+      try {
+        $rule->visitNode($workflow, $node);
+      } catch (UnexpectedValueException $e) {
+        $this->errors[$stepId] = ['step_id' => $stepId, 'message' => $e->getMessage()];
+      } catch (Throwable $e) {
+        $this->errors[$stepId] = ['step_id' => $stepId, 'message' => __('Unknown error.', 'mailpoet')];
+      }
+    }
+  }
+
+  public function complete(Workflow $workflow): void {
+    foreach ($this->rules as $rule) {
+      $rule->complete($workflow);
+    }
+
+    if ($this->errors) {
+      throw Exceptions::workflowNotValid(__('Some steps are not valid', 'mailpoet'), $this->errors);
+    }
+  }
+}

mailpoet/lib/Automation/Engine/Validation/WorkflowRules/ValidStepValidationRule.php

@@ -8,19 +8,14 @@ use MailPoet\Automation\Engine\Data\Workflow;
 use MailPoet\Automation\Engine\Exceptions;
 use MailPoet\Automation\Engine\Integration\Payload;
 use MailPoet\Automation\Engine\Integration\Subject;
-use MailPoet\Automation\Engine\Integration\ValidationException;
 use MailPoet\Automation\Engine\Registry;
 use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowNode;
 use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowNodeVisitor;
-use Throwable;
 
 class ValidStepValidationRule implements WorkflowNodeVisitor {
   /** @var Registry */
   private $registry;
 
-  /** @var array<string, array{step_id: string, message: string}> */
-  private $errors = [];
-
   public function __construct(
     Registry $registry
   ) {
@@ -28,7 +23,6 @@ class ValidStepValidationRule implements WorkflowNodeVisitor {
   }
 
   public function initialize(Workflow $workflow): void {
-    $this->errors = [];
   }
 
   public function visitNode(Workflow $workflow, WorkflowNode $node): void {
@@ -43,22 +37,12 @@ class ValidStepValidationRule implements WorkflowNodeVisitor {
       return;
     }
 
-    try {
     $subjects = $this->collectSubjects($workflow, $node->getParents());
     $args = new StepValidationArgs($workflow, $step, $subjects);
     $registryStep->validate($args);
-    } catch (Throwable $e) {
-      $this->errors[$step->getId()] = [
-        'step_id' => $step->getId(),
-        'message' => $e instanceof ValidationException ? $e->getMessage() : __('Unknown error.', 'mailpoet'),
-      ];
-    }
   }
 
   public function complete(Workflow $workflow): void {
-    if ($this->errors) {
-      throw Exceptions::workflowNotValid(__('Some steps are not valid', 'mailpoet'), $this->errors);
-    }
   }
 
   /**

mailpoet/lib/Automation/Engine/Validation/WorkflowValidator.php

@@ -14,6 +14,7 @@ use MailPoet\Automation\Engine\Validation\WorkflowRules\TriggersUnderRootRule;
 use MailPoet\Automation\Engine\Validation\WorkflowRules\UnknownStepRule;
 use MailPoet\Automation\Engine\Validation\WorkflowRules\ValidStepArgsRule;
 use MailPoet\Automation\Engine\Validation\WorkflowRules\ValidStepOrderRule;
+use MailPoet\Automation\Engine\Validation\WorkflowRules\ValidStepRule;
 use MailPoet\Automation\Engine\Validation\WorkflowRules\ValidStepValidationRule;
 
 class WorkflowValidator {
@@ -56,9 +57,11 @@ class WorkflowValidator {
       new NoJoinRule(),
       new NoSplitRule(),
       $this->unknownStepRule,
+      new ValidStepRule([
         $this->validStepArgsRule,
         $this->validStepOrderRule,
         $this->validStepValidationRule,
+      ]),
     ]);
   }
 }

mailpoet/tests/unit/Automation/Engine/Validation/WorkflowRules/ValidStepRuleTest.php

@@ -0,0 +1,109 @@
+<?php declare(strict_types = 1);
+
+namespace MailPoet\Automation\Engine\Validation\WorkflowRules;
+
+require_once __DIR__ . '/WorkflowRuleTest.php';
+
+use Codeception\Stub\Expected;
+use Exception;
+use MailPoet\Automation\Engine\Data\Step;
+use MailPoet\Automation\Engine\Data\Workflow;
+use MailPoet\Automation\Engine\Exceptions\UnexpectedValueException;
+use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowNodeVisitor;
+use MailPoet\Automation\Engine\Validation\WorkflowGraph\WorkflowWalker;
+
+class ValidStepRuleTest extends WorkflowRuleTest {
+  public function testItRunsStepValidation(): void {
+    $workflow = $this->getWorkflow();
+    $workflow->setStatus(Workflow::STATUS_ACTIVE);
+
+    $rule = new ValidStepRule([
+      $this->makeEmpty(WorkflowNodeVisitor::class, [
+        'initialize' => Expected::once(),
+        'visitNode' => Expected::once(),
+        'complete' => Expected::once(),
+      ])
+    ]);
+    (new WorkflowWalker())->walk($workflow, [$rule]);
+  }
+
+  public function testItCollectsRecognizedErrors(): void {
+    $workflow = $this->getWorkflow();
+    $workflow->setStatus(Workflow::STATUS_ACTIVE);
+
+    $rule = new ValidStepRule([
+      $this->makeEmpty(WorkflowNodeVisitor::class, [
+        'visitNode' => Expected::once(function () {
+          throw UnexpectedValueException::create()->withMessage('Test error');
+        }),
+      ]),
+    ]);
+
+    try {
+      (new WorkflowWalker())->walk($workflow, [$rule]);
+    } catch (UnexpectedValueException $e) {
+      $this->assertSame(
+        [
+          'root' => [
+            'step_id' => 'root',
+            'message' => 'Test error',
+          ],
+        ],
+        $e->getErrors()
+      );
+      return;
+    }
+    $this->fail(sprintf("Exception of class '%s' was not thrown.", UnexpectedValueException::class));
+  }
+
+
+  public function testItCollectsUnrecognizedErrorsWithAGenericMessage(): void {
+    $workflow = $this->getWorkflow();
+    $workflow->setStatus(Workflow::STATUS_ACTIVE);
+
+    $rule = new ValidStepRule([
+      $this->makeEmpty(WorkflowNodeVisitor::class, [
+        'visitNode' => Expected::once(function () {
+          throw new Exception(' Unknown test error');
+        }),
+      ]),
+    ]);
+
+    try {
+      (new WorkflowWalker())->walk($workflow, [$rule]);
+    } catch (UnexpectedValueException $e) {
+      $this->assertSame(
+        [
+          'root' => [
+            'step_id' => 'root',
+            'message' => 'Unknown error.',
+          ],
+        ],
+        $e->getErrors()
+      );
+      return;
+    }
+    $this->fail(sprintf("Exception of class '%s' was not thrown.", UnexpectedValueException::class));
+  }
+
+  public function testItValidatesOnlyActiveWorkflows(): void {
+    $workflow = $this->getWorkflow();
+    $workflow->setStatus(Workflow::STATUS_DRAFT);
+    $rule = new ValidStepRule([
+      $this->makeEmpty(WorkflowNodeVisitor::class, [
+        'initialize' => Expected::never(),
+        'visitNode' => Expected::never(),
+        'complete' => Expected::never(),
+      ])
+    ]);
+    (new WorkflowWalker())->walk($workflow, [$rule]);
+  }
+
+  private function getWorkflow(): Workflow {
+    return $this->make(Workflow::class, [
+      'steps' => [
+        'root' => new Step('root', 'root', 'core:root', [], []),
+      ],
+    ]);
+  }
+}