Release v2.0: namespace/Grammar refactor, query/schema fixes, soft-delete, pivot, aggregates#17
Open
abdul-kaioum wants to merge 102 commits into
Open
Release v2.0: namespace/Grammar refactor, query/schema fixes, soft-delete, pivot, aggregates#17abdul-kaioum wants to merge 102 commits into
abdul-kaioum wants to merge 102 commits into
Conversation
… bindings with placeholder
- update() and save() will return the model - delete, destroy will return affected rows count
with() builds `foreignKey IN ( SELECT * FROM (<parent>) AS subquery )` from a
clone of the parent query narrowed to the local key. select() resets the
select list but not selectRaw, so a parent selectRaw('... as x') leaked a
second column into the subquery -> MySQL "Operand should contain 1 column(s)".
Strip selectRaw on the key-subquery clone via a shared prepareKeySubquery()
helper, used by both the hasMany/belongsTo and pivot eager paths. Byte-identical
when the parent has no selectRaw.
Assisted-By: AI
The `foreignKey IN ( SELECT key ... )` eager subquery cloned the parent and
stripped selectRaw but kept ORDER BY. A parent ordered by a selectRaw alias
(orderBy('dsc') after selectRaw('... as dsc')) then referenced a column absent
from the narrowed subquery -> "Unknown column in order clause"; and ORDER BY is
meaningless for set membership regardless.
Move prepareKeySubquery onto QueryBuilder (so it can reset the protected
orderBy) and drop ORDER BY unless a LIMIT pins which parent rows the set comes
from. Byte-identical for parents without selectRaw/orderBy.
Assisted-By: AI
…tions 41 tests locking verified current behavior (no source change): - WhereClauseEdgeTest: whereIn type-placeholders, implicit-IN array value, IS NULL, falsy values not dropped, operator/LIKE variants, whereBetween, nested-closure binding order, whereRaw/selectRaw/having binding order, soft-delete scope binding order through the paren-wrap. - SelectJoinAggregateEdgeTest: column-alias variants (dotted/lowercase/ backticked/spaced/multi-as), RIGHT/FULL/CROSS join, on()/orOn() chaining, custom-$prefix join ON qualification, count()=COUNT(pk), max/min empty=null, aggregate-runs-on-clone, asc() pk fallback. - RelationEagerEdgeTest: eager/whereHas/withCount constraint closures, multiple whereHas, withCount alias, select+withCount no dup, relation alias attach, parent filter in eager subquery, eager-on-first(). Assisted-By: AI
…ase 1) Pure repairs of crashing / invalid-SQL paths plus additive forceDelete/restore; byte-identical for every currently-working input (momus-reviewed). - insert(): route to bulk only for a true list-of-rows, so a single row whose first value is an array no longer crashes via ksort(). - where/whereIn: null + explicit operator -> IS [NOT] NULL; empty array -> the false constant `0 = 1` (was invalid `IN ()`); a nested IN element -> one JSON-encoded placeholder; object value -> wp_json_encode (was fatal on prepare). - empty save() (no dirty columns) skips the malformed `UPDATE ... SET` and returns the model (fires saving/saved). - insert([]) / empty bulk rows / upsert($v, []) no longer emit malformed SQL. - aggregate(fn, '*') -> COUNT(*) (was invalid COUNT(`t`.*)); count() unchanged. - forceDelete() (real DELETE bypassing the soft scope) and restore() on soft-delete models. - take()/skip() cast to int -> blocks limit/offset injection. - eager key subquery also strips the parent's groupBy/having. Assisted-By: AI
Assisted-By: AI
…d bulk, schema DDL (zero-BC Phase 2) Behavior-changing fixes verified safe against bit-pi/bit-crm/bit-social/bit-assist usage (consumers pass only identifiers to order/group by, don't use the cast aliases, etc.); byte-identical for currently-working inputs (momus-reviewed). - E2: orderBy()/groupBy() validate the column is a plain identifier (`^[A-Za-z0-9_.`]+$`) and throw otherwise -> blocks ORDER BY/GROUP BY injection. Valid identifiers emit byte-identical SQL; raw expressions still go through orderByRaw(). - B1: cast aliases integer/float/double/json/datetime now map onto the real casters (were silent no-ops). - C1: relation values (Collection / Model / list-of-Model) are excluded from the save/update column set -> lazy relation access no longer corrupts a later save(); scalar/JSON-array columns still persist. - C6: bulk insert aligns each row to the header columns by key (was silent misalignment for ragged rows); uniform rows unchanged. - B4: Schema edit-mode unique() emits `ADD UNIQUE INDEX`; renameColumn() in edit() emits `RENAME COLUMN`; decimal($p, $s) works (incl. scale 0). Assisted-By: AI
… ragged bulk) Assisted-By: AI
An eager-loaded parent with no related rows stored null, so accessing the relation fell through offsetExists() to a fresh lazy query (the N+1 the eager load exists to prevent) that returned an empty Collection. Store [] instead: offsetExists() is true, the resolved-empty value is returned with no extra query. Empty either way (count 0, falsy); the type now matches a non-empty eager relation (plain array). No existing test relied on the null. Assisted-By: AI
A model with $soft_deletes = true now injects deleted_at IS NULL on every SELECT automatically; trashed rows no longer appear. Opt out with public $soft_delete_scope = false to restore the unfiltered read. refresh() reloads a row by its own primary key with withTrashed(), so a re-hydrated trashed model still reports exists() === true and the next save() UPDATEs instead of re-INSERTing a duplicate. Scope is injected on SELECT only; delete/restore/forceDelete/insert/ upsert/aggregate paths are unaffected. Assisted-By: AI
Add docs/relations.md — a standalone relationship reference covering hasOne/belongsTo (the shared oneToOne alias and reversed-from-Laravel key naming), hasMany, belongsToMany pivot relations (signature, key defaults, withPivot, pivot_* attributes, read-only limits), eager/lazy loading, and relation aggregates. Linked from README and usage.md; the deep belongsToMany block and relation Limitations bullets in usage.md now point to it. Document the soft-delete default flip: usage.md (property table, deleting section, limitations) and breaking-changes.md (new 2.13 + summary row 13). Assisted-By: AI
Failing tests pinning the desired behavior for relation-name resolution: with()/withCount()/whereHas() must reject a non-relation method name with a RuntimeException, and getActiveRelationKey() must fail loudly on an unknown relation tag rather than returning a silent null. These are the RED phase for the not-yet-implemented relation-safety fix (option alpha) and fail until that guard lands. Assisted-By: AI
Relation resolution (prepareRelation, the single chokepoint for with/ withCount/whereHas/withWhereHas) previously called any method whose name matched a relation, gated only by method_exists — letting a framework Model method (e.g. refresh(), which runs a SELECT) or any consumer no-arg method run through a relation name, and failing messily on typos. Now: a name declared on the framework Model is rejected WITHOUT being called (declaring-class identity via ReflectionMethod, compared to Model::class so it holds under php-scoper; memoized per class::method); a consumer-declared method is called and its return validated as a relation query (QueryBuilder whose model has an active relation key) or rejected with a RuntimeException. A name that fails method_exists is still silently skipped (zero-BC for optional/typo'd relation lists). getActiveRelationKey() now throws on an unknown relation tag instead of returning a silent null. Turns the RED RelationResolutionSafetyTest green (216 tests, all pass). Assisted-By: AI
Add the empirically-confirmed footguns a v1 consumer hits, under their
existing sections:
- 2.1 an is_array($result) "got rows?" guard now inverts (Collection when
rows exist, [] when empty) -> silent data loss; use empty()/!empty().
- 2.2 $model->update([...])->save() fatals ("save() on false") on a 0-row
update -> drop the trailing ->save().
- 2.4 a function/expression in select() only "survives" if it contains a
'.', so CONCAT(url,...) breaks on dotless hosts (localhost); use selectRaw.
- 2.9 a leftover no-arg ->withCount() throws ArgumentCountError, including
when reached via eager-load relation resolution.
Assisted-By: AI
exec() returns rows-affected (0 for a no-op UPDATE) and false only on a real DB error or a cancelled pre-event. save()'s existing-model branch checked exec() for truthiness, so a valid UPDATE that changed no rows (an idempotent re-save where no value differs) returned false — misreporting success as failure. This was the root cause of `$model->update([...])->save()` fataling (false->save()) and made `if (!$model->save())` falsely error on no-op saves. Compare against false so any non-error result (including 0 rows) returns the Model and fires 'saved', consistent with the 'updated' event that already fired. Insert path and genuine-error path unchanged. Documented in breaking-changes.md §3 (+ §2.2 note updated). Adds SaveZeroRowUpdateFixTest (0-row returns Model, chained update()->save() no fatal, real error still false). Assisted-By: AI
save()'s insert branch decided success by lastInsertId() ($wpdb->insert_id), so a successful INSERT into a table with a manual/composite key (insert_id 0) returned false and never fired 'saved'. It also risked returning the Model on a FAILED insert when insert_id held a stale non-zero value from an earlier insert in the same request. Decide success from exec() (false only on real error/cancel), then still assign the auto-increment id to the primary key when present. A manual/ composite-key insert now returns the Model; a genuine error returns false before touching lastInsertId (no stale PK, no 'saved'). Documented in breaking-changes.md §3. Adds SaveInsertReturnFixTest (auto-increment sets PK; manual-PK returns Model; error returns false). Assisted-By: AI
Add the edge cases the session's fixes introduced but left unverified: - soft-delete: refresh() reloads a trashed row without the scope filter (withTrashed) so exists() stays true. - relation guard: a method returning a plain QueryBuilder with no active relation key is rejected; a framework Model method is rejected; the 4th entry point withWhereHas() rejects a non-relation too. - save() insert: a failed insert carrying a stale non-zero insert_id returns false and does not set the PK from it. - saved event fires on a 0-row UPDATE and a manual-PK INSERT, and never on a failed write (new SavedEventUser fixture). getActiveRelationKey() short-circuits a null relation tag before the isset() so it no longer trips PHP 8's "null as array offset" deprecation (surfaced by the plain-QueryBuilder rejection test). Behavior unchanged — both still throw. Assisted-By: AI
…, events) Cover the three edges previously flagged as skipped: - relation guard memoizes the framework-vs-consumer verdict per class::method (reflection-asserted: refresh -> true, posts -> false, no collision). - a relation declared on an intermediate base class (leaf -> base -> Model) resolves, since its declaring class is the base, not the framework Model. - created fires on a successful INSERT and updated fires even on a 0-row UPDATE (SavedEventUser gains created/updated counters). New fixtures RelationBaseModel/RelationLeafModel. The memoization test guards ReflectionProperty::setAccessible() behind PHP_VERSION_ID (required on 7.4, a deprecated no-op on 8.1+) to keep the suite deprecation-clean. Assisted-By: AI
Raise the minimum PHP from ^7.4 || ^8.0 to >=8.2 (breaking: the package no longer installs on PHP < 8.2). Add phpunit/phpunit ^11.5 as a dev dependency and a `composer test` script so tests run from the vendored PHPUnit instead of a gitignored phpunit.phar. Point the compat gate at 8.2- and pin config.platform.php to 8.2.0 for reproducible installs. Documented in breaking-changes.md §2.14 (+ summary row 14). Assisted-By: AI
QueryBuilder had a $distinct property but no method to set it and Grammar never read it, so ->distinct() threw "Call to undefined method". Add distinct()/isDistinct() and emit SELECT DISTINCT. Additive: default false keeps existing SELECTs byte-identical. Assisted-By: AI
avg()/sum() were missing (only count/max/min existed), so calling them
directly threw while aggregate('avg', ...) worked. Add both as wrappers.
Also guard aggregate()'s $function, which is interpolated straight into
SQL, with a bare-identifier regex (strtoupper + [A-Za-z_][A-Za-z0-9_]*).
This blocks the injection surface without a fixed allowlist, so real
aggregates like GROUP_CONCAT/STDDEV keep working (no BC break).
Assisted-By: AI
A column qualified by the unprefixed model or joined table name (e.g. custom_apps.id) was emitted verbatim while the physical table is wp_custom_apps, producing "unknown column/table" errors. QueryBuilder now exposes an unprefixed->physical table map (model table + non-aliased joins) and resolveQualifier(); Grammar applies it at compile time to SELECT columns, WHERE/HAVING/JOIN-ON columns, GROUP BY and ORDER BY. Resolution is order-independent and idempotent. Aliases win over the map, and already-physical / unknown qualifiers pass through untouched, so only already-broken SQL changes (BC-safe). Three bug-locking tests that pinned the old passthrough are updated to assert the resolved output. Residual (documented): a joined-table qualifier inside a nested where() closure is not resolved (the child builder has no joins). Assisted-By: AI
- Resolve joined-table qualifiers inside nested where groups: closure
conditions and the soft-delete scope wrap now inherit the parent's
join/from context, so a joined table's unprefixed name resolves inside
a nested group exactly as at top level (the inherited joins feed only
the table map; they never emit JOIN SQL for the nested builder).
- aggregate(): stop upper-casing $function so a direct aggregate('avg', …)
keeps its pre-existing SQL/alias case, and reset DISTINCT on the clone
so distinct()->count() no longer emits a spurious SELECT DISTINCT.
- Extract the bare-identifier guard into assertSafeAggregateFunction() and
apply it in withAggregate() too, closing the same raw-$function
interpolation surface there.
Assisted-By: AI
- resolveQualifier(): compute the dot position once instead of scanning twice. - getTableMap()/getTableAliases(): drop the isset()/coalesce guards (join() always populates raw/prefixed/userAlias) and use symmetric null checks so the map and alias set stay complementary. - Replace inheritQualifierContext() with a newNestedQuery() factory that constructs the sub-builder and inherits join/from context in one call, naming the nested-builder intent and keeping newQuery() for the independent bulkInsert re-query. Assisted-By: AI
Address Gemini review on PR #17: - Split table/alias on \s+as\s+ (with trim + a 2-part limit) so extra spaces/tabs around AS no longer leak into the table name or alias. - prepareOn() qualifies the second operand only when it is a bare column identifier; constants, quoted values and function calls (10, 'active', NOW()) are left untouched instead of becoming invalid SQL like posts.10. Assisted-By: AI
Add /tests, phpunit.xml and phpcs.xml to export-ignore so composer dist tarballs ship only the runtime package. Assisted-By: AI
Ship the leanest runtime tarball; docs remain in the repo. Assisted-By: AI
abdul-kaioum
changed the title
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.
Delivers v2.0 of the package to
main. Supersedes #16 — this branch alreadycontains every
refactor/namespace-grammarcommit, so #16 is folded in here andcan be closed.
This is a major (breaking) release. Full, itemised list with before/after and
migration notes:
docs/breaking-changes.md(14 breakingchanges, §2.1–2.14). Highlights:
Model::get()returns aCollection(notarray),select()back-tick-qualifies columns,delete()with noWHEREno longer wipes atable, soft-delete reads exclude trashed rows by default, and minimum PHP is now 8.2.
What's in it
Architecture
Query\Grammar,Concerns\*),Collection, PHP 8.2 baseline, PHPUnit 11 (composer test).Query builder — fixes & additions
distinct()(was undefined) → emitsSELECT DISTINCT.avg()/sum()added (onlycount/max/minexisted);aggregate()function name guarded against SQL injection (bare-identifier regex, not an allowlist — GROUP_CONCAT/STDDEV keep working). Same guard applied towithAggregate().custom_apps.id) resolve to the physicalwp_*table across SELECT/WHERE/HAVING/ON/GROUP BY/ORDER BY; aliases win, already-physical/unknown pass through. Order-independent, idempotent.wp_wp_*double-prefix; keepwp_for custom-$prefixmodels; robustASalias split (tolerates extra whitespace); ON second operand qualified only when it's a bare column (constants/NOW()left intact). (Gemini review items addressed.)select('col AS alias'), eager-load key subquery (drops ORDER BY / parent selectRaw), bulk insert always returns aCollection, JSON-encoded array/object insert/upsert values, order/group-by injection guard.Features
withTrashed()/onlyTrashed();public $soft_delete_scope = falseopts out).belongsToMany(read-side).creating/createdmodel events.Schema
change()→MODIFY COLUMN; direct-call drop helpers emit full SQL;Schema::withWpPrefix()opt-in.Packaging
tests/,docs/,phpunit.xml,phpcs.xmlexport-ignored — dist tarball ships runtime +composer.json+READMEonly.Verification
php-cs-fixer+ PHPCompatibility8.2-clean; no PHPUnit deprecations.bit-piandbit-crm(each plugin's scoped copy) — CRUD + distinct/avg/sum/qualified-join against the real tables, all writes rolled back.Deferred (own PRs)
upsertupdated_at = VALUES(created_at)mapping (needs a design decision).belongsToManywrite-side (attach/detach/sync).🤖 Generated with Claude Code