Relation/aggregate/event fixes + Concerns namespaces + Query\Grammar extraction#16
Open
abdul-kaioum wants to merge 61 commits into
Open
Relation/aggregate/event fixes + Concerns namespaces + Query\Grammar extraction#16abdul-kaioum wants to merge 61 commits into
abdul-kaioum wants to merge 61 commits into
Conversation
… bindings with placeholder
- update() and save() will return the model - delete, destroy will return affected rows count
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
`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
Relocate the three internal traits into `BitApps\WPDatabase\Concerns` (Relations, QueriesRelationships, HasEvents) to separate cross-cutting concerns from the public surface. Pure organization — no public class moved, no logic change. Relations/QueriesRelationships gain `use` imports for the QueryBuilder (and Model) types they reference; Model and QueryBuilder import the relocated traits. PSR-4 resolves the new path automatically. Assisted-By: AI
Pull the SELECT-side SQL compilation (~16 methods: compileSelect, getWhere,
getConditions, processConditions, getJoin/GroupBy/Having/OrderBy/From/Limit/
Offset, prepareColumnForWhere/ValueForWhere/OperatorForWhere, prepareRawSelect)
out of the 1,889-line QueryBuilder into a stateless `BitApps\WPDatabase\Query\
Grammar` collaborator. The builder keeps state, fluent setters, execution and
binding mutation; Grammar reads builder state via getters and appends bindings
through the builder. `toSql()` now dispatches explicitly (no stringly
`{'prepare'.$method}()`). Write-side compilation (insert/update/delete) and the
setter-time/static-coupled helpers stay on the builder. Behavior unchanged —
toSql output is byte-for-byte identical; adds Query\GrammarTest.
Assisted-By: AI
Post-extraction cleanup (no behavior change):
- Grammar: only compileSelect/getWhere/getJoin are called cross-object; make the
other 13 compilation helpers private. Reword the class docblock to state it
drives the builder's binding lifecycle (it is not side-effect-free).
- QueryBuilder: drop the unused getMethod() and getRawSql() (the latter dup'd
prepareRaw()); guard getClauseList() to where/having only instead of exposing
every property via $this->{$type}; initialise $sql in toSql().
Assisted-By: AI
Characterization tests asserting both compiled SQL and binding-array order (the risk surface of the Query\Grammar extraction): inner/left joins, nested closure grouping with flattened binding order, whereHas correlated exists sub-query (plain + constrained), selectRaw-before-where binding order, when() true/default branches, with() closure-constrained eager load, and bulk update. Assisted-By: AI
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the QueryBuilder class by extracting its SQL compilation logic into a new Grammar class, separating query building from SQL generation. It also introduces comprehensive tests to verify the compiled SQL and binding order. A critical bug was identified in the new Grammar class where a misplaced parenthesis in the LIKE operator check results in an incorrect boolean comparison inside strtoupper().
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.
| $sql = rtrim($sql, ',') . ')'; | ||
| } elseif (isset($clause['operator']) && strpos($clause['operator'], 'IS') !== false) { | ||
| $sql .= ' ' . $clause['value']; | ||
| } elseif (isset($clause['operator']) && strtoupper($clause['operator'] === 'LIKE')) { |
There was a problem hiding this comment.
There is a misplaced parenthesis in the LIKE operator check. The expression strtoupper($clause['operator'] === 'LIKE') compares the operator to 'LIKE' first (resulting in a boolean), and then passes that boolean to strtoupper(). This should be corrected to apply strtoupper() to the operator first, and then compare it to 'LIKE'.
} elseif (isset($clause['operator']) && strtoupper($clause['operator']) === 'LIKE') {`strtoupper($clause['operator'] === 'LIKE')` uppercased a boolean instead of the operator, so the LIKE branch only matched the exact string 'LIKE' and never a lowercase/mixed-case operator. Apply strtoupper() to the operator, then compare. Behavior is unchanged for the realistic case (a string LIKE value is bound as %s by the fall-through branch either way); this fixes the intent and a contrived numeric-operand edge. Reported by Gemini Code Assist on the PR. Assisted-By: AI
- upsert: sort() the column-name list (was ksort() — a no-op on the numeric
list) so columns line up with the alphabetically-ksorted row values; use
in_array() instead of array_key_exists() to detect created_at in the update
list (the latter checked numeric keys, so the created_at→updated_at rewrite
on duplicate key never fired).
- Model: add castToBoolean() (the documented 'boolean' cast dispatched to a
missing method and was silently ignored; only 'bool' worked).
- Collection::pluck(): resolve dynamic/accessor attributes via Model::getAttribute()
($item->{$key} ?? null short-circuits __isset and returns null for unloaded
relations/accessors).
- QueryBuilder::withCast(): add a chainable builder-level method (forwarding to
the model returned the Model and broke fluent chaining).
Reported by Gemini Code Assist on the PR. Adds regression tests + CastModel/
AccessorModel fixtures.
Assisted-By: AI
abdul-kaioum
changed the title
abdul-kaioum
changed the base branch from
refactor/relations-static-access
to
main
- breaking-changes §3: count() returns int and preserves a real 0; note the Concerns/ + Query\Grammar internal relocation (public API unchanged). - §4.5: bool/boolean cast; chainable QueryBuilder::withCast(). Assisted-By: AI
Assisted-By: AI
Assisted-By: AI
…hape Assisted-By: AI
- Replace broken Deal::belongsTo(Contact::class,'contact_id','id') example which generated WHERE contacts.contact_id=deals.id (wrong column table). Correct call is belongsTo(Contact::class,'id','contact_id') which generates WHERE contacts.id IN (SELECT contact_id FROM deals). - Document that hasOne() is a literal alias of belongsTo() (oneToOne type, same implementation); no separate reverse-direction exists. - Clarify key convention: $foreignKey = column on related table, $localKey = column on calling table (applies to all relation methods). - Add belongsToMany() caveat: declared but has no pivot-table logic. - Show 'relation as alias' aliasing form for aggregate methods. - Note that withMin/Max/Avg/Sum require 'relation.column' form; bare relation name defaults to * which is only meaningful for count/exists. Assisted-By: AI
…ns section - Model events: remove creating/created from example and events list; they fire internally but HasEvents has no registrar for them. Note boot hooks (booting/booted) are overridable methods, not Closure registrars. Add saving/saved fire on both insert and update. - Schema builder: replace full column/modifier/FK/altering content with a one-screen teaser + link to docs/schema.md (detail moves to Task 8). - Add ## Limitations & known issues section covering joins (double-prefix, ON-column, prepareOn reuse), belongsToMany pivot, belongsTo/hasOne alias + reversed key naming, soft-delete write-only, upsert MySQL-only + updated_at bug, creating/created unsubscribable, bulk insert bare-array fallback. - TOC: add Limitations & known issues entry. Assisted-By: AI
Assisted-By: AI
…hPrefix) Assisted-By: AI
Assisted-By: AI
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #15 (this branch contains all of its commits). Diffs against
main. Three layers:1. Bug fixes (relation aggregates, soft delete, events, casts)
withMin/Max/Avg/Sum('rel.column')now parse and aggregate the column (was hardcoded*→ silently no-op).deleted_at = <ts>on matched rows and honours the no-WHERE guard (was malformedSET <ts> = NULL, unguarded).savinghandler returningfalseaborts the write;saving/savedfire on insert and update;retrievedfires after hydration.aggregate()/count()no longer collapse a genuine0/'0'tonull;count()returnsint.?typeinstead of 8.0type|null).2. Restructure (no behavior change)
BitApps\WPDatabase\Concerns(PSR-4, no public class moved).QueryBuilderinto a statelessBitApps\WPDatabase\Query\Grammarcollaborator;toSql()dispatches explicitly.QueryBuildershed ~480 lines.toSqloutput byte-for-byte identical.3. Review fixes (from Gemini Code Assist)
sort()the column list (was aksort()no-op → value/column misalignment);in_array()for thecreated_at→updated_atswap (wasarray_key_exists(), always false).Model: implement the documented'boolean'cast.Collection::pluck(): resolve dynamic/accessor attributes viagetAttribute().QueryBuilder::withCast(): chainable builder method.Grammar: fix the LIKE operator check (strtoupper($op) === 'LIKE').Verification
php-cs-fixer+ PHPCompatibility7.4-clean.Notes (pre-existing, out of scope)
join('table')double-prefixes the joined table (wp_wp_*) — pre-existing join-prefix bug.🤖 Generated with Claude Code