From 0645d3de8d715ff6663d59bef27d84a4b518a182 Mon Sep 17 00:00:00 2001 From: memleakd <121398829+memleakd@users.noreply.github.com> Date: Mon, 18 May 2026 17:59:40 +0200 Subject: [PATCH] refactor(database): centralize builder condition handling - Route WHERE/HAVING condition insertion through shared prefix handling - Extract WHERE/HAVING condition compilation into focused private helpers - Add regression coverage for operator and null condition handling - Reduce the BaseBuilder empty() PHPStan baseline count Signed-off-by: memleakd <121398829+memleakd@users.noreply.github.com> --- system/Database/BaseBuilder.php | 227 ++++++++++--------- tests/system/Database/Builder/WhereTest.php | 37 +++ utils/phpstan-baseline/empty.notAllowed.neon | 2 +- 3 files changed, 159 insertions(+), 107 deletions(-) diff --git a/system/Database/BaseBuilder.php b/system/Database/BaseBuilder.php index e12ad26a466c..a5dfe095816f 100644 --- a/system/Database/BaseBuilder.php +++ b/system/Database/BaseBuilder.php @@ -855,16 +855,14 @@ protected function whereColumnHaving(string $qbKey, string $first, string $secon $escape ??= $this->db->protectIdentifiers; - $prefix = $this->{$qbKey} === [] ? $this->groupGetType('') : $this->groupGetType($type); - - $this->{$qbKey}[] = [ + $this->addWhereHavingCondition($qbKey, [ 'columnComparison' => true, - 'condition' => $prefix, + 'condition' => '', 'escape' => $escape, 'first' => $first, 'operator' => $operator, 'second' => $second, - ]; + ], $type); return $this; } @@ -977,17 +975,15 @@ private function whereBetweenHaving(string $qbKey, ?string $key = null, $values $lowerBind = $this->setBind($key, $values[0], $escape); $upperBind = $this->setBind($key, $values[1], $escape); $not = $not ? ' NOT' : ''; - $prefix = $this->{$qbKey} === [] ? $this->groupGetType('') : $this->groupGetType($type); - - $this->{$qbKey}[] = [ + $this->addWhereHavingCondition($qbKey, [ 'betweenComparison' => true, - 'condition' => $prefix, + 'condition' => '', 'escape' => $escape, 'key' => $key, 'lowerBind' => $lowerBind, 'not' => $not, 'upperBind' => $upperBind, - ]; + ], $type); return $this; } @@ -1010,13 +1006,12 @@ protected function whereExistsSubquery($subquery, bool $not = false, string $typ throw new InvalidArgumentException(sprintf('%s() expects $subquery to be of type BaseBuilder or closure', debug_backtrace(0, 2)[1]['function'])); } - $prefix = $this->QBWhere === [] ? $this->groupGetType('') : $this->groupGetType($type); $operator = $not ? 'NOT EXISTS' : 'EXISTS'; - $this->QBWhere[] = [ - 'condition' => "{$prefix}{$operator} {$this->buildSubquery($subquery, true)}", + $this->addWhereHavingCondition('QBWhere', [ + 'condition' => "{$operator} {$this->buildSubquery($subquery, true)}", 'escape' => false, - ]; + ], $type); return $this; } @@ -1055,8 +1050,6 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type } foreach ($keyValue as $k => $v) { - $prefix = empty($this->{$qbKey}) ? $this->groupGetType('') : $this->groupGetType($type); - if ($rawSqlOnly) { $k = ''; $op = ''; @@ -1104,22 +1097,42 @@ protected function whereHaving(string $qbKey, $key, $value = null, string $type $op = ''; } + $condition = $k . $op . $v; + if ($v instanceof RawSql) { - $this->{$qbKey}[] = [ - 'condition' => $v->with($prefix . $k . $op . $v), - 'escape' => $escape, - ]; - } else { - $this->{$qbKey}[] = [ - 'condition' => $prefix . $k . $op . $v, - 'escape' => $escape, - ]; + $condition = $v->with($condition); } + + $this->addWhereHavingCondition($qbKey, [ + 'condition' => $condition, + 'escape' => $escape, + ], $type); } return $this; } + /** + * @param array $condition + */ + private function addWhereHavingCondition(string $clause, array $condition, string $type): void + { + $prefix = $this->getWhereHavingPrefix($clause, $type); + + if ($condition['condition'] instanceof RawSql) { + $condition['condition'] = $condition['condition']->with($prefix . $condition['condition']); + } else { + $condition['condition'] = $prefix . $condition['condition']; + } + + $this->{$clause}[] = $condition; + } + + private function getWhereHavingPrefix(string $clause, string $type): string + { + return $this->{$clause} === [] ? $this->groupGetType('') : $this->groupGetType($type); + } + /** * Generates a WHERE field IN('item', 'item') SQL query, * joined with 'AND' if appropriate. @@ -1268,14 +1281,10 @@ protected function _whereIn(?string $key = null, $values = null, bool $not = fal $ok = $this->setBind($ok, $whereIn, $escape); - $prefix = empty($this->{$clause}) ? $this->groupGetType('') : $this->groupGetType($type); - - $whereIn = [ - 'condition' => "{$prefix}{$key}{$not} IN :{$ok}:", + $this->addWhereHavingCondition($clause, [ + 'condition' => "{$key}{$not} IN :{$ok}:", 'escape' => false, - ]; - - $this->{$clause}[] = $whereIn; + ], $type); return $this; } @@ -1408,7 +1417,7 @@ protected function _like($field, string $match = '', string $type = 'AND ', stri $v = $match; $insensitiveSearch = false; - $prefix = empty($this->{$clause}) ? $this->groupGetType('') : $this->groupGetType($type); + $prefix = $this->getWhereHavingPrefix($clause, $type); if ($side === 'none') { $bind = $this->setBind($field->getBindingKey(), $v, $escape); @@ -1442,7 +1451,7 @@ protected function _like($field, string $match = '', string $type = 'AND ', stri $v = mb_strtolower($v, 'UTF-8'); } - $prefix = empty($this->{$clause}) ? $this->groupGetType('') : $this->groupGetType($type); + $prefix = $this->getWhereHavingPrefix($clause, $type); if ($side === 'none') { $bind = $this->setBind($k, $v, $escape); @@ -3479,98 +3488,104 @@ protected function compileWhereHaving(string $qbKey): string { if (! empty($this->{$qbKey})) { foreach ($this->{$qbKey} as &$qbkey) { - // Is this condition already compiled? - if (is_string($qbkey)) { - continue; - } + $qbkey = $this->compileWhereHavingCondition($qbkey); + } - if ($qbkey instanceof RawSql) { - continue; - } + return ($qbKey === 'QBHaving' ? "\nHAVING " : "\nWHERE ") + . implode("\n", $this->{$qbKey}); + } + + return ''; + } - if ($qbkey['condition'] instanceof RawSql) { - $qbkey = $qbkey['condition']; + /** + * @used-by compileWhereHaving() + * + * @param array|RawSql|string $condition + */ + private function compileWhereHavingCondition(array|RawSql|string $condition): RawSql|string + { + // Is this condition already compiled? + if (is_string($condition) || $condition instanceof RawSql) { + return $condition; + } - continue; - } + if ($condition['condition'] instanceof RawSql) { + return $condition['condition']; + } - if (($qbkey['columnComparison'] ?? false) === true) { - $qbkey = $this->compileColumnComparison($qbkey); + if (($condition['columnComparison'] ?? false) === true) { + return $this->compileColumnComparison($condition); + } - continue; - } + if (($condition['betweenComparison'] ?? false) === true) { + return $this->compileBetweenComparison($condition); + } - if (($qbkey['betweenComparison'] ?? false) === true) { - $qbkey = $this->compileBetweenComparison($qbkey); + if ($condition['escape'] === false) { + return $condition['condition']; + } - continue; - } + return $this->compileEscapedCondition($condition['condition']); + } - if ($qbkey['escape'] === false) { - $qbkey = $qbkey['condition']; + /** + * @used-by compileWhereHavingCondition() + */ + private function compileEscapedCondition(string $condition): string + { + // Split multiple conditions + $conditions = preg_split( + '/((?:^|\s+)AND\s+|(?:^|\s+)OR\s+)/i', + $condition, + -1, + PREG_SPLIT_DELIM_CAPTURE | PREG_SPLIT_NO_EMPTY, + ); - continue; - } + foreach ($conditions as &$condition) { + $op = $this->getOperator($condition); + if ( + $op === false + || preg_match( + '/^(\(?)(.*)(' . preg_quote($op, '/') . ')\s*(.*(?getOperator($condition); - if ( - $op === false - || preg_match( - '/^(\(?)(.*)(' . preg_quote($op, '/') . ')\s*(.*(? '(test <= foo)', /* the whole thing */ + // 1 => '(', /* optional */ + // 2 => 'test', /* the field name */ + // 3 => ' <= ', /* $op */ + // 4 => 'foo', /* optional, if $op is e.g. 'IS NULL' */ + // 5 => ')' /* optional */ + // ]; - // $matches = [ - // 0 => '(test <= foo)', /* the whole thing */ - // 1 => '(', /* optional */ - // 2 => 'test', /* the field name */ - // 3 => ' <= ', /* $op */ - // 4 => 'foo', /* optional, if $op is e.g. 'IS NULL' */ - // 5 => ')' /* optional */ - // ]; - - if ($matches[4] !== '') { - $protectIdentifiers = false; - if (str_contains($matches[4], '.')) { - $protectIdentifiers = true; - } - - if (! str_contains($matches[4], ':')) { - $matches[4] = $this->db->protectIdentifiers(trim($matches[4]), false, $protectIdentifiers); - } - - $matches[4] = ' ' . $matches[4]; - } + if ($matches[4] !== '') { + $protectIdentifiers = false; + if (str_contains($matches[4], '.')) { + $protectIdentifiers = true; + } - $condition = $matches[1] . $this->db->protectIdentifiers(trim($matches[2])) - . ' ' . trim($matches[3]) . $matches[4] . $matches[5]; + if (! str_contains($matches[4], ':')) { + $matches[4] = $this->db->protectIdentifiers(trim($matches[4]), false, $protectIdentifiers); } - $qbkey = implode('', $conditions); + $matches[4] = ' ' . $matches[4]; } - return ($qbKey === 'QBHaving' ? "\nHAVING " : "\nWHERE ") - . implode("\n", $this->{$qbKey}); + $condition = $matches[1] . $this->db->protectIdentifiers(trim($matches[2])) + . ' ' . trim($matches[3]) . $matches[4] . $matches[5]; } - return ''; + return implode('', $conditions); } /** - * @used-by compileWhereHaving() + * @used-by compileWhereHavingCondition() * * @param array{columnComparison: true, condition: string, escape: bool, first: string, operator: string, second: string} $condition */ @@ -3585,7 +3600,7 @@ private function compileColumnComparison(array $condition): string } /** - * @used-by compileWhereHaving() + * @used-by compileWhereHavingCondition() * * @param array{betweenComparison: true, condition: string, escape: bool, key: string, lowerBind: string, not: string, upperBind: string} $condition */ diff --git a/tests/system/Database/Builder/WhereTest.php b/tests/system/Database/Builder/WhereTest.php index a8a0586b798d..57151ff8a48a 100644 --- a/tests/system/Database/Builder/WhereTest.php +++ b/tests/system/Database/Builder/WhereTest.php @@ -151,6 +151,43 @@ public function testWhereLikeInAssociateArray(): void $this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect())); } + /** + * @param mixed $value + */ + #[DataProvider('provideWhereOperatorRegressionCases')] + public function testWhereOperatorRegressionCases(string $key, $value, string $expectedSQL): void + { + $builder = $this->db->table('jobs job'); + + $builder->where($key, $value); + + $this->assertSame($expectedSQL, str_replace("\n", ' ', $builder->getCompiledSelect())); + } + + /** + * @return iterable + */ + public static function provideWhereOperatorRegressionCases(): iterable + { + return [ + 'like operator with value' => [ + 'job.status LIKE', + 'p%', + 'SELECT * FROM "jobs" "job" WHERE "job"."status" LIKE \'p%\'', + ], + 'equals operator with null' => [ + 'job.deleted_at =', + null, + 'SELECT * FROM "jobs" "job" WHERE "job"."deleted_at" IS NULL', + ], + 'not equals operator with null' => [ + 'job.deleted_at !=', + null, + 'SELECT * FROM "jobs" "job" WHERE "job"."deleted_at" IS NOT NULL', + ], + ]; + } + public function testWhereCustomString(): void { $builder = $this->db->table('jobs'); diff --git a/utils/phpstan-baseline/empty.notAllowed.neon b/utils/phpstan-baseline/empty.notAllowed.neon index 0f92e89f912e..e10a142b0421 100644 --- a/utils/phpstan-baseline/empty.notAllowed.neon +++ b/utils/phpstan-baseline/empty.notAllowed.neon @@ -34,7 +34,7 @@ parameters: - message: '#^Construct empty\(\) is not allowed\. Use more strict comparison\.$#' - count: 28 + count: 24 path: ../../system/Database/BaseBuilder.php -