Merge pull request #377 from cakephp/fix-event-manager-rector-idempotency

Fix EventManagerOnRector idempotency issue

Author: LordSimal

src/Rector/Cake6/EventManagerOnRector.php

@@ -4,6 +4,9 @@
 namespace Cake\Upgrade\Rector\Cake6;
 
 use PhpParser\Node;
+use PhpParser\Node\Expr\Array_;
+use PhpParser\Node\Expr\ArrowFunction;
+use PhpParser\Node\Expr\Closure;
 use PhpParser\Node\Expr\MethodCall;
 use PHPStan\Type\ObjectType;
 use Rector\Rector\AbstractRector;
@@ -63,6 +66,19 @@ public function refactor(Node $node): ?Node
             return null;
         }
 
+        $secondArgValue = $node->args[1]->value;
+        $thirdArgValue = $node->args[2]->value;
+
+        // Skip if already transformed - 2nd arg is a callable expression (new format)
+        if ($this->isCallableExpression($secondArgValue)) {
+            return null;
+        }
+
+        // Skip if already transformed - 3rd arg is array literal (new format)
+        if ($thirdArgValue instanceof Array_) {
+            return null;
+        }
+
         // Swap the 2nd and 3rd arguments
         $secondArg = $node->args[1];
         $thirdArg = $node->args[2];
@@ -72,4 +88,12 @@ public function refactor(Node $node): ?Node
 
         return $node;
     }
+
+    /**
+     * Check if the expression is a callable (Closure or ArrowFunction).
+     */
+    private function isCallableExpression(Node\Expr $expr): bool
+    {
+        return $expr instanceof Closure || $expr instanceof ArrowFunction;
+    }
 }

tests/TestCase/Rector/MethodCall/EventManagerOnRector/Fixture/fixture.php.inc

@@ -27,6 +27,9 @@ class Fixture
         // Should be transformed - 3 arguments with variable
         $options = ['priority' => 100];
         $eventManager->on('Model.beforeFind', $options, $handler);
+
+        // Should be transformed - 3 arguments with arrow function as 3rd arg
+        $eventManager->on('Model.beforeValidate', ['priority' => 50], fn($event) => true);
     }
 }
 
@@ -61,6 +64,9 @@ class Fixture
         // Should be transformed - 3 arguments with variable
         $options = ['priority' => 100];
         $eventManager->on('Model.beforeFind', $handler, $options);
+
+        // Should be transformed - 3 arguments with arrow function as 3rd arg
+        $eventManager->on('Model.beforeValidate', fn($event) => true, ['priority' => 50]);
     }
 }
 

tests/TestCase/Rector/MethodCall/EventManagerOnRector/Fixture/skip_already_transformed.php.inc

@@ -0,0 +1,26 @@
+<?php
+
+namespace Cake\Upgrade\Test\TestCase\Rector\MethodCall\EventManagerOnRector\Fixture;
+
+use Cake\Event\EventManager;
+
+class SkipAlreadyTransformed
+{
+    public function run()
+    {
+        $eventManager = EventManager::instance();
+
+        // Should NOT be transformed - already in new format with closure as 2nd arg
+        $eventManager->on('Model.beforeSave', function ($event) {
+            return true;
+        }, ['priority' => 90]);
+
+        // Should NOT be transformed - already in new format with arrow function as 2nd arg
+        $eventManager->on('Controller.initialize', fn($event) => true, ['priority' => 10]);
+
+        // Should NOT be transformed - already in new format with variable callable
+        $eventManager->on('Model.beforeFind', $callable, ['priority' => 100]);
+    }
+}
+
+?>