Simplify SubjectTransformer

[MAILPOET-4935]

mailpoet/lib/AdminPages/Pages/AutomationEditor.php

@@ -101,7 +101,7 @@ class AutomationEditor {
       $steps[$key] = [
         'key' => $step->getKey(),
         'name' => $step->getName(),
-        'subject_keys' => $step instanceof Trigger ? $this->subjectTransformerHandler->subjectKeysForTrigger($step) : $step->getSubjectKeys(),
+        'subject_keys' => $step instanceof Trigger ? $this->subjectTransformerHandler->getSubjectKeysForTrigger($step) : $step->getSubjectKeys(),
         'args_schema' => $step->getArgsSchema()->toArray(),
       ];
     }

mailpoet/lib/Automation/Engine/Control/SubjectTransformerHandler.php

@@ -5,8 +5,6 @@ namespace MailPoet\Automation\Engine\Control;
 use MailPoet\Automation\Engine\Data\Automation;
 use MailPoet\Automation\Engine\Data\Step as StepData;
 use MailPoet\Automation\Engine\Data\Subject;
-use MailPoet\Automation\Engine\Integration\Step;
-use MailPoet\Automation\Engine\Integration\SubjectTransformer;
 use MailPoet\Automation\Engine\Integration\Trigger;
 use MailPoet\Automation\Engine\Registry;
 
@@ -21,181 +19,85 @@ class SubjectTransformerHandler {
     $this->registry = $registry;
   }
 
-  public function subjectKeysForTrigger(Trigger $trigger): array {
+  public function getSubjectKeysForAutomation(Automation $automation): array {
-    $subjectKeys = $trigger->getSubjectKeys();
+    $triggerData = array_values(array_filter(
-    $possibleKeys = [];
-    foreach ($subjectKeys as $key) {
-      $possibleKeys = array_merge($possibleKeys, $this->getPossibleTransformations($key));
-    }
-    return array_unique(array_values(array_merge($subjectKeys, $possibleKeys)));
-  }
-
-  /** @return string[] */
-  public function subjectKeysForAutomation(Automation $automation): array {
-    $configuredTriggers = array_filter(
       $automation->getSteps(),
       function (StepData $step): bool {
         return $step->getType() === StepData::TYPE_TRIGGER;
       }
-    );
-
-    $triggers = array_filter(array_map(
-      function(StepData $step): ?Step {
-        return $this->registry->getStep($step->getKey());
-      },
-      $configuredTriggers
     ));
 
-    $triggerKeys = [];
+    $triggers = array_filter(array_map(
+      function (StepData $step): ?Trigger {
+        return $this->registry->getTrigger($step->getKey());
+      },
+      $triggerData
+    ));
+    $all = [];
     foreach ($triggers as $trigger) {
-      $triggerKeys[$trigger->getKey()] = $trigger->getSubjectKeys();
+      $all[] = $this->getSubjectKeysForTrigger($trigger);
     }
-    if (!$triggerKeys) {
+    $all = count($all) > 1 ? array_intersect(...$all) : $all[0] ?? [];
-      return [];
+    return array_values(array_unique($all));
-    }
-
-    $triggerKeys = count($triggerKeys) === 1 ? current($triggerKeys) : array_intersect(...array_values($triggerKeys));
-    $possibleKeys = [];
-    foreach ($triggerKeys as $key) {
-      $possibleKeys = array_merge($possibleKeys, $this->getPossibleTransformations($key));
-    }
-    return array_unique(array_values(array_merge($triggerKeys, $possibleKeys)));
   }
 
-  private function getPossibleTransformations(string $key, string $stopKey = null): array {
+  public function getSubjectKeysForTrigger(Trigger $trigger): array {
-    if (!$stopKey) {
+    $all = [];
-      $stopKey = $key;
+    foreach ($trigger->getSubjectKeys() as $key) {
+      $all = array_merge($all, $this->getSubjectKeysForSingleKey($key));
     }
-    $allTransformer = $this->registry->getSubjectTransformer();
+    return $all;
-    $possibleTransformer = array_filter(
-      $allTransformer,
-      function (SubjectTransformer $transformer) use ($key): bool {
-        return $transformer->accepts() === $key;
-      }
-    );
-
-    $possibleKeys = [];
-    foreach ($possibleTransformer as $transformer) {
-      $possibleKey = $transformer->returns();
-      if ($possibleKey === $stopKey) {
-        continue;
-      }
-      $possibleKeys[$possibleKey] = $possibleKey;
-      $possibleKeys = array_merge(
-        $possibleKeys,
-        $this->getPossibleTransformations($possibleKey, $stopKey)
-      );
-    }
-    return $possibleKeys;
   }
 
-  /**
+  public function getAllSubjects(Subject ...$subjects): array {
-   * @param Trigger $trigger
+    $transformerMap = $this->getTransformerMap();
-   * @param Subject ...$subjects
+    $all = [];
-   * @return Subject[]
-   */
-  public function provideAllSubjects(Trigger $trigger, Subject ...$subjects): array {
-    $allSubjectsKeys = $this->subjectKeysForTrigger($trigger);
-    $allSubjectKeyTargets = array_filter(
-      array_diff($allSubjectsKeys, array_map(
-      function(Subject $subject): string {
-        return $subject->getKey();
-      },
-      $subjects
-      )),
-    function(string $target) use ($subjects): bool {
-      return $this->canSubjectsTransformTo($target, ...$subjects);
-    }
-    );
-
-    $allSubjects = [];
-    foreach ($subjects as $existingSubject) {
-      $allSubjects[$existingSubject->getKey()] = $existingSubject;
-    }
-    foreach ($allSubjectKeyTargets as $target) {
-      if (isset($allSubjects[$target])) {
-        continue;
-      }
-      foreach ($subjects as $subject) {
-        $transformedSubject = $this->transformSubjectTo($subject, $target);
-        if ($transformedSubject) {
-          $allSubjects[$transformedSubject->getKey()] = $transformedSubject;
-        }
-      }
-    }
-
-    $allSubjectsTransformed = true;
-    do {
-      foreach ($allSubjectKeyTargets as $key) {
-        if (!isset($allSubjects[$key])) {
-          $allSubjectsTransformed = false;
-          break;
-        }
-      }
-      $allSubjects = $allSubjectsTransformed ? $allSubjects : $this->provideAllSubjects($trigger, ...array_values($allSubjects));
-    } while (!$allSubjectsTransformed);
-    return array_values($allSubjects);
-  }
-
-  private function transformSubjectTo(Subject $subject, string $target): ?Subject {
-    $transformerChain = $this->getTransformerChain($subject->getKey(), $target);
-    if (!$transformerChain) {
-      return null;
-    }
-    foreach ($transformerChain as $transformer) {
-      $subject = $transformer->transform($subject);
-    }
-    return $subject;
-  }
-
-  /**
-   * @return SubjectTransformer[]
-   */
-  private function getTransformerChain(string $from, string $to): array {
-    $transformers = $this->registry->getSubjectTransformer();
-    $transformerChain = [];
-
-    //walk the graph of transformers to find the shortest path
-    $queue = [];
-    $queue[] = [$from, []];
-    while (count($queue) > 0) {
-      $current = array_shift($queue);
-      $currentKey = $current[0];
-      $currentPath = $current[1];
-      if ($currentKey === $to) {
-        $transformerChain = $currentPath;
-        break;
-      }
-      foreach ($transformers as $transformer) {
-        if ($transformer->accepts() === $currentKey) {
-          $newPath = $currentPath;
-          $newPath[] = $transformer;
-          $queue[] = [$transformer->returns(), $newPath];
-        }
-      }
-    }
-
-    return $transformerChain;
-  }
-
-  private function canSubjectsTransformTo(string $target, Subject ...$subjects): bool {
-    if (
-      in_array($target, array_map(
-      function(Subject $subject): string {
-        return $subject->getKey();
-      },
-      $subjects
-      ))
-    ) {
-      return true;
-    }
-
     foreach ($subjects as $subject) {
-      $possibleTransformations = $this->getPossibleTransformations($subject->getKey());
+      $all[$subject->getKey()] = $subject;
-      if (in_array($target, $possibleTransformations)) {
+    }
-        return true;
+
+    $queue = array_map(
+      function(Subject $subject): string {
+        return $subject->getKey();
+      },
+      $subjects
+    );
+    while ($key = array_shift($queue)) {
+      foreach ($transformerMap[$key] ?? [] as $transformer) {
+          $newKey = $transformer->returns();
+        if (!isset($all[$newKey])) {
+          $all[$newKey] = $transformer->transform($all[$key]);
+          $queue[] = $newKey;
+        }
       }
     }
-    return false;
+    return array_values($all);
+  }
+
+  private function getTransformerMap(): array {
+    $transformerMap = [];
+    foreach ($this->registry->getSubjectTransformer() as $transformer) {
+      $transformerMap[$transformer->accepts()] = array_merge($transformerMap[$transformer->accepts()] ?? [], [$transformer]);
+    }
+    return $transformerMap;
+  }
+
+  /**
+   * @return string[]
+   */
+  private function getSubjectKeysForSingleKey(string $subjectKey): array {
+    $transformerMap = $this->getTransformerMap();
+    $all = [$subjectKey];
+    $queue = [$subjectKey];
+    while ($key = array_shift($queue)) {
+      foreach ($transformerMap[$key] ?? [] as $transformer) {
+        $newKey = $transformer->returns();
+        if (!in_array($newKey, $all, true)) {
+          $all[] = $newKey;
+          $queue[] = $newKey;
+        }
+      }
+    }
+    return $all;
   }
 }

mailpoet/lib/Automation/Engine/Control/TriggerHandler.php

@@ -59,7 +59,7 @@ class TriggerHandler {
 
   /** @param Subject[] $subjects */
   public function processTrigger(Trigger $trigger, array $subjects): void {
-    $subjects = $this->subjectTransformerHandler->provideAllSubjects($trigger, ...$subjects);
+    $subjects = $this->subjectTransformerHandler->getAllSubjects(...$subjects);
     $automations = $this->automationStorage->getActiveAutomationsByTrigger($trigger);
     foreach ($automations as $automation) {
       $step = $automation->getTrigger($trigger->getKey());

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

@@ -45,7 +45,7 @@ class ValidStepOrderRule implements AutomationNodeVisitor {
       return;
     }
 
-    $subjectKeys = $this->subjectTransformerHandler->subjectKeysForAutomation($automation);
+    $subjectKeys = $this->subjectTransformerHandler->getSubjectKeysForAutomation($automation);
     $missingSubjectKeys = array_diff($requiredSubjectKeys, $subjectKeys);
     if (count($missingSubjectKeys) > 0) {
       throw Exceptions::missingRequiredSubjects($step, $missingSubjectKeys);

mailpoet/tests/unit/Automation/Engine/Control/SubjectTransformerHandlerTest.php

@@ -40,7 +40,7 @@ class SubjectTransformerHandlerTest extends MailPoetUnitTest {
     $trigger->expects($this->any())->method('getSubjectKeys')->willReturn([$triggerSubject]);
     $registry = $this->createMock(Registry::class);
     $registry->expects($this->any())->method('getSubjectTransformer')->willReturn($transformers);
-    $registry->expects($this->any())->method('getStep')->willReturnCallback(function($key) use ($trigger){
+    $registry->expects($this->any())->method('getTrigger')->willReturnCallback(function($key) use ($trigger){
       return $key === 'trigger' ? $trigger : null;
     });
     $testee = new SubjectTransformerHandler($registry);
@@ -50,8 +50,8 @@ class SubjectTransformerHandlerTest extends MailPoetUnitTest {
     $triggerData->expects($this->any())->method('getKey')->willReturn('trigger');
     $automation = $this->createMock(Automation::class);
     $automation->method('getSteps')->willReturn([$triggerData]);
-    $result = $testee->subjectKeysForAutomation($automation);
+    $result = $testee->getSubjectKeysForAutomation($automation);
-    $this->assertEquals(['subject_a', 'subject_b1', 'subject_c1', 'subject_d1', 'subject_b2', 'subject_c2'], $result);
+    $this->assertEquals(['subject_a', 'subject_b1', 'subject_b2', 'subject_c1', 'subject_c2', 'subject_d1'], $result);
   }
 
   public function testItDoesNotRunInfiniteWhileFindingAllSubjects(): void {
@@ -75,7 +75,7 @@ class SubjectTransformerHandlerTest extends MailPoetUnitTest {
     $trigger->expects($this->any())->method('getSubjectKeys')->willReturn([$triggerSubject]);
     $registry = $this->createMock(Registry::class);
     $registry->expects($this->any())->method('getSubjectTransformer')->willReturn($transformers);
-    $registry->expects($this->any())->method('getStep')->willReturnCallback(function($key) use ($trigger){
+    $registry->expects($this->any())->method('getTrigger')->willReturnCallback(function($key) use ($trigger){
       return $key === 'trigger' ? $trigger : null;
     });
     $testee = new SubjectTransformerHandler($registry);
@@ -85,7 +85,7 @@ class SubjectTransformerHandlerTest extends MailPoetUnitTest {
     $triggerData->expects($this->any())->method('getKey')->willReturn('trigger');
     $automation = $this->createMock(Automation::class);
     $automation->method('getSteps')->willReturn([$triggerData]);
-    $result = $testee->subjectKeysForAutomation($automation);
+    $result = $testee->getSubjectKeysForAutomation($automation);
     $this->assertEquals(['subject_a', 'subject_b', 'subject_c'], $result);
   }
 
@@ -102,7 +102,7 @@ class SubjectTransformerHandlerTest extends MailPoetUnitTest {
 
     $registry = $this->createMock(Registry::class);
     $registry->expects($this->any())->method('getSubjectTransformer')->willReturn([]);
-    $registry->expects($this->any())->method('getStep')->willReturnCallback(function($key) use ($trigger1, $trigger2){
+    $registry->expects($this->any())->method('getTrigger')->willReturnCallback(function($key) use ($trigger1, $trigger2){
       if ($key === 'trigger1') {
         return $trigger1;
       }
@@ -123,7 +123,7 @@ class SubjectTransformerHandlerTest extends MailPoetUnitTest {
 
     $automation = $this->createMock(Automation::class);
     $automation->method('getSteps')->willReturn([$trigger1Data, $trigger2Data]);
-    $result = $testee->subjectKeysForAutomation($automation);
+    $result = $testee->getSubjectKeysForAutomation($automation);
     $this->assertEquals(['b', 'c'], $result);
   }
 
@@ -164,7 +164,7 @@ class SubjectTransformerHandlerTest extends MailPoetUnitTest {
     $trigger->expects($this->any())->method('getSubjectKeys')->willReturn(['from']);
 
     $registry = $this->createMock(Registry::class);
-    $registry->expects($this->any())->method('getStep')->willReturnCallback(
+    $registry->expects($this->any())->method('getTrigger')->willReturnCallback(
       function($key) {
         if ($key !== 'trigger') {
           return null;
@@ -180,7 +180,7 @@ class SubjectTransformerHandlerTest extends MailPoetUnitTest {
     $testee = new SubjectTransformerHandler($registry);
 
     $subject = new Subject('from', ['key' => 'value']);
-    $subjects = $testee->provideAllSubjects($trigger, $subject);
+    $subjects = $testee->getAllSubjects($subject);
     $this->assertNotNull($subjects);
     $this->assertCount(3, $subjects);
     $this->assertSame(['from', 'middle', 'to'], array_map(function(Subject $subject): string { return $subject->getKey();

mailpoet/tests/unit/Automation/Engine/Validation/AutomationRules/ValidStepOrderRuleTest.php

@@ -35,7 +35,7 @@ class ValidStepOrderRuleTest extends AutomationRuleTest {
     ]);
 
     $subjectTransformer = $this->createMock(SubjectTransformerHandler::class);
-    $subjectTransformer->expects($this->any())->method('subjectKeysForAutomation')->willReturnCallback(function($a) use ($automation) {
+    $subjectTransformer->expects($this->any())->method('getSubjectKeysForAutomation')->willReturnCallback(function($a) use ($automation) {
       return $a === $automation ? ['subject-a'] : [];
     });
     $rule = new ValidStepOrderRule($registry, $subjectTransformer);
@@ -63,7 +63,7 @@ class ValidStepOrderRuleTest extends AutomationRuleTest {
     ]);
 
     $subjectTransformer = $this->createMock(SubjectTransformerHandler::class);
-    $subjectTransformer->expects($this->any())->method('subjectKeysForAutomation')->willReturnCallback(function($a) use ($automation) {
+    $subjectTransformer->expects($this->any())->method('getSubjectKeysForAutomation')->willReturnCallback(function($a) use ($automation) {
       return $a === $automation ? ['subject-a'] : [];
     });
 
@@ -90,7 +90,7 @@ class ValidStepOrderRuleTest extends AutomationRuleTest {
     ]);
 
     $subjectTransformer = $this->createMock(SubjectTransformerHandler::class);
-    $subjectTransformer->expects($this->any())->method('subjectKeysForAutomation')->willReturnCallback(function($a) use ($automation) {
+    $subjectTransformer->expects($this->any())->method('getSubjectKeysForAutomation')->willReturnCallback(function($a) use ($automation) {
       return $a === $automation ? ['subject-a'] : [];
     });
     $rule = new ValidStepOrderRule($registry, $subjectTransformer);
@@ -113,7 +113,7 @@ class ValidStepOrderRuleTest extends AutomationRuleTest {
     ]);
 
     $subjectTransformer = $this->createMock(SubjectTransformerHandler::class);
-    $subjectTransformer->expects($this->any())->method('subjectKeysForAutomation')->willReturnCallback(function($a) use ($automation) {
+    $subjectTransformer->expects($this->any())->method('getSubjectKeysForAutomation')->willReturnCallback(function($a) use ($automation) {
       return $a === $automation ? ['subject-a', 'subject-b'] : [];
     });
 
@@ -140,7 +140,7 @@ class ValidStepOrderRuleTest extends AutomationRuleTest {
     ]);
 
     $subjectTransformer = $this->createMock(SubjectTransformerHandler::class);
-    $subjectTransformer->expects($this->any())->method('subjectKeysForAutomation')->willReturnCallback(function($a) use ($automation) {
+    $subjectTransformer->expects($this->any())->method('getSubjectKeysForAutomation')->willReturnCallback(function($a) use ($automation) {
       return $a === $automation ? ['subject-a', 'subject-b'] : [];
     });
     $rule = new ValidStepOrderRule($registry, $subjectTransformer);