From 9678b11c012ad25cfcadc19eb405ce922491e748 Mon Sep 17 00:00:00 2001 From: Nicos Panayides Date: Mon, 3 Feb 2025 15:57:23 +0200 Subject: [PATCH 1/6] Added test for implicit conditions and fix detection of unaliased identifiers in the query --- src/Model/Behavior/TrashBehavior.php | 15 +++++++++------ .../TestCase/Model/Behavior/TrashBehaviorTest.php | 11 +++++++++++ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/Model/Behavior/TrashBehavior.php b/src/Model/Behavior/TrashBehavior.php index 6a58019..db5160e 100644 --- a/src/Model/Behavior/TrashBehavior.php +++ b/src/Model/Behavior/TrashBehavior.php @@ -6,8 +6,7 @@ use ArrayObject; use Cake\Core\Configure; use Cake\Core\Exception\CakeException; -use Cake\Database\Expression\BetweenExpression; -use Cake\Database\Expression\ComparisonExpression; +use Cake\Database\Expression\FieldInterface; use Cake\Database\Expression\IdentifierExpression; use Cake\Database\Expression\QueryExpression; use Cake\Database\Expression\UnaryExpression; @@ -184,13 +183,15 @@ public function beforeFind(EventInterface $event, SelectQuery $query, ArrayObjec $addCondition = true; $query->traverseExpressions(function ($expression) use (&$addCondition, $field): void { - if (!$addCondition) { + if ($addCondition === false) { return; } if ( $expression instanceof IdentifierExpression - && $expression->getIdentifier() === $field + && ($expression->getIdentifier() === $field + || $this->table()->aliasField($expression->getIdentifier()) == $field + ) ) { $addCondition = false; @@ -198,8 +199,10 @@ public function beforeFind(EventInterface $event, SelectQuery $query, ArrayObjec } if ( - ($expression instanceof ComparisonExpression || $expression instanceof BetweenExpression) - && $expression->getField() === $field + $expression instanceof FieldInterface + && ($expression->getField() === $field + || $this->table()->aliasField($expression->getField()) == $field + ) ) { $addCondition = false; } diff --git a/tests/TestCase/Model/Behavior/TrashBehaviorTest.php b/tests/TestCase/Model/Behavior/TrashBehaviorTest.php index e3f886d..1e94cea 100644 --- a/tests/TestCase/Model/Behavior/TrashBehaviorTest.php +++ b/tests/TestCase/Model/Behavior/TrashBehaviorTest.php @@ -361,6 +361,17 @@ public function testTrashNonAccessibleProperty() $this->assertCount(3, $this->Articles->find('withTrashed')); } + public function testFindWithImplicitCondition() + { + $this->assertCount(2, $this->Articles->find()->where([ + 'trashed IS NOT' => null + ])); + + $this->assertCount(2, $this->Articles->find()->where([ + 'Articles.trashed IS NOT' => null + ])); + } + /** * Test it can find only trashed records. * From ecadfc681c2100a545cdbadfcb5bd26cbca7e475 Mon Sep 17 00:00:00 2001 From: Nicos Panayides Date: Mon, 3 Feb 2025 16:06:20 +0200 Subject: [PATCH 2/6] use strict comparison --- src/Model/Behavior/TrashBehavior.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Model/Behavior/TrashBehavior.php b/src/Model/Behavior/TrashBehavior.php index db5160e..4ae7e59 100644 --- a/src/Model/Behavior/TrashBehavior.php +++ b/src/Model/Behavior/TrashBehavior.php @@ -190,7 +190,7 @@ public function beforeFind(EventInterface $event, SelectQuery $query, ArrayObjec if ( $expression instanceof IdentifierExpression && ($expression->getIdentifier() === $field - || $this->table()->aliasField($expression->getIdentifier()) == $field + || $this->table()->aliasField($expression->getIdentifier()) === $field ) ) { $addCondition = false; @@ -201,7 +201,7 @@ public function beforeFind(EventInterface $event, SelectQuery $query, ArrayObjec if ( $expression instanceof FieldInterface && ($expression->getField() === $field - || $this->table()->aliasField($expression->getField()) == $field + || $this->table()->aliasField($expression->getField()) === $field ) ) { $addCondition = false; From 54a6e91f261783d3fb865832b053ac6ea482973d Mon Sep 17 00:00:00 2001 From: Nicos Panayides Date: Mon, 3 Feb 2025 16:14:34 +0200 Subject: [PATCH 3/6] Compare the identifiers with aliases and unaliased field to avoid issues when the field is not a simple string. --- src/Model/Behavior/TrashBehavior.php | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/Model/Behavior/TrashBehavior.php b/src/Model/Behavior/TrashBehavior.php index 4ae7e59..6244ce2 100644 --- a/src/Model/Behavior/TrashBehavior.php +++ b/src/Model/Behavior/TrashBehavior.php @@ -179,19 +179,18 @@ public function trash(EntityInterface $entity, array $options = []): bool */ public function beforeFind(EventInterface $event, SelectQuery $query, ArrayObject $options, bool $primary): void { - $field = $this->getTrashField(); + $field = $this->getTrashField(false); + $fieldIdentifiers = [ $field, $this->table()->aliasField($field)]; $addCondition = true; - $query->traverseExpressions(function ($expression) use (&$addCondition, $field): void { + $query->traverseExpressions(function ($expression) use (&$addCondition, $fieldIdentifiers): void { if ($addCondition === false) { return; } if ( $expression instanceof IdentifierExpression - && ($expression->getIdentifier() === $field - || $this->table()->aliasField($expression->getIdentifier()) === $field - ) + && in_array($expression->getIdentifier(), $fieldIdentifiers, true) ) { $addCondition = false; @@ -200,9 +199,7 @@ public function beforeFind(EventInterface $event, SelectQuery $query, ArrayObjec if ( $expression instanceof FieldInterface - && ($expression->getField() === $field - || $this->table()->aliasField($expression->getField()) === $field - ) + && in_array($expression->getField(), $fieldIdentifiers, true) ) { $addCondition = false; } From e90f5a2095f9cd600b217a84b0700f290c10c61b Mon Sep 17 00:00:00 2001 From: Nicos Panayides Date: Mon, 3 Feb 2025 16:25:19 +0200 Subject: [PATCH 4/6] phpcs fixes --- tests/TestCase/Model/Behavior/TrashBehaviorTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/TestCase/Model/Behavior/TrashBehaviorTest.php b/tests/TestCase/Model/Behavior/TrashBehaviorTest.php index 1e94cea..3365d72 100644 --- a/tests/TestCase/Model/Behavior/TrashBehaviorTest.php +++ b/tests/TestCase/Model/Behavior/TrashBehaviorTest.php @@ -364,11 +364,11 @@ public function testTrashNonAccessibleProperty() public function testFindWithImplicitCondition() { $this->assertCount(2, $this->Articles->find()->where([ - 'trashed IS NOT' => null + 'trashed IS NOT' => null, ])); $this->assertCount(2, $this->Articles->find()->where([ - 'Articles.trashed IS NOT' => null + 'Articles.trashed IS NOT' => null, ])); } From 5d8ce1983c140e4a71dd684b39f9f16aada0ec75 Mon Sep 17 00:00:00 2001 From: Nicos Panayides Date: Tue, 4 Feb 2025 09:03:28 +0200 Subject: [PATCH 5/6] Update src/Model/Behavior/TrashBehavior.php Co-authored-by: ADmad --- src/Model/Behavior/TrashBehavior.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Model/Behavior/TrashBehavior.php b/src/Model/Behavior/TrashBehavior.php index 1215a7d..c21ef32 100644 --- a/src/Model/Behavior/TrashBehavior.php +++ b/src/Model/Behavior/TrashBehavior.php @@ -201,7 +201,7 @@ protected function shouldAddTrashCondition(SelectQuery $query): bool $addCondition = true; $query->traverseExpressions(function ($expression) use (&$addCondition, $fieldIdentifiers): void { - if ($addCondition === false) { + if (!$addCondition) { return; } From c556a39041f105d35368df5f37479096ff52dd75 Mon Sep 17 00:00:00 2001 From: Nicos Panayides Date: Tue, 4 Feb 2025 09:03:36 +0200 Subject: [PATCH 6/6] Update src/Model/Behavior/TrashBehavior.php Co-authored-by: ADmad --- src/Model/Behavior/TrashBehavior.php | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Model/Behavior/TrashBehavior.php b/src/Model/Behavior/TrashBehavior.php index c21ef32..3d483f9 100644 --- a/src/Model/Behavior/TrashBehavior.php +++ b/src/Model/Behavior/TrashBehavior.php @@ -196,8 +196,7 @@ public function beforeFind(EventInterface $event, SelectQuery $query, ArrayObjec */ protected function shouldAddTrashCondition(SelectQuery $query): bool { - $field = $this->getTrashField(false); - $fieldIdentifiers = [ $field, $this->table()->aliasField($field)]; + $fieldIdentifiers = [$this->getTrashField(false), $this->getTrashField()]; $addCondition = true; $query->traverseExpressions(function ($expression) use (&$addCondition, $fieldIdentifiers): void {