Skip to content

Commit

Permalink
Fix finder compatibilty with 4.x
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
markstory committed Apr 9, 2024
1 parent 97f49cf commit 43219fa
Show file tree
Hide file tree
Showing 3 changed files with 56 additions and 10 deletions.
37 changes: 27 additions & 10 deletions src/ORM/Table.php
Expand Up @@ -2666,22 +2666,29 @@ public function callFinder(string $type, SelectQuery $query, mixed ...$args): Se
*/
public function invokeFinder(Closure $callable, SelectQuery $query, array $args): SelectQuery
{
// TODO this is where the missing shims need to be.
$reflected = new ReflectionFunction($callable);
$params = $reflected->getParameters();
$secondParam = $params[1] ?? null;
$secondParamType = null;

$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])) {
$secondParamType = $secondParam?->getType();
$secondParamTypeName = $secondParamType instanceof ReflectionNamedType ? $secondParamType->getName() : null;
// Backwards compatibility of 4.x style finders with signature `findFoo(SelectQuery $query, array $options)`

// 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 ($secondParamIsOptions) {
if (isset($args[0])) {
deprecationWarning(
'5.0.0',
Expand All @@ -2697,8 +2704,9 @@ public function invokeFinder(Closure $callable, SelectQuery $query, array $args)
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')`
// 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',
Expand All @@ -2710,6 +2718,15 @@ public function invokeFinder(Closure $callable, SelectQuery $query, array $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());
}

if ($args) {
$query->applyOptions($args);
// Fetch custom args without the query options.
Expand Down
17 changes: 17 additions & 0 deletions tests/TestCase/ORM/TableTest.php
Expand Up @@ -1286,6 +1286,23 @@ public function testFindTypedParameters(): void
$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');
Expand Down
12 changes: 12 additions & 0 deletions tests/test_app/TestApp/Model/Table/ArticlesTable.php
Expand Up @@ -57,6 +57,18 @@ public function findWithAuthors($query, array $options = []): SelectQuery
return $query->contain('Authors');
}

/**
* 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
*/
Expand Down

0 comments on commit 43219fa

Please sign in to comment.