Fire model.relation.beforeAdd/afterAdd events from AttachOneOrMany::create() - #244
Fire model.relation.beforeAdd/afterAdd events from AttachOneOrMany::create()#244wakqasahmed wants to merge 2 commits into
Conversation
…reate() create() built and persisted the related model directly via parent::create(), bypassing add() entirely whenever no sessionKey was given (the immediate, non-deferred-binding path). add() is what fires model.relation.beforeAdd/afterAdd and handles the single-attachment sibling deletion for AttachOne, so both silently never ran when using $model->relation()->create(...) directly instead of ->add(). Route create() through add() in both cases: build an unsaved model via newInstance() when sessionKey is null so add()'s own $model->save() is the only persist, keeping parent::create() for the deferred-binding path where the model must exist immediately.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. Walkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Attachment creation now uses the common add path so relation add events fire while single and multiple attachment behavior remains covered. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mjauvin
added
the
maintenance
mjauvin
added
accepted
|
Please, fix the PHPSTAN expected number of delete calls from 3 to 2. Looks good to me otherwise. |
|
Also, while you're at it, change the "private" in the error message to "public" (or remove altogether) |
create() no longer has its own direct call to the parent's private delete() (per the prior commit, it now routes through add(), which already had one) -- the baseline's expected-count for this message was never updated to match, so PHPStan silently over-tolerated one fewer occurrence than actually exists. Verified via a clean --memory-limit=1G analyse run with zero errors at count: 2.
|
Fixed the PHPStan count — pushed. Verified with a clean On the second point — could you point me to which message/line you mean? I don't see a literal "private" string in anything this PR touches, and I want to make sure I fix the right spot rather than guess. |
In the phpstan rule you fixed for the count, the text mentions "private" although it is public. |
mjauvin
added
the
accepted
2 participants
Summary
AttachOneOrMany::create()builds and persists the related model directly viaparent::create(), bypassingadd()entirely whenever nosessionKeyis given — the normal, immediate (non-deferred-binding) path most$model->relation()->create(...)calls take.add()is what actually firesmodel.relation.beforeAdd/model.relation.afterAdd, and also handles deleting the existing sibling attachment forAttachOne— so both of those silently never ran throughcreate().Fix
Route
create()throughadd()in both branches: whensessionKeyis null, build an unsaved model via$this->related->newInstance($attributes)soadd()'s own$model->save()is the only place the model actually gets persisted (avoids a double-save). The deferred-binding path (sessionKey !== null) still usesparent::create(), unchanged, since the model needs to exist immediately for the deferred-binding table row that follows.The single-attachment sibling-deletion logic that used to live directly in
create()is removed from there —add()already has the identicalif ($this instanceof AttachOne) { $this->delete(); }check, so nothing is lost, just de-duplicated onto the one method bothsave()andcreate()now share.Testing
Added
testCreateFiresRelationEventsto bothAttachOneTestandAttachManyTest(confirmed each fires exactly once, with the correct relation name and model instance, by bindingmodel.relation.beforeAdd/afterAddand asserting on the callback args). AddedtestCreateReplacesExistingSingleAttachmenttoAttachOneTestandtestCreateDoesNotReplaceExistingAttachmentstoAttachManyTestto lock in the existing single-vs-multi behavior wasn't disturbed by moving the sibling-deletion check.Verified red-before/green-after: the new event tests fail against the pre-fix code (0 events fired) and pass after. Ran the full
tests/Databasesuite (222 tests, 1018 assertions) — all pass, no regressions.Fixes wintercms/winter#1147.
Implemented with AI assistance (Claude Code); verified against a real PHP 8.2 test environment before submitting.
Summary by CodeRabbit
Bug Fixes
Tests