Address code-review findings (#1 hardening, #2, #6, #7, #8)#4
Conversation
- Cap the referredUsers include at 100 rows, newest-first, so ?include=referredUsers can't do an unbounded read on a prolific referrer (#2). - Add a 10-attempt guard to InviteCode::generateUniqueCode() that throws instead of looping forever if the code space is exhausted (#7). - Constructor-inject NotificationSyncer into RecordReferral; keep the container only for the request-scoped PendingReferralState (#8). - Drop the unused Avatar and humanTime registry lookups from forum.js and rebuild dist (#6).
The forum-side grouping fully overrides core's NotificationList.content, which couples it to core internals. Add three guards: - Pin the comment to the core version the copy mirrors (2.0.0-rc.5) with a note to reconcile on core bumps. - Wrap the reproduced pipeline in try/catch that falls back to core's original content() on any error, so a future core refactor degrades to plain notification rendering instead of breaking the whole list. Also filter null NotificationType vnodes to match core. - Add an integration test locking the serialized contract the override reads: contentType == linkrobinsReferralRegistered and a users subject. Rebuild dist.
The referredUsers ToMany getter returned an Illuminate Collection, which core's ToMany::serializeValue array_maps over after it has been coerced to an array, turning each User model into its array form and throwing "resource(): Argument #1 ($model) must be of type object, array given" (HTTP 500) on any ?include=referredUsers request. This affected the relationship regardless of the row cap. Return a plain array of User objects (->all()) so each item stays an object through serialization. Adds integration coverage for the include (owner sees the referred user; non-owner does not), which the relationship never had.
|
Update from an end-to-end test in a live sandbox forum: the Pushed 270409f: return a plain array of User objects ( The rest of the referral flow was verified working end-to-end in the sandbox: code generation, registration with an invite code, referral recorded + counter incremented + code uses incremented, referrer notified, and the notifications endpoint returning 200 with the correctly-serialized referral notification. |
1 participant
Addresses the actionable items from the v1.4.0 code review (#3).
Changes
referredUsersinclude.?include=referredUsersdid an unbounded->get(); it is now ordered newest-first and capped at 100 with a documented constant, so a prolific referrer can't trigger a huge read/payload.generateUniqueCode(). The collision retry loop is now bounded to 10 attempts and throws aRuntimeExceptionon exhaustion instead of potentially looping forever.NotificationSyncer. Resolved via the constructor instead ofContainer::make(); the container is kept only for the genuinely request-scopedPendingReferralState.Avatar/humanTimeregistry lookups fromforum.js; dist rebuilt.NotificationList.contentoverride resilient. The full override now runs inside a try/catch that falls back to core's originalcontent()on any error, so a future core refactor degrades to plain notification rendering instead of breaking the whole notifications page. The comment is pinned to the core version it mirrors, and a regression test locks the serialized contract the override reads.The other review findings (#3 column-on-users, #4 source layout) were assessed as not valid and are not changed; see the triage notes in #3.
Verification
Note: while writing the #1 regression test I found a separate, more serious upstream issue where this extension's User-subject notification can make core's
/api/notificationsreturn a 400 on forums without a Post-subject notification type. That is reported and fixed upstream (flarum/framework#4815, flarum/framework#4816) and is out of scope for this PR.