From 699129bd84238a35fb93ab54efc62d085fb5c4af Mon Sep 17 00:00:00 2001 From: vlakoff Date: Wed, 27 Oct 2021 07:05:41 +0200 Subject: [PATCH 1/8] Add variable $op, in order to not alter variable $k Conceptually cleaner, and will be helpful for the next commit. --- system/Database/BaseBuilder.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index f87f21bf7308..a2c407194ed2 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -666,9 +666,9 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type $bind = $this->setBind($k, $v, $escape); if (empty($op)) { - $k .= ' ='; + $op = ' ='; } else { - $k .= " {$op}"; + $op = " {$op}"; } if ($v instanceof Closure) { @@ -679,13 +679,16 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type } } elseif (! $this->hasOperator($k) && $qbKey !== 'QBHaving') { // value appears not to have been set, assign the test to IS NULL - $k .= ' IS NULL'; + $op = ' IS NULL'; } elseif (preg_match('/\s*(!?=|<>|IS(?:\s+NOT)?)\s*$/i', $k, $match, PREG_OFFSET_CAPTURE)) { - $k = substr($k, 0, $match[0][1]) . ($match[1][0] === '=' ? ' IS NULL' : ' IS NOT NULL'); + $k = substr($k, 0, $match[0][1]); + $op = $match[1][0] === '=' ? ' IS NULL' : ' IS NOT NULL'; + } else { + $op = ''; } $this->{$qbKey}[] = [ - 'condition' => $prefix . $k . $v, + 'condition' => $prefix . $k . $op . $v, 'escape' => $escape, ]; } From 223ca28400a95594490052b032e1a921237138fb Mon Sep 17 00:00:00 2001 From: vlakoff Date: Wed, 27 Oct 2021 07:34:15 +0200 Subject: [PATCH 2/8] Do not inappropriately register bind when the value is a closure Such closure just has nothing to do in the $binds array. The code currently works because the binds compiler determines the binds to apply not by looking at the $binds array, but by looking at the query string, so it doesn't reach the closure. If we modify the binds compiler to instead look at the $binds array (which makes more sense), it would attempt to replace this registered named bind in the query string, and trying to replace with a closure would trigger a "cannot cast to string" error. --- system/Database/BaseBuilder.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index a2c407194ed2..51066398be33 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -663,8 +663,6 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type } } - $bind = $this->setBind($k, $v, $escape); - if (empty($op)) { $op = ' ='; } else { @@ -675,7 +673,8 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type $builder = $this->cleanClone(); $v = '(' . str_replace("\n", ' ', $v($builder)->getCompiledSelect()) . ')'; } else { - $v = " :{$bind}:"; + $bind = $this->setBind($k, $v, $escape); + $v = " :{$bind}:"; } } elseif (! $this->hasOperator($k) && $qbKey !== 'QBHaving') { // value appears not to have been set, assign the test to IS NULL From 0ae8a670b2b90a89745c201f3846496b25128869 Mon Sep 17 00:00:00 2001 From: vlakoff Date: Wed, 27 Oct 2021 07:41:15 +0200 Subject: [PATCH 3/8] Code simplification We can simply merge these two blocks. --- system/Database/BaseBuilder.php | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index 51066398be33..f393c6f3874d 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -661,12 +661,9 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type if (substr($k, -1 * strlen($op)) === $op) { $k = rtrim(strrev(preg_replace(strrev('/' . $op . '/'), strrev(''), strrev($k), 1))); } - } - - if (empty($op)) { - $op = ' ='; - } else { $op = " {$op}"; + } else { + $op = ' ='; } if ($v instanceof Closure) { From abd0f7a93f314d4f3ce35fe5aac34eb4541db9f9 Mon Sep 17 00:00:00 2001 From: vlakoff Date: Wed, 27 Oct 2021 07:48:56 +0200 Subject: [PATCH 4/8] Another code simplification Exact same effect: this complicated code was for replacing the last occurrence (there is no built-in function for this), and there is an englobing condition to ensure we replace the substring $op only if it is at the very end of $k. As a bonus, previous code was missing a preg_quote(), but we no longer need it. --- system/Database/BaseBuilder.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index f393c6f3874d..0d0c21e75b3f 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -658,8 +658,8 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type $op = trim(current($op)); - if (substr($k, -1 * strlen($op)) === $op) { - $k = rtrim(strrev(preg_replace(strrev('/' . $op . '/'), strrev(''), strrev($k), 1))); + if (substr($k, -strlen($op)) === $op) { + $k = rtrim(substr($k, 0, -strlen($op))); } $op = " {$op}"; } else { From 3e8903aa306e0cb6c3638b054299eda22b53881f Mon Sep 17 00:00:00 2001 From: vlakoff Date: Wed, 27 Oct 2021 07:58:01 +0200 Subject: [PATCH 5/8] Do not append $op if it has not been removed from $k If getOperator() found an $op, but it is not at the very end of $k, it does not get removed from $k. Previous code was nevertheless appending $op, so it got repeated a second time in the result, which is obviously wrong. --- system/Database/BaseBuilder.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index 0d0c21e75b3f..e98084fcbec4 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -659,9 +659,11 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type $op = trim(current($op)); if (substr($k, -strlen($op)) === $op) { - $k = rtrim(substr($k, 0, -strlen($op))); + $k = rtrim(substr($k, 0, -strlen($op))); + $op = " {$op}"; + } else { + $op = ''; } - $op = " {$op}"; } else { $op = ' ='; } From 01dfc65437c8972faa4ec3d2f17fec228e91f70f Mon Sep 17 00:00:00 2001 From: vlakoff Date: Wed, 27 Oct 2021 07:59:09 +0200 Subject: [PATCH 6/8] Adding a space in query This space isn't mandatory, and the query string gets normalized later, adding this space, but let's make it cleanly in the first place. --- system/Database/BaseBuilder.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index e98084fcbec4..b5088b23acea 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -670,7 +670,7 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type if ($v instanceof Closure) { $builder = $this->cleanClone(); - $v = '(' . str_replace("\n", ' ', $v($builder)->getCompiledSelect()) . ')'; + $v = ' (' . str_replace("\n", ' ', $v($builder)->getCompiledSelect()) . ')'; } else { $bind = $this->setBind($k, $v, $escape); $v = " :{$bind}:"; From 65b8569f0b1dd9de816b7ed197b9683dd9f3f9fd Mon Sep 17 00:00:00 2001 From: vlakoff Date: Thu, 28 Oct 2021 00:34:30 +0200 Subject: [PATCH 7/8] Micro-optimization strtr() is faster for single-character replacements. --- system/Database/BaseBuilder.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index b5088b23acea..0a720cf33036 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -670,7 +670,7 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type if ($v instanceof Closure) { $builder = $this->cleanClone(); - $v = ' (' . str_replace("\n", ' ', $v($builder)->getCompiledSelect()) . ')'; + $v = ' (' . strtr($v($builder)->getCompiledSelect(), "\n", ' ') . ')'; } else { $bind = $this->setBind($k, $v, $escape); $v = " :{$bind}:"; @@ -842,7 +842,7 @@ protected function _whereIn(?string $key = null, $values = null, bool $not = fal if ($values instanceof Closure) { $builder = $this->cleanClone(); - $ok = str_replace("\n", ' ', $values($builder)->getCompiledSelect()); + $ok = strtr($values($builder)->getCompiledSelect(), "\n", ' '); } else { $whereIn = array_values($values); $ok = $this->setBind($ok, $whereIn, $escape); From 68ad98764be764384378ceaed0bce49adb5d4317 Mon Sep 17 00:00:00 2001 From: vlakoff Date: Sat, 30 Oct 2021 08:18:34 +0200 Subject: [PATCH 8/8] Add unit test for the case "$v === null and $k has operator" For this code path: "elseif (preg_match('/\s*(!?=|<>|IS(?:\s+NOT)?)\s*$/i', $k ..." The above code path "elseif (! $this->hasOperator($k) ..." is covered in testOrWhereInClosure() and testOrWhereNotInClosure(). --- tests/system/Database/Builder/WhereTest.php | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/system/Database/Builder/WhereTest.php b/tests/system/Database/Builder/WhereTest.php index 653c837058aa..e58af53cadab 100644 --- a/tests/system/Database/Builder/WhereTest.php +++ b/tests/system/Database/Builder/WhereTest.php @@ -111,6 +111,22 @@ public function testWhereAssociateArray() $this->assertSame($expectedBinds, $builder->getBinds()); } + public function testWhereAssociateArrayKeyHasEqualValueIsNull() + { + $builder = $this->db->table('users'); + + $where = [ + 'deleted_at =' => null, + ]; + + $expectedSQL = 'SELECT * FROM "users" WHERE "deleted_at" IS NULL'; + $expectedBinds = []; + + $builder->where($where); + $this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect())); + $this->assertSame($expectedBinds, $builder->getBinds()); + } + public function testWhereCustomString() { $builder = $this->db->table('jobs');