Browse Source

Fix finder compatibilty with 4.x

We have rector rules to update the callsites of finders to use named
parameters. What we lack is rector operations to upgrade finder
declarations to use named parameters. Because this is an impossible
task, we should have better compatibility for new style finder calls
with old style options arrays. This helps improve finder ergonomics
(less typing) for more folks.

This would have saved me a bunch of work when upgraded an application
recently and making the upgrade smoother is important to me.
Mark Story 2 years ago
parent
commit
31aecbd302

+ 36 - 32
src/ORM/Table.php

@@ -2669,45 +2669,49 @@ class Table implements RepositoryInterface, EventListenerInterface, EventDispatc
         $reflected = new ReflectionFunction($callable);
         $params = $reflected->getParameters();
         $secondParam = $params[1] ?? null;
-        $secondParamType = null;
 
-        if ($args === [] || isset($args[0])) {
-            $secondParamType = $secondParam?->getType();
-            $secondParamTypeName = $secondParamType instanceof ReflectionNamedType ? $secondParamType->getName() : null;
-            // Backwards compatibility of 4.x style finders with signature `findFoo(SelectQuery $query, array $options)`
+        $secondParamType = $secondParam?->getType();
+        $secondParamTypeName = $secondParamType instanceof ReflectionNamedType ? $secondParamType->getName() : null;
+
+        $secondParamIsOptions = (
+            count($params) === 2 &&
+            $secondParam?->name === 'options' &&
+            !$secondParam->isVariadic() &&
+            ($secondParamType === null || $secondParamTypeName === 'array')
+        );
+
+        if (($args === [] || isset($args[0])) && $secondParamIsOptions) {
+            // Backwards compatibility of 4.x style finders
+            // with signature `findFoo(SelectQuery $query, array $options)`
             // called as `find('foo')` or `find('foo', [..])`
-            if (
-                count($params) === 2 &&
-                $secondParam?->name === 'options' &&
-                !$secondParam->isVariadic() &&
-                ($secondParamType === null || $secondParamTypeName === 'array')
-            ) {
-                if (isset($args[0])) {
-                    deprecationWarning(
-                        '5.0.0',
-                        'Using options array for the `find()` call is deprecated.'
-                        . ' Use named arguments instead.'
-                    );
+            if (isset($args[0])) {
+                $args = $args[0];
+            }
+            $query->applyOptions($args);
 
-                    $args = $args[0];
-                }
+            return $callable($query, $query->getOptions());
+        }
 
-                $query->applyOptions($args);
+        // Backwards compatibility for 4.x style finders with signatures like
+        // `findFoo(SelectQuery $query, array $options)` called as
+        // `find('foo', key: $value)`.
+        if (!isset($args[0]) && $secondParamIsOptions) {
+            $query->applyOptions($args);
 
-                return $callable($query, $query->getOptions());
-            }
+            return $callable($query, $query->getOptions());
+        }
 
-            // Backwards compatibility for core finders like `findList()` called in 4.x style
-            // with an array `find('list', ['valueField' => 'foo'])` instead of `find('list', valueField: 'foo')`
-            if (isset($args[0]) && is_array($args[0]) && $secondParamTypeName !== 'array') {
-                deprecationWarning(
-                    '5.0.0',
-                    "Calling `{$reflected->getName()}` finder with options array is deprecated."
-                     . ' Use named arguments instead.'
-                );
+        // Backwards compatibility for core finders like `findList()` called in 4.x
+        // style with an array `find('list', ['valueField' => 'foo'])` instead of
+        // `find('list', valueField: 'foo')`
+        if (isset($args[0]) && is_array($args[0]) && $secondParamTypeName !== 'array') {
+            deprecationWarning(
+                '5.0.0',
+                "Calling `{$reflected->getName()}` finder with options array is deprecated."
+                 . ' Use named arguments instead.'
+            );
 
-                $args = $args[0];
-            }
+            $args = $args[0];
         }
 
         if ($args) {

+ 17 - 0
tests/TestCase/ORM/TableTest.php

@@ -1286,6 +1286,23 @@ class TableTest extends TestCase
         $this->assertSame(2, $author->id);
     }
 
+    public function testFindTypedParameterCompatibility(): void
+    {
+        $articles = $this->fetchTable('Articles');
+        $article = $articles->find('titled')->first();
+        $this->assertNotEmpty($article);
+
+        // Options arrays should work
+        $article = $articles->find('titled', ['title' => 'Second Article'])->first();
+        $this->assertNotEmpty($article);
+        $this->assertEquals('Second Article', $article->title);
+
+        // Named parameters should be compatible with options finders
+        $article = $articles->find('titled', title: 'Second Article')->first();
+        $this->assertNotEmpty($article);
+        $this->assertEquals('Second Article', $article->title);
+    }
+
     public function testFindForFinderVariadic(): void
     {
         $testTable = $this->fetchTable('Test');

+ 12 - 0
tests/test_app/TestApp/Model/Table/ArticlesTable.php

@@ -58,6 +58,18 @@ class ArticlesTable extends Table
     }
 
     /**
+     * Finder for testing named parameter compatibility
+     */
+    public function findTitled(SelectQuery $query, array $options): SelectQuery
+    {
+        if (!empty($options['title'])) {
+            $query->where(['Articles.title' => $options['title']]);
+        }
+
+        return $query;
+    }
+
+    /**
      * Example public method
      */
     public function doSomething(): void