Refactor/relations static access#15
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces major updates to the database package, including returning a new Collection class for multi-row query results, adding model lifecycle events, and relocating relation eager-loading methods to the QueryBuilder to support static access and improve IDE navigation. It also adds new query builder features like upsert(), when(), and a guard against empty WHERE clauses in deletes. While these changes significantly improve the library, several critical issues were identified: QueryBuilder::upsert() contains bugs where ksort() is incorrectly applied to numerically indexed columns and array_key_exists() is used on a list of values; Collection::pluck() can fail to load dynamic attributes or relations due to null coalescing; the documented 'boolean' cast is unimplemented; calling withCast() on the builder breaks fluent chaining; and unsetting bindings leaves non-contiguous array keys.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| $columns = array_keys($values[0]); | ||
| ksort($columns); |
There was a problem hiding this comment.
The use of ksort($columns) here is a critical bug. Since $columns is a numerically indexed array of column names (e.g., [0 => 'email', 1 => 'first_name']), sorting it by keys (ksort) does not change the order of the column names. However, in the loop below, each row is sorted alphabetically by its keys (ksort($row)). This mismatch causes the generated SQL to align values with the wrong columns if the input keys are not already sorted alphabetically. Use sort($columns) instead to sort the column names alphabetically.
$columns = array_keys($values[0]);
sort($columns);|
|
||
| $sql .= empty($insertAbleValues) ? ' default values' : ' ' . implode(',', $insertAbleValues); | ||
| $sql .= ' ON DUPLICATE KEY UPDATE '; | ||
| if (\array_key_exists('created_at', $update)) { |
There was a problem hiding this comment.
Using \array_key_exists('created_at', $update) is incorrect because $update is a list of column names (values), not keys. This check will always return false, meaning 'created_at' is never replaced with 'updated_at'. Consequently, on duplicate key, created_at will be incorrectly overwritten instead of updating updated_at. Use \in_array('created_at', $update, true) instead.
if (\in_array('created_at', $update, true)) {| return new static(array_map(function ($item) use ($key) { | ||
| return is_array($item) ? $item[$key] ?? null : (is_object($item) ? $item->{$key} ?? null : null); | ||
| }, $this->items)); |
There was a problem hiding this comment.
Using $item->{$key} ?? null on a Model instance will return null for relations or dynamic attributes that haven't been loaded yet. This is because the null coalescing operator (??) triggers __isset(), which only checks $this->attributes. To safely resolve relations and dynamic attributes, check if the item is an instance of Model and use getAttribute($key).
return new static(array_map(function ($item) use ($key) {
if (is_array($item)) {
return $item[$key] ?? null;
}
if (is_object($item)) {
if ($item instanceof Model) {
return $item->getAttribute($key);
}
return $item->{$key} ?? null;
}
return null;
}, $this->items));| private function castToBool($value) | ||
| { | ||
| return \boolval($value); | ||
| } |
There was a problem hiding this comment.
The documentation states that 'boolean' is a supported cast type, but castToBoolean() is not implemented in the Model class. As a result, 'boolean' casts are silently ignored and return raw values. Implement castToBoolean() to delegate to castToBool().
private function castToBool($value)
{
return \boolval($value);
}
private function castToBoolean($value)
{
return $this->castToBool($value);
}| /** | ||
| * Clone this class with all conditions | ||
| */ | ||
| public function clone() | ||
| { | ||
| return clone $this; | ||
| } |
There was a problem hiding this comment.
Currently, calling withCast() on a QueryBuilder forwards the call to the Model instance, which returns the Model instance instead of the QueryBuilder. This breaks fluent chaining on the builder. Adding a withCast() method directly to QueryBuilder that delegates to the model and returns $this keeps the builder interface consistent.
/**
* Clone this class with all conditions
*/
public function clone()
{
return clone $this;
}
/**
* Add casts to the bound model at runtime.
*
* @param array $casts
* @return $this
*/
public function withCast(array $casts)
{
$this->_model->withCast($casts);
return $this;
}| public function prepare($sql = null) | ||
| { | ||
| if (\is_null($sql) && isset($this->_method)) { | ||
| $sql = $this->{'prepare' . $this->_method}(); | ||
| if (\is_null($sql)) { | ||
| $sql = $this->toSql(); | ||
| } | ||
|
|
||
| return empty($this->bindings) | ||
| || strpos($sql, '%') === false | ||
| ? $sql : Connection::prepare($sql, $this->bindings); | ||
| } |
There was a problem hiding this comment.
Unsetting elements from $this->bindings during prepareInsert() or prepareUpdate() leaves gaps in the array keys (non-contiguous keys). This can cause issues with custom wpdb mocks or potential future changes in PHP/WordPress. Re-indexing $this->bindings using array_values() right before preparing the query ensures a clean, packed array.
public function prepare($sql = null)
{
if (\is_null($sql)) {
$sql = $this->toSql();
}
$this->bindings = array_values($this->bindings);
return empty($this->bindings)
|| strpos($sql, '%') === false
? $sql : Connection::prepare($sql, $this->bindings);
}… bindings with placeholder
- update() and save() will return the model - delete, destroy will return affected rows count
- Regex doesn't machtes due to have a mew line after SELECT
Move eager-loading and aggregate methods (with, withCount, withMin/Max/Avg/ Sum/Exists, whereHas, withWhereHas) out of the Relations trait into a new QueriesRelationships trait on QueryBuilder. Because they are no longer declared on Model, `Model::with()` etc. now resolve through __callStatic instead of fatally erroring with "Non-static method ... cannot be called statically". Relation definitions (hasMany/belongsTo) stay on the model's Relations trait. - add Model::query() as a real, IDE-navigable builder entry point - @mixin QueryBuilder + restored @method static tags for cross-IDE support (PhpStorm navigation + VS Code/Intelephense autocomplete) - fix with(['a','b']) array form (previously dropped by func_get_args) - DRY the relation-key lookup via Model::getActiveRelationKey() - add PHPUnit suite (42 tests) with wpdb/WP stubs and fixtures - document version breaking changes in docs/breaking-changes.md Assisted-By: AI
Add docs/usage.md covering models, CRUD, the query builder, collections, casts, relationships and aggregates, model events, transactions, raw queries and the schema builder. Rewrite the README stub into a quick-start that links the usage guide and breaking-changes notes. - fix paginate() @method tag (arg order was reversed vs the real signature) Assisted-By: AI
Red tests documenting four confirmed defects to be fixed next: - withMin/withMax/withAvg/withSum ignore the `relation.column` argument (hardcode `*`), so the aggregate is never emitted. - Soft delete builds `SET <timestamp> = NULL` instead of `SET deleted_at = <timestamp>`, and bypasses the no-WHERE guard. - A `saving` handler returning false does not abort the write; `retrieved` fires before the model is hydrated on single-row reads. - count()/min()/aggregate() collapse a genuine 0/'0' to null via !empty(). Adds SoftPost, EventUser, RetrieveUser fixtures and wires them into the bootstrap. Assisted-By: AI
- withMin/withMax/withAvg/withSum now parse the `relation.column` argument and aggregate that column (was hardcoded `*` → silently emitted nothing). - Soft delete writes `deleted_at = <ts>` on matched rows and honours the no-WHERE guard (was malformed `SET <ts> = NULL`, and unguarded). - A `saving` handler returning false aborts the write; saving/saved fire on both insert and update; `retrieved` fires after the model is hydrated. - aggregate()/count() no longer collapse a genuine 0/'0' to null; count() returns int; canonical selectRaw reset removes an undefined-key warning. Assisted-By: AI
f67c728 to
a5d0d6d
Compare
`callable|null` (Collection::first/last) and `array|null` (QueryBuilder::upsert) are PHP 8.0+ union syntax and fatal-parse on 7.4, which composer.json still supports (^7.4). Replace with the `?type` nullable form. Assisted-By: AI
No description provided.