# Season `group_id` at Accept Time — Implementation Brief **Decision:** D-8, resolved as **D + E + CF11** **Date written:** August 5, 2026 **Status:** ⏸ **HELD, August 5 2026 — see `D8_OPTIONS_COMPARISON.md`.** Andrew's read is that D is too much change for the value, and the numbers support it: the cleanup approach (**CF10 + B + E**) is ~7-9 hrs against D's ~25-30, touches **none** of the 29 `Invitation.group_id` sites, and introduces no concurrency changes. **This document is not cancelled** — the research in §4, §5.7 and Appendix A is accurate and useful either way, and if D is ever revisited it is ready to build. Read the comparison memo before this file. **Original status:** ⬜ **RESEARCHED AND SPECIFIED — NOT IMPLEMENTED. NOT REVIEWED BY CALEB.** **Review:** two independent adversarial passes against the working tree. **Six blockers and fifteen majors found and folded in — including two blockers in the first pass's own corrections. See Appendix C.** **Scope:** season groups only. The tournament flow is untouched throughout. **Supersedes:** **CF10** entirely. Amends **CF11** and **D-9**. Amends `getGroupSize()`. **Depends on:** CF8's `GroupDissolveService` (`b22734f`) and CF4's `LiveRegistrationTrait` (`b46c28b`), both merged to `main` and ~~both still unexecuted~~ **both LOCAL-tested as of Aug 5 (plan §10.8)**. > **[Aug 5] If D is ever revisited, note what has changed underneath it.** The chosen option shipped > instead: **CF10 + B + E**, specified in `GROUP_CLEANUP_BRIEF.md` and implemented on branch > `fix/solo_group_auto_delete` (`dde90b5`, `995e286`, plus an uncommitted third commit). > `GroupDissolveService` has gained two methods — `clearGroupIfOrphaned()` and > `clearInviterGroupIfOrphaned()` — and four classes now inject it. **CF10 is implemented**, so this > document's "Supersedes: CF10 entirely" line no longer describes reality. §4/Appendix A (all 29 > `Invitation.group_id` sites), §5.7 and Appendix C remain accurate and are why this file was held > rather than deleted. --- > ## ▶ READ THIS FIRST > > **Line numbers in this document were verified against the working tree on August 5, 2026, after > PR #124 (`1eb1a15`).** Every citation below was read out of the file, not copied from an earlier > document. They will still drift the moment anyone edits `GroupController.php` — **re-locate by > method name before trusting a number**, per the standing rule in > `ROSTER_RECONCILIATION_PLAN.md` §10.1. > > **This change touches capacity arithmetic and two payment-adjacent code paths.** It is not a > refactor. Read §5 (the model) and §11 (what breaks) before writing a line. > > **Four questions must be answered before any code is written** — §14 **Q1** (the locking mechanism), > **Q6** (the admin loses a page), **Q9** (the mutual-invitation guard) and **Q10** (the checkout path > has nothing to lock). Q10 is the one place this brief is knowingly incomplete, and it is on the > money path. > > **The rollback is not code-only.** §13. Reverting leaves every in-flight invitation with a NULL > `group_id`, which the restored accept paths will happily write onto acceptors. Rolling forward needs > no data step; rolling back does. > > ~~**Prerequisite that is not negotiable:** the six fixes merged in PR #124 have never been linted or > executed…~~ **✅ SATISFIED — §10.8 passed on LOCAL, Aug 5.** `GroupDissolveService` has now run. The > reasoning was sound and is worth keeping: building on unexecuted code means a bug in the service > surfaces as a bug in whatever is built on top, with several callers to bisect instead of one. --- > ## 👤 FOR CALEB — a 20-minute path through 2,000 lines > > This is your proposal, specified. **You do not need to read all of it to review it.** > > **Read these five, in this order:** > > | | | Why | > |---|---|---| > | **§3** | The target invariant | One paragraph. If you disagree with it, nothing else matters. | > | **§5.2** | The `getGroupSize()` rework | The cost you flagged. Three real behaviour changes, not zero. | > | **§5.4** | The races | **The riskiest part of the whole change.** Two review passes each found it wrong in a different way. | > | **§5.7** | `groups_locked` | Found last — D lets a new group appear after the roster is locked. | > | **§14** | Twelve open questions | Four of them block writing code. | > > **Then skim §10**, which argues that D makes Andrew's option E worth less than it looked and CF11 > worth more. That is the one place this document disagrees with the recommendation it is implementing. > > **Skip on a first pass:** §4 (the site-by-site trace), §6 (per-file instructions), §12 (35 test > cases), Appendix A. They are for whoever writes the code, not for the reviewer. > > **The four questions that block work:** **Q1** (is `SELECT … FOR UPDATE` on both registration rows the > right mechanism?), **Q6** (the admin loses the Group Invitations page for a pre-acceptance > invitation — accept, or build a replacement?), **Q9** (add a guard so two players cannot hold mutual > pending invitations?), **Q10** (checkout creates the registration *after* resolving the group, so > there is no row to lock — restructure?). > > **If you only have five minutes:** read §5.4 and answer Q1. --- ## 1. What this document is `SESSION_HANDOFF.md` records decision **D-8** as open with five candidate answers (A–E) and a recommendation of **D + E, with CF11 demoted to a rare fallback for route 4**. Andrew has taken the recommendation. This document is the research and the implementation specification for all three, so that a later session can build it without re-deriving anything, and so Caleb can review the design before code exists rather than after. **What it is not:** it is not a data repair. Per Andrew's July 28 call (plan §7) there is no data repair this cycle. Registrations that already carry a stranded solo `group_id` stay exactly as they are; §9's Disband button and the existing admin Dissolve are how those get cleaned up, one at a time, as they surface. **§12 includes read-only SQL to count them so the size of that backlog is known** — running it changes nothing. ### The three parts | Part | What it does | Closes | |---|---|---| | **D** | Do not create a season `group_id` until an invitation is **accepted** | routes 1, 2, 3 | | **E** | Deleting the last pending invitation of a group of one clears the group | route 4 residue | | **CF11** | A solo manager can Disband their own group | route 4 | Route numbering is `SESSION_HANDOFF.md`'s and plan §10.3 CF11's: | # | How a player ends up a permanent "group of one" | Closed by | |---|---|---| | 1 | Invite fails validation after the `group_id` was already saved | **D** | | 2 | Invitee declines, or accepts a rival group | **D** | | 3 | Invitee never answers; inviter deletes the invitation | **D** | | 4 | Group had members; all of them left or were removed | **CF11**, helped by **E** | **Read §10 before assuming E and CF11 keep the shape they had in the pre-D world.** D changes what they are for. That section is the one part of this brief that argues with the decision it is implementing, and it is deliberate. --- ## 2. The problem, restated from the code `GroupController::invite()` mints and **saves** a season `group_id` onto the inviter's own registration before the invitee has been resolved, let alone validated: ```php // GroupController.php:926-941 — the season branch of invite() else { $season = $registration->get('season')->entity; $team = NULL; $max_group_size = $season->getMaxGroupSize(); // Get or create group_id $group_id = $registration->get('group_id')->value; if (!$group_id) { $group_id = $this->uuid->generate(); $registration->set('group_id', $group_id); $registration->save(); // ← committed here, line 936 } // Check current group size $current_size = $this->getGroupSize($season->id(), $group_id); } ``` **Four early `return`s sit between that `save()` and the invitation being created** — verified by reading the method end to end. (Two more, at **989** and **1008**, are inside the `if ($is_tournament)` arm at **983** and are unreachable on this path.) | Line | Condition | |---|---| | 945 | `$current_size >= $max_group_size` — group already full | | 961 | no `$invitee_email` — empty search box | | 1034 | invitee is already in this group, or in another group for the season | | 1089 | a non-declined invitation to that address already exists | Every one of them `return`s without unwinding the save. So a player who mistypes an address, or picks someone who turns out to be in another group, walks away holding a `group_id` with no invitation behind it — and that state is a one-way door: - `invite()`'s own Check 1 (**1025-1034**) blocks anyone else from inviting them: *"This player is already in another group for this season."* - `RosterBuilderController::mergeToGroup()` (**319**) and `createGroup()` (**534**, **541**) refuse to drag them into a group. - `leaveGroup()` (**1957-1960**) refuses them by design: *"As the group manager, you cannot leave your own group."* - `manage()` gates the "invitations addressed to you" list on `if (!$group_id)` (**602**), so they cannot even *see* an invitation that was already waiting. The only exit is an admin using Dissolve Group. **That is the admin-ticket load Andrew flagged**, and it is why this is being fixed at the source rather than managed. ### What is already handled, and must not be re-fixed Traced August 4 and re-confirmed August 5: **all three season accept paths already decline competing invitations** for that season — - `GroupController::acceptSeasonInvitation()` **1486-1500** - `RegistrationController::acceptSeasonInvitationDirectly()` **509-523** - `OrderCompleteSubscriber::createSeasonRegistration()` **570-572** → `declinePendingInvitationsForRegistration()` **999** So when A and C both invite B and B accepts A, **C's invitation is already auto-declined** and stops burning C's cap. What is *not* cleared is C's own `group_id`. That is the entirety of route 2, and D removes it by never creating C's `group_id` in the first place. --- ## 3. The target invariant > **A season `Registration.group_id` is non-empty if and only if two or more players have agreed to be > a group.** Corollaries the implementation must preserve, and that §12's SQL asserts: 1. A pending invitation implies **nothing** about the inviter's `group_id`. It may be set (the inviter already has a group) or NULL (this is their first). 2. A group with exactly one live member is a **transient** state reachable only by route 4, not a steady state produced by inviting. 3. `Invitation.group_id` is **NULL while `pending`** and **written at accept time**. See §5.3 — this is a deliberate choice, and it is what keeps ten existing queries working unchanged. ### The derivation rule — the one sentence the whole change turns on > **The group of a pending invitation is whatever group the *inviter* is in, or a new one minted the > moment someone accepts.** Everything in §6 is mechanical consequence of that sentence. --- ## 4. Verified current-state trace **The handoff says `Invitation.group_id` is "load-bearing in seven places". It is load-bearing in twenty-nine.** The full inventory is in Appendix A. This section is the part that matters for design. ### 4.1 The Invitation entity, in full `src/Entity/Invitation.php`, entity id `ccsoccer_invitation`. Thirteen base fields: | Field | Type | Req | Note | |---|---|---|---| | `inviter` | ref → user | ✔ | **The pivot of this whole change.** | | `invitee_email` | email | ✔ | de-facto identity key | | `invitee` | ref → user | | set only if they already have an account | | `season` | ref → season | | *not set for tournament invites* | | `group_id` | string(255) | | view-display only, **no form widget** | | `team` | ref → team | | tournament invites only | | `token` | string(64) | | magic-link token | | `status` | list_string | ✔ | `pending` \| `accepted` \| `declined` \| `expired`; default `pending` | | `message`, `responded_at`, `notified`, `created`, `changed` | | | | Three facts that shape the design: 1. **There is no `tournament` field on Invitation.** Tournament context is reached only through `team` → `team.tournament`. Querying a `tournament` field on an invitation throws — that is the live PROD fatal CF2 fixes (`GroupInvitationsForm.php:1449-1456` documents it). **Do not add a `tournament` condition anywhere in this work.** 2. **There is no `getGroupId()`/`setGroupId()` accessor.** All ~15 reads use raw `$invitation->get('group_id')->value`. There is no single choke point to intercept, which is why §6 has to enumerate sites rather than change one method. 3. **`Invitation::accept()` is called from two places** — `OrderCompleteSubscriber.php:541` (season) and `:808` (tournament). Every other accept path sets `status` and `responded_at` by hand. `decline()` has three callers: `GroupDissolveService.php:198`, `OrderCompleteSubscriber.php:1023` and `:1040`. Neither method is a usable hook point as things stand; §6 commit 3 routes the season accepts through `accept()` as a side effect. ### 4.2 The hazard that decides the shape of the fix ⚠ *corrected after review* Under D, pending invitations carry a NULL `group_id`. **Two different query styles react to that in two different ways, and only one of them is survivable.** **Style 1 — `loadByProperties(['group_id' => NULL, …])` throws an uncaught exception.** It does *not* silently return the wrong rows. `EntityStorageBase::buildPropertyQuery()` casts every value to an array (`(array) NULL` → `[]`) and calls `condition($name, [], 'IN')`; `Condition::condition()` then throws: ```php // web/core/lib/Drupal/Core/Database/Query/Condition.php:106-108 if (empty($value) && is_array($value)) { throw new InvalidQueryException(sprintf("Query condition '%s %s ()' cannot be empty.", $field, $operator)); } ``` **So every unguarded `loadByProperties` site is a 500, not a wrong number.** That is loud rather than silent, which is a mercy — but one of them sits in `OrderCompleteSubscriber`, after payment. **Style 2 — `getQuery()->condition('group_id', NULL)` compiles to `= NULL` and matches nothing.** Silent, returns zero rows. Two sites use this style: `GroupController:838-841` and `OrderCompleteSubscriber:508-515`. Six sites have a truthiness guard and are safe either way: `manage()` 589, `removeMember()` 1860, `leaveGroup()` 1989, `TeamBalancerService` 1086, `Team::getRosterStats()` 684, `myRegistrations()` 254. **Six sites are unguarded and must be re-keyed, not merely NULL-checked:** | Site | File:line | Style | Failure | |---|---|---|---| | `myRegistrations()` invitation card | `GroupController:270-274` | 1 | **fatal** on My Registrations for anyone holding a pending invitation | | `manage()` sent-invitations list | `GroupController:510-515` | 1 | **fatal** — but unreachable, gated at 501; see §6.4 | | `invite()` duplicate check | `GroupController:1054-1059` | 1 | **fatal** on the second invite | | `getGroupSize()` | `GroupController:1931-1936` | 1 | **fatal** on every season invite | | `GroupInvitationsForm::loadPendingInvitations()` | `:341-345` | 1 | **fatal** on the admin page | | `GroupDissolveService::dissolveGroup()` | `:191-194` | 1 | **fatal** on every dissolve | | `OrderCompleteSubscriber` capacity guard | `:508-515` | 2 | silent — confirmed count reads 0, guard never fires | | `invitationHasMatchingRegistration()` | `:838-841` | 2 | silent — safe in practice, accepted-only (§5.3) | That table is the real cost of D. **It also means D cannot be shipped incrementally by "leaving the NULLs and seeing what happens" — the first invite after deploy fatals.** All six re-keys are mandatory and all six re-key to the same predicate. ### 4.3 The three sites that read the invitation's `group_id` to write a registration These are the accept paths. All three do the same two lines and all three must change. ```php // GroupController.php:1420-1422 — acceptSeasonInvitation() protected function acceptSeasonInvitation($invitation) { $season_id = $invitation->get('season')->target_id; $group_id = $invitation->get('group_id')->value; // ← becomes NULL under D ... // GroupController.php:1458-1462 // Accept - update registration with group_id $registration->set('group_id', $group_id); $registration->set('invited_by', $invitation->get('inviter')->target_id); $registration->set('invitation_status', 'accepted'); $registration->save(); ``` ```php // RegistrationController.php:474-487 — acceptSeasonInvitationDirectly() protected function acceptSeasonInvitationDirectly($invitation, $season, $registration, $inviter_name) { $group_id = $invitation->get('group_id')->value; // ← becomes NULL under D if ($registration->get('group_id')->value) { … return; } $registration->set('group_id', $group_id); ``` ```php // OrderCompleteSubscriber.php:495-543 — createSeasonRegistration() if ($invitation && $invitation->isPending()) { $candidate_group_id = $invitation->get('group_id')->value; // ← becomes NULL under D $confirmed_count = …->condition('group_id', $candidate_group_id)… // ← reads 0, guard dies ``` **Note that the site has two Accept buttons routing through different controllers** — My Registrations goes to `GroupController`, the Register page to `RegistrationController` — plus checkout, plus the admin form. **Four accept paths, not two.** Both of the first two were broken by the Aug 4 bug for exactly this reason. Any change to accept semantics has to be made four times or extracted once. **This is why §6 merges the "stop minting" and "start minting at accept" work into one commit.** A build in which minting has stopped but the accept paths still read the invitation writes `group_id = NULL` onto the acceptor while setting `invited_by` and `invitation_status = 'accepted'` — manufacturing, on the acceptor's row, exactly the orphan state D exists to prevent. ### 4.4 The cap, and a pre-existing hole in it ```php // GroupController.php:1919-1939 protected function getGroupSize($season_id, $group_id) { // Count accepted registrations $accepted = $this->entityTypeManager->getStorage('ccsoccer_registration') ->loadByProperties(['season' => $season_id, 'group_id' => $group_id]); // 1924-1928 // Count pending invitations $pending = $this->entityTypeManager->getStorage('ccsoccer_invitation') ->loadByProperties(['season' => $season_id, 'group_id' => $group_id, 'status' => 'pending']); return count($accepted) + count($pending); } ``` Two things about it: - **One caller only** — `invite()` line 940. (`getTeamSize()` at 2011 is its tournament twin and is out of scope.) - **`invite()` never checks that the caller is the group manager.** It checks ownership of the registration (875) and nothing else; the manager-only rule lives in the Twig template (`ccsoccer-group-manage.html.twig:107`, `{% if is_manager … %}`). A non-manager member who POSTs the invite route directly gets their group's `group_id` at 932 and creates an invitation into it. **So "pending invitations of a group" is not the same set as "pending invitations sent by the manager"**, and any re-key that assumes it is will under-count. §5.2 handles it. ### 4.5 The two audit-trail invitations that are *not* invite-time `RosterBuilderController::mergeToGroup()` **394-405** and `createGroup()` **569-580** both create an Invitation with `'status' => 'accepted'` and a `group_id`, as an audit record of an admin drag in the season Roster Builder. **These are accept-time by nature and must keep writing `group_id`.** They are not a counter-example to D; they are the rule. `TournamentRosterBuilderController` has no equivalent — it never touches `Invitation.group_id` at all. ### 4.6 Verified negatives — things that look like they should change and must not - **`GroupController::deleteInvitation()` (1334-1384) and `nudge()` (1180-1329) never read or write `group_id`.** `deleteInvitation()` hard-deletes and returns. That is the gap **E** fills. `nudge()` reads only `notified`, so its 48-hour throttle is unaffected. - **`GroupController::declineInvitation()` (1625+) never touches `group_id`.** Correct as-is under D. - **`available()` (`RegistrationController:61`) and `userSearch()` (`GroupController:660-783`) never touch `group_id`.** Unaffected. - **`WaitlistManagerService` contains no `group_id`.** Unaffected. - **No view config references `group_id`.** The two JS hits (`js/roster-builder.js:784`, `js/tournament-roster-builder.js:668`) POST an existing group id to the merge endpoints, sourced from `data-group-id` rendered off Registration/Team data. Unaffected. - **`Team::getRosterStats()`'s season branch (681-692)** keys pending invitations off `Team.group_id`. Season Teams are not given a `group_id` by any code path found — `TournamentTeamManager::createTeam()` is the only minter. **This branch is very probably already inert.** Verify (§14 Q5) rather than assume; if it is inert, leave it alone and add a comment, do not delete it. - **`_ccsoccer_repair_legacy_team_group_ids()` (`ccsoccer.install:4678`) repairs Registration rows only.** Invitations carrying a legacy `team_` group_id were never repaired and never audited. Out of scope here; recorded so it is not discovered as a surprise. **Templates are NOT a verified negative.** An earlier draft of this brief claimed no template change was needed. That is wrong — see §6.5 and §6.6. `templates/` contains no literal `group_id`, which is what misled the first pass, but two templates render data that changes meaning under D. --- ## 5. The design ### 5.1 One new service, not seven edits Every re-key in §4.2 needs the same two questions answered: 1. *Which uids are in this group?* 2. *Which pending invitations belong to this group?* Write them once. **New service `ccsoccer.group_membership`, class `Drupal\ccsoccer\Service\GroupMembershipService`.** A service rather than a trait because `TeamBalancerService`, `GroupInvitationsForm`, `GroupDissolveService`, `CancelRegistrationForm` and three controllers all need it, and because the mint needs `@database` injected (§5.4) — which a trait cannot do cleanly. It uses `LiveRegistrationTrait::isDeadRegistrationStatus()` so there is exactly one definition of "live" (D-4 / D-13). **Do not introduce a second.** ```php final class GroupMembershipService { use LiveRegistrationTrait; /** LIVE member registrations of a season group, keyed by uid. @return Registration[] */ public function getMemberRegistrations(int $season_id, ?string $group_id): array; /** Live member uids of a season group, or [$fallback_uid] when no group yet. @return int[] */ public function getMemberUids(int $season_id, ?string $group_id, ?int $fallback_uid = NULL): array; /** Pending invitations belonging to a group — i.e. sent by a live member. @return Invitation[] */ public function getPendingInvitations(int $season_id, ?string $group_id, ?int $fallback_uid = NULL): array; /** Accepted members + pending invitations. Replaces GroupController::getGroupSize(). */ public function getGroupSize(int $season_id, ?string $group_id, ?int $fallback_uid = NULL): int; /** Count of LIVE registrations carrying this group_id. 0 for NULL/''. */ public function getLiveMemberCount(int $season_id, ?string $group_id): int; /** * THE MINT AND THE JOIN, as one locked operation. * * Resolves the inviter's group (minting one if this is the first acceptance) * AND writes group_id / invited_by / invitation_status onto the acceptor — * both under the same row locks. Splitting the two leaves a third race open; * see §5.4. Callers stamp the invitation afterwards and do nothing else. * * Call ONLY from an accept path, after every guard has passed. * Throws GroupMintConflictException when the acceptor is already grouped. */ public function joinInviterGroup( RegistrationInterface $inviter_registration, RegistrationInterface $acceptor_registration ): string; } ``` **Six methods.** `joinInviterGroup()` does the mint *and* the acceptor's write — an earlier draft called it `resolveGroupForInviter()` and left the acceptor's write to the caller, which the second review showed leaves a race open (§5.4 race C). **If you find `resolveGroupForInviter` anywhere in a later document, it is the superseded name.** `getMemberRegistrations()` exists because two callers need each member's `invited_by` to render the Manager badge, which cannot be derived from a uid list (§6.5). ### 5.2 `getGroupSize()` — the rework, and the three ways it genuinely differs ``` getGroupSize(season, group_id, fallback_uid): if group_id is non-empty: members = LIVE registrations in season with group_id → uids accepted = count(members) else: members = [fallback_uid] accepted = 1 # the inviter's own registration if members is empty: return 0 # never pass [] into an IN condition pending = count(invitations WHERE season = season AND status = 'pending' AND inviter IN members) return accepted + pending ``` **An earlier draft of this brief claimed the rework was "exactly cap-neutral". It is not, and the review was right to reject that.** There are three real divergences, all of which are corrections rather than accidents, but all of which change counts on live data and must be tested rather than assumed: **Divergence 1 — cancelled members stop occupying slots.** Today `accepted` is unfiltered (**1924-1928**); this is **CF3 site 1**, and a cancelled member currently holds a group slot forever. The new count is status-filtered. **Groups on PROD carrying a cancelled row will get smaller.** This is the intended behaviour of CF3 and is unavoidable here — the method is being rewritten and leaving it unfiltered would mean writing the bug twice. **Say so in the commit message so CF3's own commit does not filter it again.** **Divergence 2 — an invitation sent by someone who has since left the group stops counting.** `leaveGroup()` (**1980-2002**) clears the leaver's own `group_id` and declines the leaver's own *accepted* invitation. It does nothing to invitations the leaver **sent** — and §4.4 establishes that members can send them. So: member M of group X sends an invite, then leaves X. The invitation keeps `group_id = X`. The old count includes it; the new `inviter IN members(X)` does not. **The cap differs by one, and it differs before D even lands.** Arguably the new answer is right (the person who vouched for that invitee is gone), but it is a behaviour change and belongs in the commit message. **Divergence 3 — the same leaver's invitation appears in the manager's list.** §6.4's change to `manage()`'s sent-invitations query is scoped by `inviter`, so M's orphaned invitation now surfaces on **M's** page rather than nobody's. Better than today, still different. **What is unchanged: the normal path arithmetic.** Season max 6, manager M, no group, no cancellations: | Action | old count | new count | outcome | |---|---|---|---| | M invites #1 | group minted, accepted 1 + pending 0 = 1 | accepted 1 (fallback) + pending 0 = 1 | allowed | | M invites #2 … #5 | 2 … 5 | 2 … 5 | allowed | | M invites #6 | 1 + 5 = 6 | 1 + 5 = 6 | **blocked** ✔ | | all 5 accept | group of 6 | group of 6 | ✔ | Identical at every step. **§12.2 case 9 is the test that proves it.** ### 5.3 What `Invitation.group_id` means after this change | Invitation status | `group_id` | |---|---| | `pending` | **always NULL** | | `accepted` | the group joined, **written at accept time** | | `declined` | whatever it was — NULL if never accepted | | `expired` | never written (D-3) | **Writing it at accept rather than abandoning the field is deliberate.** It keeps four existing queries working with no change at all — all four verified against the tree: - `removeMember()` **1861-1867** — decline the removed member's accepted invitation, keyed on `group_id` + `status = accepted` - `leaveGroup()` **1990-1996** — same shape, for self-leave - `invitationHasMatchingRegistration()` **836-841** — sole caller is `invite()` **1073**, correctly gated on `$status === 'accepted'` - `GroupDissolveService` **191-194** — the `accepted` half of its decline All four hold **only if every one of the four accept paths stamps the field.** Miss one and these degrade silently, months later, as a re-invite that will not go through. **Do not "clean up" the field, and do not skip the stamp in the path that looks least important.** ### 5.4 The races that D introduces ⚠ *rewritten twice after review* **This is the most important subsection in the brief. Two review passes each found it wrong, in different ways. Treat everything here as load-bearing and re-derive it before implementing.** Today the group is minted once, by one user, in one request. Under D it is minted by whichever *invitee* accepts first. There are **three** distinct races, and each obvious fix closes only one. **Race A — two invitees of the same inviter accept simultaneously.** Both read the inviter's `group_id` as NULL, both mint, last write wins. The two acceptors end up in two different groups, one of which the inviter is not in. **Race B — mutual invitations.** *This one is created by D and does not exist today.* Today, when B invites A, `invite()` has already minted A's `group_id` at **933-937**, so Check 1 (**1022-1035**) sees it, `$existing_group_id != $group_id`, and the second invitation is refused at **1034**. Under D neither party has a `group_id`, `if ($existing_group_id)` at **1025** is FALSE, and **both invitations are created.** If A and B then accept each other's simultaneously, each mints a group onto the other's registration: **A lands in B's group, B lands in A's**, and — because both rows now carry `invited_by` — `is_manager` (`empty(invited_by)`; **419**, **484**, **264**) is FALSE for both. Neither can Disband (§9 guard 5 requires an empty `invited_by`), neither can be invited by anyone else (Check 1 blocks them), and only `leaveGroup()` is an accidental way out. **That is strictly worse than the bug being fixed.** **Race C — one invitee accepts two invitations from two different inviters simultaneously.** *Found by the second review; it is the race the first fix created.* B holds pending invitations from A and from C. Two requests: request 1 locks {A, B}, mints G1 onto A; request 2 locks {C, B}. If the acceptor's own row is written **outside** the lock — by the caller, after the service returns — then request 2 still reads B as ungrouped, mints G2 onto C, and both callers then write B's row. Last write wins. **A is left alone in G1: a permanent group of one with an empty `invited_by`** — the exact shape D exists to remove. A per-inviter lock closes A and **not** B — the two requests take different lock names. Locking both rows closes A and B and **not** C, unless the acceptor's write also happens inside the lock. **That is why `joinInviterGroup()` writes the acceptor's row itself rather than returning a string for the caller to apply.** **Two further constraints the mechanism must satisfy:** - **It must work inside a transaction.** `SqlContentEntityStorage::save()` wraps every save in a transaction on the default connection, and `OrderCompleteSubscriber` runs inside Commerce's order transition. Drupal's `lock` service is `DatabaseLockBackend` on `@database`: its semaphore row is written inside the caller's open transaction and is **invisible to concurrent requests until commit**. It provides no mutual exclusion in the checkout path. - **`loadUnchanged()` is not sufficient, and neither is a locking read followed by `loadUnchanged()`.** `loadUnchanged()` is `resetCache([$id]); return $this->load($id);` — a plain, *non-locking* SELECT. A `SELECT … FOR UPDATE` reads the latest committed row but **does not refresh the transaction's read view**; a subsequent ordinary read still returns the snapshot value. So the decision must be made **from the values the locking read itself returned.** An earlier draft of this section discarded `fetchAssoc()` and re-read through the entity API — which reintroduced exactly the staleness the lock was taken to avoid. *This is subtle and it is the single easiest thing to get wrong here.* **The mechanism: row locks on both registrations in id order; decide from the locked read; write both rows before releasing.** ```php // GroupMembershipService::joinInviterGroup() // // SELECT … FOR UPDATE is a "current read" in InnoDB: it reads the latest // committed row and blocks on any uncommitted write to it. It is the only // primitive here that works inside OrderCompleteSubscriber's transaction, where // Drupal's lock service does not. // // BOTH rows are locked, in ascending id order: // - the inviter's, because that is where the group_id is minted (race A) // - the acceptor's, because mutual invitations mean the two requests would // otherwise lock disjoint rows (race B), and because one acceptor with two // inviters would otherwise be written twice (race C) // Ascending id order is what prevents deadlock between symmetric requests. $inviter_id = (int) $inviter_registration->id(); $acceptor_id = (int) $acceptor_registration->id(); $ids = [$inviter_id, $acceptor_id]; sort($ids, SORT_NUMERIC); $txn = $this->database->startTransaction(); try { // Capture the LOCKED values. Do not re-read these through the entity API — // an ordinary read inside this transaction returns the pre-lock snapshot. $locked = []; foreach ($ids as $id) { $locked[$id] = $this->database->select('ccsoccer_registration', 'r') ->fields('r', ['id', 'group_id']) ->condition('r.id', $id) ->forUpdate() ->execute() ->fetchAssoc(); } // Race C: the acceptor may have joined a group while we waited on the lock. if (!empty($locked[$acceptor_id]['group_id'])) { throw new GroupMintConflictException('acceptor already grouped'); } $group_id = $locked[$inviter_id]['group_id'] ?: NULL; if (!$group_id) { $group_id = $this->uuid->generate(); $inviter = $this->entityTypeManager->getStorage('ccsoccer_registration') ->load($inviter_id); $inviter->set('group_id', $group_id); $inviter->save(); // entity API for the WRITE only } // The acceptor's write happens HERE, inside the lock — not in the caller. $acceptor = $this->entityTypeManager->getStorage('ccsoccer_registration') ->load($acceptor_id); $acceptor->set('group_id', $group_id); $acceptor->set('invited_by', $inviter_registration->get('player')->target_id); $acceptor->set('invitation_status', 'accepted'); $acceptor->save(); return $group_id; } catch (\Throwable $e) { $txn->rollBack(); throw $e; } // $txn commits when it goes out of scope. ``` **Read from the locked array; write through the entity API.** The entity write is fine — it is the *read* that must not go through `load()` inside the transaction. Using the entity API for the write keeps `postSave()` hooks and cache invalidation working. **Three practical notes:** - **Confirm the table and column names first** — `drush sqlq "DESCRIBE ccsoccer_registration"`. A raw `select()` gets no help from the entity API if they are wrong. **§14 Q8.** - **`forUpdate()` is a no-op on SQLite** (`core/modules/sqlite/.../Select.php`). If any kernel or functional test runs on SQLite the locking is silently absent — the tests will pass and prove nothing. §12.2 cases 24-26 must run on MySQL/MariaDB. - **`startTransaction()` inside an already-open transaction opens a savepoint**, not a new transaction. That is fine for the locks (they are held to the *outer* commit) but it means the `rollBack()` in the catch unwinds only this savepoint — which is the desired behaviour on the checkout path. **Callers must handle `GroupMintConflictException`:** - interactive accept paths → *"You have already joined a group for this season."* + redirect - `OrderCompleteSubscriber` → **catch, log at `error`, flag the order, create the registration ungrouped.** Payment has been taken; an ungrouped registration is recoverable, a fatal in the subscriber is not. **Two alternatives, recorded so they are not re-proposed:** - **Deterministic group id** (`$inviter_registration->uuid()`) makes concurrent mints converge with no locking at all — genuinely elegant, and it closes race A completely. Rejected because the id would be **reused** if that manager's group is dissolved and they later start a new one, and any stale row still carrying the old value (audit class SB, explicitly *not* repaired this cycle) would silently re-join the new group. It also does nothing for races B or C. - **Drupal's `lock` service.** Rejected for the transaction-visibility reason above. It would work for the two interactive paths and fail exactly where the money is. **§14 Q1 asks Caleb to confirm the `forUpdate()` approach.** It is the load-bearing piece of this design and the piece most likely to be wrong in a way testing will not reveal. ### 5.5 Race B has a cheaper partial answer worth considering Race B exists because D lets two players hold mutual pending invitations. A guard in `invite()` — *"refuse if the invitee already has a pending invitation out to you for this season"* — removes the state entirely, costs one query, and is easy to reason about. It does not remove the need for locking (race A remains), but it turns the worst outcome from "two manager-less groups" into "an error message". **§14 Q9.** ### 5.6 Accept-time capacity — in scope or not Today the cap is enforced at **invite** time only. **Three of the four accept paths have no capacity check at all** — `GroupController::acceptSeasonInvitation()`, `RegistrationController::acceptSeasonInvitationDirectly()`, and `GroupInvitationsForm::acceptInvitation()`. Only `OrderCompleteSubscriber` has one (**508-533**). Already tracked in `OUTSTANDING_ISSUES.md` P5 as *"Accept-from-My-Registrations has no capacity check"*. D does not make this worse arithmetically, but it moves the moment a group comes into existence to accept time, which makes the gap look increasingly odd. **Recommendation: do not bundle it.** This change already touches four accept paths and the capacity arithmetic; adding a new refusal path to all four in the same commit makes it unreviewable and un-revertable independently — the exact reasoning behind **D-14** (why CF5 and CF9 were split). Ship D first, then the accept-time cap as a follow-up that will be four small calls to `GroupMembershipService::getGroupSize()` once this service exists. **§14 Q2.** ### 5.7 `groups_locked` is enforced on one accept path out of four — and D makes that worse ⚠ **Verified by grep across all four accept paths.** `groups_locked` appears in `GroupController` only: | Path | Checks `groups_locked`? | |---|---| | `GroupController::acceptSeasonInvitation()` — My Registrations Accept | **yes**, 1424-1429 | | `RegistrationController::acceptSeasonInvitationDirectly()` — Register-page Accept, reached from the magic link at `:117` | **no** — the string does not appear in the file | | `OrderCompleteSubscriber::createSeasonRegistration()` — checkout | **no** — the string does not appear in the file | | `GroupInvitationsForm::acceptInvitation()` — admin | **no** — arguably correct; an admin override is the point | `invite()` (**882-889**), `leaveGroup()` (**1975**), `removeMember()` (**1715-1723**) and `manage()`'s display (**618-620**) all honour it. So the gate is real everywhere a player *starts* something and missing on three of the four places a player *finishes* one. **Today this is a bounded pre-existing bug:** accepting after the lock moves a player into a group that already existed. The roster builder's grouping does not change, only its contents. **Under D it becomes a different bug.** The group is created at accept time — so a pending invitation accepted after the admin locks groups **produces a group_id that did not exist when they locked**. `TeamBalancerService` and `RosterBuilderForm` read no lock flag at all (confirmed by grep); they simply bucket on whatever `group_id`s are present. A new bucket appearing mid-build changes the Suggest Rosters output underneath the admin, silently. **That is precisely what `groups_locked` exists to prevent**, and roster building is the forcing function for this entire work cluster. **Recommendation: add the `groups_locked` check to `acceptSeasonInvitationDirectly()` and to `OrderCompleteSubscriber` as part of commit 3**, not as a follow-up — unlike the capacity check in §5.6, this one is *created* by D rather than merely adjacent to it, and it is three lines copied from `acceptSeasonInvitation()` **1424-1429** rather than a new refusal path to design. **The checkout case needs a decision, not a copy.** Payment has been captured by the time `OrderCompleteSubscriber` runs. Refusing the group there means creating the registration ungrouped and telling nobody — the same shape as the existing capacity-exceeded branch (**518-533**), and it should reuse that mechanism: log, flag the order, carry on. **Do not throw, and do not fail the order.** **§14 Q12.** Whether locking groups should also block a *paid* checkout from joining a group is a league-policy question, not a code one. **One line while you are in there:** `joinInviterGroup()` sets `group_id` on the inviter's registration and **must not touch their `invited_by` or `invitation_status`.** A group manager keeps `invitation_status = 'none'` — that is decision **D-11**, *"'none' means in this group, not by invitation"*. Only the acceptor's row gets `'accepted'`. --- ## 6. The work, in order **Five commits.** An earlier draft had six, with "stop minting" and "start minting at accept" split — **that split is unsafe** (§4.3) and has been merged into commit 3. Repo convention applies to every file: archive to `archive/_.php` before editing (**check the filename is not already taken** — that bit us on July 27), then `ddev php -l`, then LOCAL click-through. > **`ddev drush cr` is mandatory** after commit 1 (new service) and again after commit 2 (constructor > and `services.yml` changes in seven classes). A stale container fatals the page. | # | Contents | Behaviour change? | |---|---|---| | 1 | `GroupMembershipService` + exception class + services.yml | none | | 2 | Inject it everywhere; re-key the broken queries; fix the `manage()` / `myRegistrations()` display blocks | **yes, small** — see §5.2 | | 3 | **D** — stop minting at invite; mint at accept in all four paths | **yes, the flip** | | 4 | **E** — delete-last-invitation clears an empty group | yes | | 5 | **CF11** — Disband button | yes | > **Two service injections, not one.** Commit 2 injects `ccsoccer.group_membership` into seven > classes. Commits 4 and 5 also need `ccsoccer.group_dissolve` on **`GroupController`, which does not > have it today** — its constructor takes exactly six arguments (`entity_type.manager`, `current_user`, > `uuid`, `ccsoccer.notification`, `flood`, `database`) and `ccsoccer.group_dissolve` is injected only > into `CancelRegistrationForm` and `GroupInvitationsForm`. **Do it once, in commit 2**, so there is > one container change rather than three. ### Commit 1 — `GroupMembershipService` **New:** `src/Service/GroupMembershipService.php` **New:** `src/Exception/GroupMintConflictException.php` *(extends `\RuntimeException`)* **Edit:** `ccsoccer.services.yml` ```yaml ccsoccer.group_membership: class: Drupal\ccsoccer\Service\GroupMembershipService arguments: ['@entity_type.manager', '@logger.factory', '@database', '@uuid'] ``` Implement the six methods in §5.1 plus the mint exactly as §5.4. **No caller yet.** Pure addition. Implementation notes that will otherwise cost an hour each: - **`loadByProperties()` cannot express `NOT IN`.** Use `getQuery()->accessCheck(FALSE)` with `->condition('status', ['cancelled','expired'], 'NOT IN')`, or load and filter in PHP with `isDeadRegistrationStatus()`. The plan hits this same wrinkle at CF3's ten sites and CF11's recount; pick one style here and reuse it. - **`inviter IN []` throws** (`Condition.php:106`, the same exception as §4.2). Guard: if the member-uid set is empty, return `[]` / `0` before querying. - **Never pass a NULL `group_id` into a condition.** Branch first — see §4.2. - **Confirm the real table/column names** before writing the raw `forUpdate()` select (§14 Q8). - **There is no index on `ccsoccer_invitation.group_id` or `.inviter`.** At ~200 players/season the invitation table is small enough that this is irrelevant; recorded so nobody assumes an index exists. ### Commit 2 — inject the services; re-key; fix the display blocks | File | Method | Line | Change | |---|---|---|---| | `GroupController.php` | constructor + `create()` | 74-95 | inject **both** `ccsoccer.group_membership` and `ccsoccer.group_dissolve` | | `GroupController.php` | `myRegistrations()` | 139-183 | **whole `$group_status` block** — see §6.3 | | `GroupController.php` | `myRegistrations()` | 243-286 | invitation card — see §6.5 | | `GroupController.php` | `manage()` | 475, 501, 510-515, 589-596, 602 | **whole display block** — see §6.4. (577 and 598 need no edit; they become correct once 475 and 589 do.) | | `GroupController.php` | `invite()` | 1052-1059 | duplicate check → `invitee_email` + `season` + `inviter IN members-of-caller's-group` | | `GroupController.php` | `getGroupSize()` | 1919-1939 | **delete**; replace the call at 940 with the service | | `GroupPane.php` | `submitPaneForm()` | 238-247 | drop the cached `group_id` key — §6.6 | | `GroupInvitationsForm.php` | `loadPendingInvitations()` | 330-347 | season branch → service. **Tournament branch (`team`-keyed) untouched.** Signature is `(string $group_id, array $context)` with **no season id** — the season is `$context['id']`, set in `determineContext()` at **:292** and stored to form state at **:224**; widen the signature to `?string $group_id`. | | `GroupDissolveService.php` | `dissolveGroup()` | **insert at 139** | pending sweep goes **after** the registrations are loaded at :138 and **before** the clear loop at :140-188. **Not at 190-201** — by then the membership it needs has been erased. §6.7 | | `CancelRegistrationForm.php` | `submitForm()` | 386-408 | **new else-branch** — §6.9 | | `TeamBalancerService.php` | `getRosterState()` | move 1083-1089 below 1119 | §6.8 | | `RosterBuilderController.php` | `mergeToGroup()` | 385-389 | pending count → service | | `ccsoccer.services.yml` | — | — | add `@ccsoccer.group_membership` to `ccsoccer.team_balancer` and to `OrderCompleteSubscriber`'s definition | | `ccsoccer.module` | `hook_theme()` | 80-102 | add `'can_disband' => FALSE,` to `ccsoccer_group_manage`'s variables — **§9's button silently never renders without it** | **Do not reach for `\Drupal::service()`** in `GroupController`, `TeamBalancerService`, `GroupDissolveService` or `GroupInvitationsForm` — all four already do dependency injection properly. `RegistrationController` and `OrderCompleteSubscriber` are the exceptions; see §C3-c and §C3-d. **This commit is not behaviour-neutral.** §5.2's three divergences land here. §12.2 cases 9, 21 and 22 apply to this commit, not just commit 3. ### Commit 3 — D: mint at accept, not at invite ⟵ *the flip* **Both halves must ship together.** See §4.3. #### C3-a. `GroupController::invite()` — stop minting Replace **926-941**: ```php else { $season = $registration->get('season')->entity; $team = NULL; $max_group_size = $season->getMaxGroupSize(); // D-8 (option D): a season group_id is NOT minted here. It is minted by the // first ACCEPT, from the inviter's registration — see // GroupMembershipService::joinInviterGroup(). Before that there is no group, // only an inviter with outstanding invitations, and nothing to clean up if // this invitation is declined, ignored or deleted. // // $group_id may legitimately be NULL from here down. Everything below must // tolerate that. $group_id = $registration->get('group_id')->value ?: NULL; $current_size = $this->groupMembership->getGroupSize( (int) $season->id(), $group_id, (int) $this->currentUser->id() ); } ``` Then at **1113-1116**, stop writing `group_id` onto the invitation: ```php else { $invitation_data['season'] = $season->id(); // D-8: no group_id. A pending invitation carries none; it is written at // accept time. See GROUP_ID_AT_ACCEPT_BRIEF.md §5.3. } ``` **Check 1 needs no change — verified, including the loose-comparison question.** At **1025-1035**, `$existing_group_id == $group_id` is only reached inside `if ($existing_group_id)`, so the left side is always a non-empty string, and `"uuid" == NULL` is FALSE in PHP 8. A caller with no group correctly gets *"already in another group"* for an invitee who has one. **Optional in the same commit — the race-B guard (§5.5).** Refuse if the invitee already has a pending invitation out to the caller for this season. One query, and it removes the mutual-invitation state entirely. **§14 Q9.** **Housekeeping in the same commit:** delete the stray instruction comment at **965-975** — *"UPDATED CODE for GroupController.php - invite() method"*, referencing dead line numbers ~583-605. It sits inside `invite()` between the email check and the duplicate checks. **Do not touch the tournament branch (891-925).** There the `group_id` comes from the Team entity, the team is the real container, and the write at 912-915 is reconciliation with the team rather than group creation. #### C3-b. `GroupController::acceptSeasonInvitation()` — 1420-1504 ```php protected function acceptSeasonInvitation($invitation) { $season_id = $invitation->get('season')->target_id; $inviter_uid = $invitation->get('inviter')->target_id; … existing groups_locked gate (1424-1429) … … existing pickLiveRegistration for MY registration (1432-1448) … … existing "already in a group" check (1450-1456) … // D-8: the invitation no longer carries a group_id while pending. Resolve the // INVITER's live registration and derive the group from it. If the inviter has // since cancelled there is no group to join — decline rather than mint a group // onto a dead row. $inviter_registration = $this->pickLiveRegistration( $this->entityTypeManager->getStorage('ccsoccer_registration') ->loadByProperties(['player' => $inviter_uid, 'season' => $season_id]) ); if (!$inviter_registration) { $this->messenger()->addError($this->t('The player who invited you is no longer registered for this season.')); $invitation->decline(); $invitation->save(); return $this->redirect('ccsoccer.group', ['registration' => $registration->id()]); } try { // Mints the group if needed AND writes group_id / invited_by / // invitation_status onto $registration — all under one lock. The caller // does NOT write those three fields; see §5.4 race C. $group_id = $this->groupMembership->joinInviterGroup($inviter_registration, $registration); } catch (GroupMintConflictException $e) { $this->messenger()->addError($this->t('You have already joined a group for this season.')); return $this->redirect('ccsoccer.group', ['registration' => $registration->id()]); } // Stamp the group onto the invitation now that it exists (§5.3) — four // existing queries key accepted invitations by group_id. $invitation->set('group_id', $group_id); $invitation->accept(); $invitation->save(); … existing notify-inviter (1469-1482) and decline-rivals (1486-1500), unchanged … } ``` **The three `$registration->set(...)` lines at 1458-1462 are deleted, not moved** — `joinInviterGroup()` does that write. Leaving them in place is harmless but re-saves the entity for nothing; leaving them in place *and* removing the service's write reopens race C. Note the switch from the inline `set('status','accepted') + set('responded_at', …)` pair to `Invitation::accept()`. Same effect, one definition. Do the same in 3c and 3e. **The inviter-cancelled case is new and real.** Today the group_id is on the invitation, so accepting after the inviter cancels puts the acceptor in a group of one that used to be someone else's. Under D it has to be handled explicitly, and declining is the honest outcome. **§14 Q3.** #### C3-c. `RegistrationController::acceptSeasonInvitationDirectly()` — 474-530 Identical treatment. **This controller already uses `LiveRegistrationTrait`** (`:9`, `:19`) so `pickLiveRegistration()` is available. It reaches for `\Drupal::service('ccsoccer.notification')` at **495**, so a `\Drupal::service('ccsoccer.group_membership')` is stylistically consistent here. #### C3-d. `OrderCompleteSubscriber::createSeasonRegistration()` — invitation block 488-545 The delicate one. **This runs after payment has been captured. It must not throw.** **`OrderCompleteSubscriber` does NOT use `LiveRegistrationTrait`.** Add `use LiveRegistrationTrait;` to the class, or the `pickLiveRegistration()` call below is a fatal in the post-payment path — which is the one place this brief says must not fatal. ```php if ($selected_invitation_id) { $invitation = …load($selected_invitation_id); if ($invitation && $invitation->isPending()) { // D-8: derive the group from the inviter instead of the invitation. $inviter_uid = $invitation->get('inviter')->target_id; $inviter_registration = $this->pickLiveRegistration( $this->entityTypeManager->getStorage('ccsoccer_registration') ->loadByProperties(['player' => $inviter_uid, 'season' => $season->id()]) ); if (!$inviter_registration) { $this->logger->warning('Order @oid: inviter @uid has no live registration for season @sid; registration created ungrouped.', […]); } else { $candidate_group_id = $inviter_registration->get('group_id')->value; // Count CONFIRMED members only … (existing comment block 498-507 stays; // its reasoning is unchanged). With no group yet the confirmed count is // 1 — the inviter alone — which is what the guard should see. $confirmed_count = $candidate_group_id ? $this->groupMembership->getLiveMemberCount((int) $season->id(), $candidate_group_id) : 1; if ($confirmed_count >= $season->getMaxGroupSize()) { … existing flag-the-order branch 518-533 … } else { // ORDERING PROBLEM — see §14 Q10. joinInviterGroup() locks and writes // the ACCEPTOR's registration, and on this path that row does not // exist yet: it is created below at :548. The registration must be // created FIRST (ungrouped), then joined. Restructure this method so // the create() at :548-561 runs before this block, with // 'group_id' => NULL, 'invited_by' => NULL, // 'invitation_status' => 'none', and the join applied afterwards. // Do NOT pass NULL for the acceptor — that reopens races B and C on // the one path where they cost money. try { $group_id = $this->groupMembership->joinInviterGroup($inviter_registration, $registration); $invitation->set('group_id', $group_id); $invitation->accept(); $invitation->save(); } catch (\Throwable $e) { // Payment is already captured. Never let this abort the order. // Log FIRST and unconditionally — watchdog is durable; the order // flag below is written inside Commerce's open order transaction // and is lost if that transaction later rolls back. $this->logger->error('Order @oid: group join failed (@msg); registration left ungrouped.', […]); $order->setData('ccsoccer_group_mint_failed', [ 'invitation_id' => (int) $invitation->id(), 'inviter_uid' => (int) $inviter_uid, 'season_id' => (int) $season->id(), 'detected_at' => \Drupal::time()->getRequestTime(), ]); $order->save(); } } } } } ``` **Five things to get right here:** 1. **The method must be restructured so the registration is created before the group is applied.** `joinInviterGroup()` locks and writes the acceptor's row, and on this path that row does not exist until **548**. Create it ungrouped (`group_id` NULL, `invited_by` NULL, `invitation_status` `'none'` — which is already what those three locals default to at **481-483**), then join. **This is the largest single edit in commit 3 and the one most likely to be botched.** §14 Q10 asks Caleb to sanity-check the restructure before it is written. 2. **`OrderCompleteSubscriber` needs `ccsoccer.group_membership` added to its `services.yml` definition** as well as `use LiveRegistrationTrait;` on the class. Neither exists today. 3. **The `ccsoccer_group_capacity_exceeded` flag records `'group_id' => $candidate_group_id`** (**526**), which can now be NULL. **Add `'inviter_uid'`, keep `group_id`** — an admin following the flag needs something to look up. Do not silently write NULL into an audit record. 4. **Catch `\Throwable`, and log before flagging.** A lock wait, a deadlock, a conflict — all of it must degrade to "registration created ungrouped, order flagged", never to an exception escaping the subscriber. And `$order->save()` runs inside Commerce's open order transaction: if the transition later rolls back, the flag goes with it. **Watchdog is the durable record; the order flag is the convenient one.** Write both, in that order. 5. **Re-read the existing comment at 498-507** before touching the guard. It explains why pending invitations are deliberately not counted here, and that reasoning survives unchanged. #### C3-e. `GroupInvitationsForm::acceptInvitation()` — season branch, 1308-1345 Receives `$group_id` as a route parameter, so on this path the group **already exists**. Add the stamp alongside the registration write at **1325-1328**: ```php $registration->set('group_id', $group_id); … $invitation->set('group_id', $group_id); // §5.3 ``` **An earlier draft justified "unchanged" by claiming the Roster Builder only renders a group container for two or more members. That is false** — `RosterBuilderForm::renderPlayersGrouped()` **285-292** buckets on `!empty($player['group_id'])` with no count threshold, and **295-297** renders a container per bucket. A group of one *does* render. The correct statement is §11.1's: after D, a player with only *pending* invitations has no `group_id` at all and therefore no container. ### Commit 4 — E (§8) · Commit 5 — CF11 (§9) --- ## 6.3 — `myRegistrations()` season card, 139-183 The whole `$group_status` block is gated on `if ($group_id)` at **144**. Under D a manager with outstanding invitations and no accepted member has no `group_id`, so **the badge disappears entirely** — including the *"N invitations pending"* badge the template renders at `ccsoccer-my-registrations.html.twig:92-95`. A manager would open My Registrations and see no sign that they have invited anyone. Re-gate on *manager-ness plus outstanding invitations*, not on `group_id`: ```php $group_id = $registration->get('group_id')->value; $invited_by_id = $registration->get('invited_by')->target_id; $is_group_manager = empty($invited_by_id); $group_status = NULL; if ($is_group_manager) { $pending_count = count($this->groupMembership->getPendingInvitations( (int) $season->id(), $group_id, (int) $registration->get('player')->target_id )); $accepted_count = $group_id ? max(0, $this->groupMembership->getLiveMemberCount((int) $season->id(), $group_id) - 1) : 0; if ($pending_count || $accepted_count) { $group_status = ['role' => 'manager', 'accepted_count' => $accepted_count, 'pending_count' => $pending_count]; } } elseif ($group_id) { … existing member branch, unchanged (169-182) … } ``` The `if ($pending_count || $accepted_count)` wrapper keeps the badge off cards for players who have done nothing — matching today, where no `group_id` meant no badge. **No template change here**: the twig at **90-110** already handles all three count combinations. ## 6.4 — `manage()`, the whole season display block **Six coupled lines, not the one an earlier draft listed.** Under D, a manager with no group hits: | Line | Today | Under D, unfixed | |---|---|---| | **475** | `elseif ($is_season && $group_id)` builds `$group_roster` | roster empty | | **501** | `if ($is_manager && … (bool) $group_id)` gates sent invitations | **short-circuits — the whole Pending Invitations table vanishes**, no Delete, no Send Reminder | | **577** | `$accepted_count = count($group_roster)` | 0 | | **589** | `elseif ($group_id)` gates `$pending_count` | 0 | | **598** | `$spots_left = $max - $accepted - $pending` | 6 | | **602** | `if (!$group_id)` gates invitations *to* me | opens up — **this is the D-9 fix, and it is free** | The template then renders **"Accepted: 0 · Pending: 0 · Spots left: 6"** (twig **19-22**) with no roster table (twig **50**) — which is **verbatim the tournament bug already documented in the comment at `GroupController.php:422-437`** (*"saw an empty roster, 'Accepted: 0', and an inflated 'Spots left'"*), reintroduced on the season side. `spots_left` also gates the invite form at twig **107**. **Changes:** - **475** — add an `|| ($is_season && !$group_id)` arm that builds a one-row roster from the viewer's own registration, so a solo inviter sees themselves with the Manager badge. This is truthful (they *are* the manager of an as-yet-unformed group) and it makes the table render. - **501** — the real line is `if ($is_manager && ($is_tournament ? (bool) $team : (bool) $group_id)) {`. Rewrite it whole as `if ($is_manager && ($is_season || (bool) $team)) {`. The season query at 510-515 is already correctly scoped by `inviter` + `season`; **dropping `group_id` from it alone fixes nothing while 501 short-circuits.** Both must change. (Tournament behaviour is unchanged: `$team` is the gate on that arm, and `$is_season` is FALSE there.) - **510-515** — drop `'group_id' => $group_id`. - **589-596** — `$pending_count` via `getPendingInvitations()`. - **602** — widen to *"no group, or manager of a group of one"* (§9's D-9 note). **Do this in commit 2, not commit 5** — §9 describes the rule, this table schedules it. - **577, 598** — no change; they become correct once 475 and 589 do. - **`can_disband`** — compute it here (§9) and add it to the render array at 623-654. It also needs registering in `ccsoccer.module`'s `hook_theme()` (**80-102**), or Twig sees an undefined variable and the button silently never renders. ## 6.5 — `myRegistrations()` invitation card, 243-286 ⚠ *needs a template change* The card shows `group_members`. Under D a first invitation has none, so the block at twig **32-46** is skipped and the card says only *"Alice invited you"*. **Two corrections to an earlier draft of this brief:** 1. **`pending_count` (built at 284) is never read by any template.** `grep pending_count templates/` finds only `item.group_status.pending_count`, which is the *season registration* card, not the invitation card. It is dead data. **Delete it** rather than re-keying it. 2. **The label is hard-coded `Already in group:`** at `templates/ccsoccer-my-registrations.html.twig:35`. Populating `group_members` with the lone inviter would render *"**Already in group:** Alice (Manager)"* — a false statement about a group that does not exist. That is the failure mode this section was supposed to avoid. **So:** keep `group_members` as *"who is already in the group"* — genuinely empty for a first invitation — and add the inviter's name separately. ```php $group_id = $invitation->get('group_id')->value; // NULL while pending // The twig reads member.player (a user entity) and member.is_manager, and // is_manager comes from each member's own invited_by — it cannot be derived // from a uid list. Use getMemberRegistrations(), not getMemberUids(). $group_members = []; foreach ($this->groupMembership->getMemberRegistrations((int) $season->id(), $group_id) as $reg) { $group_members[] = [ 'player' => $reg->get('player')->entity, 'is_manager' => empty($reg->get('invited_by')->target_id), ]; } // getMemberRegistrations() returns [] for a NULL/'' group_id, so a first // invitation correctly produces an empty list — which is what makes the // template's else-branch below fire. ``` This also **adds the D-4 status filter** that the current loop at **255-267** lacks: today a cancelled member still shows on the invitation card as someone "already in the group". Template (`ccsoccer-my-registrations.html.twig`), replace the block at **32-46** so the label matches the content: ```twig {% if item.group_members is not empty %}
{% if item.type == 'tournament' %}Already on team:{% else %}Already in group:{% endif %} … {% elseif item.type == 'season' and item.inviter %}
{{ 'Starting a group with:'|t }} {{ item.inviter.field_first_name.value }} {{ item.inviter.field_last_name.value }}
{% endif %} ``` `item.inviter` is already passed (**281**). **Archive the template before editing** — same repo rule as PHP. ## 6.6 — `GroupPane::submitPaneForm()`, 238-247 Remove the `'group_id'` key from the stored selection. Nothing reads it; under D it would always be NULL, and a NULL in order data invites a future reader to believe it means "no group". ```php if ($invitation) { $selections[$selection_key] = [ 'invitation_id' => $invitation_id, // group_id deliberately not cached: pending invitations carry none // (D-8). OrderCompleteSubscriber derives it from the inviter at // completion time, which is the only moment it is knowable. ]; } ``` `OrderCompleteSubscriber` reads only `['invitation_id']` (**486**), so orders created before the deploy are unaffected — **re-confirm on TEST with a cart that predates the deploy** rather than trusting the read. ## 6.7 — `GroupDissolveService::dissolveGroup()` — the pending sweep Its `accepted` half still works (§5.3). Its `pending` half does not — under D there are no pending invitations carrying the group_id. Insert a pending sweep **before** the registrations are cleared, because clearing them destroys the membership that identifies which invitations to decline: ```php // D-8: pending invitations no longer carry a group_id, so they must be found // via their inviter while the group still has members. Do this BEFORE the // clear loop below — clearing group_id destroys the very membership this // lookup needs. $pending = $this->groupMembership->getPendingInvitations($season_id, $group_id); foreach ($pending as $inv) { $inv->decline(); $inv->save(); $results['declined']++; } ``` `dissolveGroup()` takes only `(string $group_id, array $options)` and does not know the season. Derive it from the first registration in the group rather than adding a required option — D-12's reasoning ("every caller would pass the same value, which is the signal it should not be a parameter") applies. **§14 Q7.** **This would be a behaviour change to CF8's service.** Flag it in the commit message if D is ever built. **[Aug 5] The service has since been LOCAL-tested (plan §10.8) and extended by the D-8 package** with `clearGroupIfOrphaned()` and `clearInviterGroupIfOrphaned()`, so it is no longer unexecuted — but it now has fourteen callers rather than one, which raises rather than lowers the bar for changing it. ## 6.8 — `TeamBalancerService::getRosterState()` Builds `group_id => pending count`. Under D the `if ($group_id)` guard at **1086** makes every pending invitation invisible, so every group renders `status: 'complete'` at **1350-1362** — green rather than yellow. The same lookup also feeds the Drush roster report (`CcsoccerCommands.php:1789`). Re-key through the inviter. **The block must move below line 1119** — `$group_id_by_uid` can only be built from the loop at **1098-1119**, which currently runs *after* `$pending_by_group` at **1083-1089**: ```php // D-8: a pending invitation carries no group_id. Attribute it to the group // its INVITER is in; an invitation from someone not yet in a group has no // group to belong to and is correctly ignored here — the Roster Builder only // draws groups that exist. $pending_by_group = []; foreach ($pending_invitations as $invitation) { $inviter_group = $group_id_by_uid[$invitation->get('inviter')->target_id] ?? NULL; if ($inviter_group) { $pending_by_group[$inviter_group] = ($pending_by_group[$inviter_group] ?? 0) + 1; } } ``` Build `$group_id_by_uid` in the same pass that builds `$player_data` (**1098-1119**) rather than issuing new queries. **See §11.1** for the visible consequence. ## 6.9 — `CancelRegistrationForm::submitForm()`, 386-408 *(added after review)* **A group-lifecycle path an earlier draft missed entirely.** Today: ```php $group_id = $registration->get('group_id')->value; … if ($group_id !== '') { // :389 … clear the row, then dissolveGroup() … } ``` Under D, **a manager with pending invitations but no `group_id` cancels their registration and their outstanding invitations are never declined.** Today they are, via `GroupDissolveService`'s pending branch. Those invitations then sit `pending` forever, pointing at a cancelled inviter — which is audit class **IA**, the thing this whole document exists to stop creating. Add an else-branch: ```php else { // D-8: no group_id does not mean no invitations. A player who invited others // but had nobody accept yet still has pending invitations that must be // declined on cancellation — otherwise they sit pending against a cancelled // inviter forever (audit class IA). $uid = (int) $registration->get('player')->target_id; foreach ($this->groupMembership->getPendingInvitations((int) $season_id, NULL, $uid) as $inv) { $inv->decline(); $inv->save(); } } ``` `$season_id` is already in scope (**335**); `$uid` is not — the method has `$player` (**350**) but no uid local. The method returns early at **353-357** when `$season` is NULL, so the tournament path never reaches this branch. **Note for §8:** an earlier draft listed "four near-identical copies of clear-three-fields". Two of those entries were wrong. `GroupInvitationsForm::dissolveGroupConfirmed()` no longer contains a copy — CF8 already made it delegate to the service at **`GroupInvitationsForm.php:884`**. And `GroupInvitationsForm::submitForm()`'s per-row decline (**1037-1040**) sets only `group_id` and `invited_by`, not `invitation_status`. **The genuine remaining copies are `leaveGroup()` 1983-1986, `removeMember()` 1830-1833, and `CancelRegistrationForm.php:393-395`.** --- ## 7. What D does *not* fix Stated explicitly so the next session does not go looking: - **Route 4.** A real group that empties out still leaves the manager holding a `group_id`. That is what §8 and §9 are for. - **Existing stranded rows.** No data repair this cycle (plan §7). Every solo `group_id` on PROD today survives the deploy and is cleared by the existing admin Dissolve, or by the manager themselves once §9 ships. §12.4 has read-only SQL to count them. - **Cancelled members occupying group slots** — CF3's remaining nine sites. §5.2 fixes only site 1 (`getGroupSize()`), because it is being rewritten anyway. - **Accept-time capacity** — §5.6, deliberately deferred. - **`groups_locked` on the admin accept path** — §5.7 fixes the two player-facing gaps and leaves `GroupInvitationsForm::acceptInvitation()` alone. An admin overriding the lock is arguably the point of being an admin. - **The tournament flow.** Untouched, entirely. --- ## 8. E — deleting the last pending invitation clears a now-empty group **Files:** `src/Controller/GroupController.php` (`deleteInvitation()`) **Today** (`deleteInvitation()` **1334-1384**) the method verifies the caller is the inviter, refuses anything not `pending`, hard-deletes, and redirects. It never touches `group_id` — verified, the string does not appear in the method. **Change.** After the delete, if the caller is the **manager of a season group** that now has exactly one live member (them) and **no remaining pending invitations**, clear the group. ```php // Capture BEFORE the delete — after it, the invitation is gone. $is_team_invite = $invitation->isTeamInvite(); $season_id = $invitation->get('season')->target_id; … $invitation->delete(); // D-8 (option E): if that was the last thing holding this group together, // clear it. Only reachable via route 4 — under option D a group cannot exist // with fewer than two members unless one has since left. See §10. if (!$is_team_invite && $season_id) { $my_reg = $this->pickLiveRegistration( $this->entityTypeManager->getStorage('ccsoccer_registration') ->loadByProperties(['player' => $this->currentUser->id(), 'season' => $season_id]) ); $my_group = $my_reg ? $my_reg->get('group_id')->value : NULL; if ($my_reg && $my_group && empty($my_reg->get('invited_by')->target_id) // manager && $this->groupMembership->getLiveMemberCount((int) $season_id, $my_group) === 1 // alone && $this->groupMembership->getPendingInvitations((int) $season_id, $my_group) === [] // nothing outstanding ) { $this->groupDissolve->dissolveGroup($my_group, [ 'notify' => FALSE, // nobody left to notify 'reason' => 'last pending invitation deleted (D-8/E)', ]); $this->messenger()->addStatus($this->t('You are no longer in a group, so you can join another player’s group or start a new one.')); return $this->redirect('ccsoccer.my_registrations'); } } ``` **Five points:** 1. **All four conditions are load-bearing.** Drop the "no remaining pending invitations" clause and a manager who sent two invites and deletes one has their group dissolved out from under the other — precisely the objection that kept option B off the table. §12.3 case 27 tests exactly this. 2. **`notify => FALSE`.** The group has one live member and they are the person who clicked. Passing `TRUE` would email the clicker to say their own group was dissolved. 3. **Reuse `GroupDissolveService`; do not clear the three fields inline.** Three near-identical copies already exist (`leaveGroup()` 1983-1986, `removeMember()` 1830-1833, `CancelRegistrationForm.php:393-395`) and the drift between them is what `ROSTER_RECONCILIATION_PLAN.md` is largely about. Do not write a fourth. **`GroupController` does not have `$this->groupDissolve` today** — commit 2 injects it (§6). 4. **`$season_id` must be read before `$invitation->delete()`.** The existing code already does this (**1348**) for its redirect — keep that ordering. 5. **Suppress or reword the existing status message.** `deleteInvitation()` already calls `addStatus('Invitation deleted.')` at **1354**, before this block. Left as-is the player sees two messages. Either move that call into an else-branch, or keep it and make the new one additive — the snippet above takes the second option. `dissolveGroup()` already logs. No extra watchdog needed. --- ## 9. CF11 — a solo manager can disband their own group **Files:** `GroupController.php` (new `disbandGroup()`), `ccsoccer.routing.yml`, `templates/ccsoccer-group-manage.html.twig`, **`ccsoccer.module`** (`hook_theme()`, **80-102** — the `ccsoccer_group_manage` variables are declared explicitly, so `can_disband` must be added there or Twig sees an undefined variable and the button **silently never renders, with no error**). `GroupController` also needs `ccsoccer.group_dissolve` injected — commit 2 does it (§6). **Superseded from the plan's CF11 brief:** every line number in it is stale (CF4 edited both files it cites) and its route table said CF10 closes route 1 — under this brief D closes routes 1, 2 **and** 3, so CF11's job shrinks to route 4 alone. **Use this section, not plan §10.3 CF11.** The scope decision (**D-7 — solo only, not a general manager-dissolve**) still stands and is not reopened. ### The method `disbandGroup($registration)` — guards in this order: 1. registration loads and `player` is the current user (copy `leaveGroup()` 1950-1954) 2. `registration_type === 'season'` — hard refuse tournaments 3. status is live — `!isDeadRegistrationStatus()` (D-4/D-13) 4. `group_id` non-empty, else *"You are not in a group."* 5. caller is the manager (`invited_by` empty), else point them at Leave Group 6. `groups_locked` on the season is FALSE — matches `leaveGroup()` 1975-1978 and `invite()` 882-889 7. **live-member recount inside the request** — see the race note below Then `$this->groupDissolve->dissolveGroup($group_id, ['notify' => FALSE, 'reason' => 'solo manager disband (CF11)'])`, redirect to `/my-registrations` with *"Your group has been disbanded. You can now join another player's group or start a new one."* **`notify => FALSE`, not `TRUE`.** The plan's CF11 brief says `notify => TRUE`; that was written when CF11 was a general dissolve. Under D-7 (solo only) there is nobody but the clicker in the group, so `TRUE` would email them about their own click. **This is a correction to the plan.** **Guard 5 has a corner case created by races B and C (§5.4).** Two players who accepted each other simultaneously both carry `invited_by`, so neither passes guard 5 and neither can Disband; and race C leaves an abandoned inviter alone in a group *with* an empty `invited_by`, who can. If those races are left unclosed, guard 5 needs an escape hatch — *"or this group has no member with an empty `invited_by`"* — which is ugly. **Closing them properly is what avoids needing it.** §12.4 query E detects the state either way. **Do not touch `Registration.team`** — CF6's recorded decision. Disbanding a friend group must not un-assign anyone from a season team. `GroupDissolveService` already leaves `team` alone (**156**). ### The race that must be handled The button renders from a page that may be minutes stale. If an invitee accepts between render and click, a naive dissolve silently ejects them. **Re-count live members inside the request and refuse if > 1:** > *"Someone joined your group since this page loaded. Refresh to see your group."* Count via `GroupMembershipService::getLiveMemberCount()` — which is exactly why §5.1 puts that method on the service rather than inlining it here. **Skipping this recount is the one thing that would make CF11 dangerous.** ### Route ```yaml ccsoccer.group.disband: path: '/my-group/{registration}/disband' defaults: _controller: '\Drupal\ccsoccer\Controller\GroupController::disbandGroup' requirements: _user_is_logged_in: 'TRUE' _csrf_token: 'TRUE' methods: [POST] ``` Both requirements matter — copy the neighbouring `ccsoccer.group.leave` entry exactly. ### Template `ccsoccer-group-manage.html.twig` renders the roster table only when `group_roster is not empty` (**50**). The Actions cell (**77-91**) has two arms today: `is_manager and not member.is_self` → Remove; `not is_manager and member.is_self` → Leave Group. **Add a third arm** for `is_manager and member.is_self`, gated on a new `can_disband` flag: ```twig {% elseif is_manager and member.is_self and can_disband %}
``` **Compute `can_disband` in `manage()`, not in Twig** — manager + season + `group_id` non-empty + exactly one live member + not locked. Re-deriving the rule in a template is how it drifts from the controller's guard. **Interaction with §6.4.** §6.4 adds a one-row roster for a solo inviter *with no group_id*. That player must **not** see Disband (guard 4 would refuse the POST anyway, but the button should not render). `can_disband` requires a non-empty `group_id`, which is what separates the two cases. ### D-9 resolves itself for routes 1-3 Plan §7 **D-9** asks whether `manage()` should show invitations addressed to a solo manager, since the `if (!$group_id)` gate at **602** hides them. **Under D this fixes itself for routes 1-3** — those players no longer have a `group_id`, so the gate lets them through with no code change. It does **not** fix itself for route 4. Widen the gate to *"no group, or manager of a group of one"*. **§6.4 schedules that edit in commit 2**, with the rest of the `manage()` display block — this section states the rule, it does not schedule it. **It is a display gate, not a data one — check it against the Roster Builder screens before changing it**, per the plan's own caution. --- ## 10. What D changes about E and CF11 — read before building them **This section argues with the decision it is implementing. That is on purpose.** The D + E + CF11 recommendation in `SESSION_HANDOFF.md` was written while D-8 was still open, and it evaluated E and CF11 against the *pre-D* world. Once D lands, both are worth less than the handoff implies. **The reasoning is three lines:** 1. Under D, a `group_id` exists only after someone accepted → a group always starts with ≥ 2 members. 2. Therefore **deleting a pending invitation can never empty a group** — the group either does not exist, or has two or more members. 3. Therefore **E's trigger is only reachable after route 4 has already fired.** Manager's group drops to one, manager invites a replacement, manager deletes that invitation. Real, but narrow. E is still worth building — it costs one small block in a method already being read, it uses a service already built, and *"cancel the invite I sent"* is discoverable in a way *"Disband Group"* is not. But **it is not the second half of D. CF11 is.** ### The alternative that D makes newly attractive Option **B** — auto-clear when a group drops to one live member — was rejected because it had six call sites and could dissolve a group out from under someone waiting on a slow invitee. **Both objections weaken under D:** - **The six sites are now two.** `deleteInvitation()` becomes a no-op case; CF8's `GroupDissolveService` already owns the admin-decline and season-cancel paths. What is left is `leaveGroup()` (**1983-1986**) and `removeMember()` (**1830-1833**) — the two places a group can actually drop to one. - **The "slow invitee" objection is now expressible.** It needs an "and no pending invitations" carve-out, and §5.1's `getPendingInvitations()` is exactly that predicate, correctly implemented once. So a defensible alternative is **D + B(narrow) + CF11**, where B is two calls to a shared `clearGroupIfEmpty()` and CF11 remains the manual escape hatch. That closes route 4 *automatically* instead of asking a player to find a button labelled "Disband". **Recommendation: build D first and ship it. Then decide between E and B(narrow) with real data** — specifically, §12.4 query B, which counts how many of today's solo groups arrived via route 4 rather than routes 1-3. If route 4 is as rare as the handoff assumes, CF11 alone is enough. **§14 Q4.** --- ## 11. Regressions and admin-facing consequences Things that will look different after this ships, listed so nobody debugs them as bugs. ### 11.1 The Roster Builder stops showing phantom groups of one A player with a pending invitation and no accepted member currently renders as a **group container of one** in the season Roster Builder — `RosterBuilderForm::renderPlayersGrouped()` **285-292** buckets on `!empty($player['group_id'])` with **no count threshold**, and **295-297** renders a container per bucket. After D they render as an **individual player card**. **This is the intended outcome** — they are not a group. But it changes what the roster build screen looks like, and an admin mid-build will notice. Mention it before deploying, not after. (A *real* group of one — route 4 — still renders a container, because it still has a `group_id`.) ### 11.2 The admin Group Invitations page becomes unreachable for a pre-acceptance invitation ⚠ **This is the sharpest edge in the whole change, and it needs an explicit answer.** `/admin/ccsoccer/group/{group_id}/invitations` is keyed on `group_id`, and the only links to it are built in `RosterBuilderForm:332`, `TournamentRosterBuilderForm:532` and `:855` — all from a rendered group container. Under D, a player with only pending invitations has no `group_id`, so **no container, no link, no page.** The admin loses the ability to: - see that invitation on the admin side at all - decline it on the player's behalf - Dissolve (which is how they currently un-strand people) The third is the *workaround* for the very problem D removes, so losing it is mostly fine. The first two are a genuine reduction in admin capability. **Three ways to answer it, in preference order:** 1. **Accept it.** The invitation is visible and deletable to the player who sent it, on their own Manage Group page, which §6.4 keeps working. Admin intervention was only ever needed because the player was stuck, and they no longer are. *Cheapest, and probably correct.* 2. **Add an admin invitations view** keyed on season rather than group_id — a small list builder over `ccsoccer_invitation` filtered to `status = pending`. Arguably worth having regardless of D; there is no admin-side list of pending invitations anywhere today. 3. **Key the admin route on the inviter's registration id** as well as group_id. More route surface, least value. **§14 Q6.** Do not ship D without deciding this. ### 11.3 A pending invitation's group becomes mutable *(added after review)* **An under-appreciated consequence of the derivation rule.** Today `Invitation.group_id` freezes the target at invite time. Under D, an outstanding invitation means *"join whatever group my inviter is in when you accept"* — and the inviter's group can change while the invitation sits pending. Two concrete cases, neither covered by any commit above: - **Reparent forward.** P has a pending invitation out. An admin drags P into group X (`RosterBuilderController.php:408-410`). P's invitation now silently means *"join group X"* — a group the invitee never saw and never agreed to, whose cap it now consumes via `inviter IN members(X)`, and which `mergeToGroup()`'s own capacity warning (**385-390**) still cannot see. - **Reparent to nothing.** A member with an outstanding invitation leaves their group (`leaveGroup()` **1983-1986**). Their invitation now means *"start a new group with me"*. Accepting it mints a fresh group. Arguably correct, definitely different. **This is inherent to option D**, not a bug in the implementation — the field's meaning genuinely moved from *"this group"* to *"my inviter's group"*. Three ways to handle it: 1. **Accept and document.** The invitee sees the inviter's name and the current group membership on the card (§6.5), so the information is on screen at accept time even if it changed since the invite. 2. **Decline outstanding invitations on merge.** In `mergeToGroup()`, decline invitations sent by the player being merged. Defensible — they did not choose this group either. 3. **Freeze at invite by keeping a nullable `intended_group_id`.** Reintroduces the state D removes. Not recommended. **§14 Q11.** Option 1 is probably right for a ~200-player casual league, but it should be a decision, not an oversight. ### 11.4 `Invitation.group_id` becomes NULL for every future pending row Anyone reading the `ccsoccer_invitation` table directly, or the entity's view display (the field is `setDisplayConfigurable('view')`), will see blanks where they used to see UUIDs. Not a bug. Worth a one-line note in `ARCHITECTURE_DECISIONS.md`. ### 11.5 Mixed data during the transition Invitations created **before** the deploy keep their `group_id`. Invitations created after do not. Every query in §6 is written to tolerate both — `inviter IN members` finds an old pending invitation just as well as a new one, because `inviter` is set on both. **No update hook, no backfill, no downtime step.** This is the main reason to re-key on `inviter` rather than adding a NULL check everywhere. ### 11.6 A pre-deploy invite whose group_id was already minted A player who clicked Invite on Aug 4 and whose invitee accepts on Aug 8 has a `group_id` on their registration (old mint) and an invitation carrying it. `joinInviterGroup()` reads the inviter's existing `group_id` and returns it — **the old group is reused, not replaced.** Correct, and worth an explicit LOCAL test (§12.2 case 13). --- ## 12. Test plan **`ddev drush cr` before any of this** — commits 1 and 2 change the container. ### 12.1 Pre-flight - [ ] `ddev php -l` on every changed file - [ ] `ddev drush cr` - [ ] `ROSTER_RECONCILIATION_PLAN.md` §10.8 passes **first** — this brief builds on CF8 and CF4, neither of which has ever executed - [ ] Registration **5088** cleaned up (it still carries bad state from the Aug 4 reproduction) - [ ] `drush config:status` clean before starting - [ ] §12.4 queries A and B run **before** any code lands, so the before/after comparison is possible ### 12.2 LOCAL click-throughs — D The first three are the whole point of the change. Check `Registration.group_id` in the database after each, not just the screen. 1. **Bad email.** Registered player with no group → invite a garbage address → error as today, **and their `group_id` is still NULL.** 2. **Invitee already in another group.** → *"already in another group for this season"*, **inviter's `group_id` still NULL.** 3. **Empty search box.** → *"Please select a player or enter an email"*, **`group_id` still NULL.** 4. **Happy path.** Valid invite → invitation created with **`group_id` NULL**; inviter's `group_id` **still NULL**; invitee's My Registrations card says *"Starting a group with: Alice"* — **not "Already in group"** (§6.5). 5. **Manage Group as a solo inviter.** Inviter opens Manage Group → **one-row roster with the Manager badge**, "Accepted: 1 · Pending: 1 · Spots left: 4", the **Pending Invitations table renders with working Delete and Send Reminder buttons**, and **no Disband button** (§6.4, §9). *This is the case an earlier draft of this brief got wrong; test it deliberately.* 6. **My Registrations as a solo inviter.** The season card shows *"1 invitation pending"* (§6.3). 7. **Accept.** Invitee accepts from My Registrations → group of 2; **both** registrations carry the same new `group_id`; the invitation now carries it too; manager badge on the inviter's row. 8. **Second invite reuses the group.** Manager invites a third → no new `group_id`; size reads 3. 9. **Cap.** Season max 6. Send invites until refused. **Must refuse on the 6th** (self + 5 pending). All five accept → group of exactly 6. A 7th invite → refused. *The §5.2 assertion.* 10. **Decline.** Invitee declines → inviter's `group_id` still NULL. 11. **Delete.** Inviter deletes their only pending invitation → `group_id` still NULL, no dissolve fired. 12. **Rival invites.** A and C both invite B; B accepts A → C's invitation auto-declined (existing), **and C's `group_id` is NULL** — the route-2 fix, visible. 13. **Legacy invitation (§11.6).** Hand-set an invitation's `group_id` and the inviter's `group_id` to a shared UUID, then accept → **the existing group is reused**, no second UUID minted. 14. **Inviter cancelled.** Invite → cancel the inviter's registration → accept → declined with *"the player who invited you is no longer registered"*, acceptor ungrouped, no crash. 15. **Inviter cancels with invitations outstanding (§6.9).** Manager with no group and two pending invitations cancels → **both invitations are `declined`.** *This is the case an earlier draft missed entirely.* 16. **Checkout path.** Invitee not yet registered → magic link → registers and pays with the invitation selected in `GroupPane` → `OrderCompleteSubscriber` mints the group. Registration carries `group_id` and `invited_by`; invitation is `accepted` with the same `group_id`. 17. **Checkout, group full.** Fill the group, then complete a checkout with a stale pending invitation → order flagged `ccsoccer_group_capacity_exceeded`, registration created ungrouped, **flag payload contains `inviter_uid`** (§C3-d point 3). 18. **Register-page accept.** The *other* Accept button — `RegistrationController` path, for an already-registered invitee arriving via token. Same assertions as 7. **Both buttons must be tested; they are different controllers and both were broken by the Aug 4 bug.** 19. **Admin accept.** Roster Builder → group container → Group Invitations → Accept, season branch. Registration **and invitation** both carry `group_id`. 20. **Admin Dissolve.** Group of 3 with one pending invitation out → Dissolve → all three registrations cleared **and the pending invitation is `declined`** (§6.7 — silently breaks if the sweep is missed). 21. **Roster Builder display.** A player with a pending invitation and no group renders as an individual card (§11.1). A real group of 2 with one pending renders as a group **with the pending badge** (§6.8) — check the colour, `status` is `pending` not `complete`. 22. **Tournament regression.** Captain invites → accept → remove. `Team.group_id` still authoritative. **Nothing in the tournament flow may change.** 23. **`groups_locked` ON — all four accept paths (§5.7).** Invite refused, as today. Then with the lock on, attempt to accept a pending invitation from **(a)** My Registrations → refused today and after; **(b)** the Register-page / magic-link path → **today this succeeds and creates a group after the lock; after commit 3 it must be refused**; **(c)** checkout with the invitation selected → the registration is still created, **ungrouped**, and the order is flagged. Then confirm the season Roster Builder shows the **same set of groups** before and after the attempts. *This case is the one that protects the roster build; do not skip the (b) and (c) halves because (a) passed.* **Concurrency (two browsers, and worth the setup — §5.4). Run these on MySQL/MariaDB, never SQLite: `forUpdate()` is a no-op on SQLite, so the tests would pass and prove nothing.** 24. **Race A — two acceptors, one inviter.** Manager invites two players. Both accept as close to simultaneously as you can manage. → **exactly one `group_id` exists**, both acceptors and the manager are in it. Repeat five times. 25. **Race B — mutual invitations.** A invites B and B invites A (under D both invitations are created — verify that first, it is itself a change). Both accept simultaneously. → **not two manager-less groups.** Either one group forms and the other accept is refused with *"already joined a group"*, or — if §5.5's guard shipped — the second invitation was never created. Then run §12.4 query E: it must return zero rows. 26. **Race C — one acceptor, two inviters.** A and C both invite B. B accepts **both** invitations simultaneously from two tabs. → one accept succeeds, the other is refused with *"already joined a group"*, and **neither A nor C is left alone in a group of one.** Check both inviters' registrations, not just B's. *This is the race the first fix created; it is the least intuitive of the three and the one most worth actually running.* ### 12.3 LOCAL click-throughs — E and CF11 27. **E's trigger.** Group of 2 → manager removes the member (group of one, `group_id` still set) → manager invites a replacement → manager deletes that invitation → **group cleared**, message shown, manager can now be invited into someone else's group. 28. **E's carve-out.** Same setup but **two** pending invitations; delete one → **group NOT cleared**, the other still pending. *This is the test that stops E becoming option B by accident.* 29. **E does not fire for a member.** Confirm no dissolve when a non-manager member deletes an invitation they sent (§4.4 — members can send invitations). 30. **CF11's obsolete acceptance test.** P1 invites P2; P3 invites P2; P2 accepts P1's. Under D **P3 never got a group**, so P1 can invite P3 immediately — *this used to require Disband.* Verify it, and verify Disband is **not** offered to P3. 31. **CF11's real case.** Group of 2 → manager removes the member → Disband appears → click → group cleared, redirect, manager can now accept another invitation. 32. **CF11 guards, one each:** manager of a group of 2 → no button; non-manager → Leave Group only; solo inviter with no `group_id` → **no button** (§9); tournament captain → no button, hand-POST refused; `groups_locked` → button gone, hand-POST refused; cancelled registration → refused; another player's registration id in the URL → refused; POST without CSRF → 403. 33. **CF11 race.** Open Manage Group as the solo manager; in another browser have an invitee accept; click Disband → refused with the refresh message, group intact, invitee still in it. 34. **D-9.** After disbanding, a waiting invitation is visible on the same page without a reload. 35. **`Registration.team` unchanged** across every one of 27-34. Check before and after. ### 12.4 Read-only SQL **Queries A and B are meaningful only BEFORE the deploy** — B counts `ccsoccer_invitation.group_id`, which post-D pending rows no longer carry. **C, D and E are the after-invariants.** **Confirm the actual column names first** — `drush sqlq "DESCRIBE ccsoccer_registration"` and `"DESCRIBE ccsoccer_invitation"`. This codebase has been bitten by assumed names. ```sql -- A. How many solo season groups exist right now? (the backlog D does not repair) SELECT r.group_id, COUNT(*) AS live_members FROM ccsoccer_registration r WHERE r.registration_type = 'season' AND r.group_id IS NOT NULL AND r.group_id <> '' AND r.status NOT IN ('cancelled','expired') GROUP BY r.group_id HAVING live_members = 1; -- B. Of those, how many ever had an invitation associated? -- invitations_ever = 0 → routes 1-3, which D prevents outright. -- invitations_ever > 0 → route 4, which only CF11/E help. -- THIS QUERY DECIDES §10's open question. SELECT g.group_id, (SELECT COUNT(*) FROM ccsoccer_invitation i WHERE i.group_id = g.group_id) AS invitations_ever FROM ( SELECT r.group_id FROM ccsoccer_registration r WHERE r.registration_type = 'season' AND r.group_id IS NOT NULL AND r.group_id <> '' AND r.status NOT IN ('cancelled','expired') GROUP BY r.group_id HAVING COUNT(*) = 1 ) g; -- C. INVARIANT — 0 rows after the deploy. A pending invitation must carry no group_id. SELECT id, inviter, season, group_id, status FROM ccsoccer_invitation WHERE status = 'pending' AND group_id IS NOT NULL AND group_id <> '' AND created > UNIX_TIMESTAMP(''); -- D. INVARIANT — an accepted season invitation must carry a group_id. -- A non-zero result means one of the four accept paths is missing its stamp (§5.3). SELECT id, inviter, invitee_email, season FROM ccsoccer_invitation WHERE status = 'accepted' AND season IS NOT NULL AND (group_id IS NULL OR group_id = '') AND created > UNIX_TIMESTAMP(''); -- E. INVARIANT — no season group may have zero LIVE members with an empty -- invited_by, i.e. no group may be manager-less. Races B and C (§5.4) are two -- causes; a manager row cancelled by a path that bypasses the dissolve is -- another. Any non-zero result needs a look either way. -- NOTE: invited_by is an entity_reference with no default (Registration.php:161-164) -- and all 25 write sites set a real uid or NULL, never 0 — so IS NULL and -- empty(...->target_id) agree here. SELECT r.group_id FROM ccsoccer_registration r WHERE r.registration_type = 'season' AND r.group_id IS NOT NULL AND r.group_id <> '' AND r.status NOT IN ('cancelled','expired') GROUP BY r.group_id HAVING SUM(CASE WHEN r.invited_by IS NULL THEN 1 ELSE 0 END) = 0; ``` Query **E** is new and is worth adding to `ROSTER_DATA_AUDIT.sql` permanently — it detects a state that nothing in the current audit looks for. It is closely related to audit class **SE** ("group has zero manager rows, or more than one") but SE is a repair-time classifier; this is a live invariant. --- ## 13. Rollback — code is easy, data is not Commits 1 and 2 are safe to leave in place — they add a service and re-key queries. **Commit 3 is the flip, and it is atomic on purpose:** reverting it alone restores invite-time minting, because commit 2's `inviter IN members` queries return the correct rows whether or not the invitation also carries a `group_id`. **Do not revert only half of commit 3.** A build in which minting has stopped but the accept paths still read the invitation writes NULL group_ids onto acceptors (§4.3). Groups minted at accept time before a rollback are ordinary groups and survive it. ### ⚠ A code-only rollback is NOT safe. It needs a data step. **Every invitation sent while D was live has `group_id` NULL and may still be `pending`.** Revert the code and the restored accept paths read `$invitation->get('group_id')->value` → NULL → and write `group_id = NULL` onto the acceptor **with `invited_by` set and `invitation_status = 'accepted'`.** That is the orphan-acceptor state §4.3 warns about, manufactured by the rollback itself, on every invitation that was in flight. **So a rollback is two steps, in this order:** 1. **Revert the code.** 2. **Immediately backfill or decline the in-flight invitations.** Either is defensible; **backfill is kinder** because nobody loses an invitation they were waiting on. ```sql -- BACKFILL: give every pending season invitation its inviter's group_id. -- Invitations whose inviter has no group are left NULL and must be declined -- (query below) — under the restored code there is nothing for them to join. UPDATE ccsoccer_invitation i JOIN ccsoccer_registration r ON r.player = i.inviter AND r.season = i.season AND r.status NOT IN ('cancelled','expired') AND r.group_id IS NOT NULL AND r.group_id <> '' SET i.group_id = r.group_id WHERE i.status = 'pending' AND i.season IS NOT NULL AND (i.group_id IS NULL OR i.group_id = ''); -- Then decline whatever is left — an inviter with no group cannot be joined -- under the reverted code, and a pending invitation pointing nowhere is -- audit class IA. UPDATE ccsoccer_invitation SET status = 'declined', responded_at = UNIX_TIMESTAMP() WHERE status = 'pending' AND season IS NOT NULL AND (group_id IS NULL OR group_id = ''); ``` **Prefer a drush script over raw SQL** if there is time — the entity API fires `postSave()` cache invalidation, and `ROSTER_RECONCILIATION_PLAN.md` §6 already argues this case. Raw SQL is here because a rollback is usually happening under time pressure. **Note the asymmetry, and plan for it:** rolling *forward* needs no data step at all (§11.5 — old invitations keep their `group_id` and every new query tolerates both shapes). Rolling *back* does. **That asymmetry is a reason to be confident going in and careful coming out**, not a reason to skip the rollback plan. --- ## 13.1 Branch, commits and deploy Repo convention, and the plan's §10.4/§10.5 shape: - **Branch `fix/D8_group_at_accept` off `main`**, once the §10.8 testing backlog is clear. **Not `fix/roster_sync`** — that branch never existed and several older documents reference it wrongly. - **Five commits as scoped in §6**, each `php -l`-clean and each left in a state where the site loads. Do not squash — §13's rollback story depends on commit 3 being isolatable. - **Archive before editing**, per repo rule: `archive/_.php`, and **check the filename is not already taken** — that bit us on July 27. Twelve-plus files here, so this is not a formality. The Twig templates get the same treatment. - **`ddev drush cr` after commit 1 and after commit 2.** Container changes: one new service, one new exception class, constructor changes in `GroupController` (two new arguments), and `services.yml` edits for `ccsoccer.team_balancer` and `OrderCompleteSubscriber`. - **Deploy order: LOCAL → TEST → PROD**, with §12.2 cases 1-9 re-run on TEST before PROD. The concurrency cases (24-26) only need to pass on LOCAL, but **they must run against MySQL/MariaDB, not SQLite** — `forUpdate()` is a no-op there (§5.4). - **`drush config:status` before and after.** No config changes are expected from this work; anything that appears is drift from something else and should be understood before it is exported. - **PROD deploy needs a database backup first** — not because this migrates data (it does not) but because §13's rollback path writes to `ccsoccer_invitation`. --- ## 13.2 Definition of done Not done until all of these are true: - [ ] Every §12.2 and §12.3 case passes on LOCAL, including **24, 25 and 26** on MySQL/MariaDB - [ ] §12.4 invariant queries **C, D and E all return zero rows** after a day of normal use on TEST - [ ] `drush config:status` clean - [ ] No new `watchdog` errors on TEST across a full register → invite → accept → cancel cycle - [ ] The **eleven** open questions in §14 have recorded answers — in this file, not in a chat log - [ ] **§12.4 query B has been run and its answer recorded**, because it decides §10 (E vs B-narrow) - [ ] `ROSTER_DATA_AUDIT.sql` has gained query E - [ ] Appendix B's cross-references are updated — in particular **CF10 marked superseded** and **D-8 marked decided**, so the next session does not re-derive this - [ ] Andrew has been told about §11.1 (phantom groups vanish from the Roster Builder) and §11.2 (the admin loses a page) **before** the PROD deploy, not after --- ## 14. Open questions for Caleb These could not be settled from the code alone. **Q1, Q6, Q9 and Q10 should be answered before any code is written.** **Q1 — is `SELECT … FOR UPDATE` on both registration rows, deciding from the locked read, the right mechanism?** §5.4. It is the load-bearing piece of this design and the piece most likely to be wrong in a way testing will not reveal — `forUpdate()` is a **no-op on SQLite**, so a green test suite proves nothing about it. Drupal's `lock` service was rejected because its semaphore row is invisible inside `OrderCompleteSubscriber`'s open transaction; `loadUnchanged()` was rejected because it does not refresh the transaction's read view. **Two review passes each found this section wrong in a different way; treat a third disagreement as likely.** If there is a house pattern for this in the codebase, use it instead. **Q2 — bundle accept-time capacity checks, or defer?** §5.6. The brief says defer, on D-14's reasoning. **Q3 — inviter cancels before the invitee accepts: decline, or let them join alone?** §C3-b. The brief declines and tells the acceptor. The alternative recreates exactly the state D exists to prevent. **Q4 — E, or B(narrow)?** §10. Recommend deciding after §12.4 query B is run. **Q5 — is `Team::getRosterStats()`'s season branch (681-692) live?** No code path found gives a season Team a `group_id`. If dead, leave it and comment; if live, it needs the §6.8 treatment. **Q6 — the admin loses the Group Invitations page for a pre-acceptance invitation.** §11.2. Three options; the brief prefers "accept it". This is an admin-workflow call and the change most likely to generate a "where did it go" message during a roster build. **Q7 — may `GroupDissolveService::dissolveGroup()` grow a season lookup?** §6.7. The brief derives the season from the first registration rather than adding a required option, following D-12's reasoning. **Q8 — confirm the real table and column names** for the raw `forUpdate()` select. `ccsoccer_registration` + `id` + `group_id` are the assumption; a raw `select()` gets no help from the entity API if they are wrong. **Q9 — add the race-B guard in `invite()`?** §5.5. One query; refuse an invitation when the invitee already has a pending invitation out to the caller. It does not remove the need for locking but it turns the worst outcome from "two manager-less groups" into an error message. **Recommend yes.** **Q10 — the checkout path has no acceptor registration row to lock yet.** §C3-d point 1. The registration is created *after* the group is resolved (**548**), and `joinInviterGroup()` needs it to exist. The brief says restructure — create the registration ungrouped, then join — rather than passing NULL, which would reopen races B and C on the one path where they cost money. **This is the largest single edit in the change and the place where a second opinion is worth most before code exists.** **Q11 — a pending invitation's target group is now mutable.** §11.3. Three options; the brief prefers "accept and document", but it should be a decision. **Q12 — should `groups_locked` block an accept on the three paths that ignore it today?** §5.7. `groups_locked` is checked in one accept path out of four. Today that is a bounded bug; under D it means **a new group can appear after the admin locked groups**, which changes the Suggest Rosters output mid-build. The brief says fix `acceptSeasonInvitationDirectly()` and `OrderCompleteSubscriber` inside commit 3, because D creates this rather than merely sitting next to it. **The checkout half is a league-policy call, not a code one:** should locking groups stop a player who has already paid from being placed in the group they were invited to, or should payment win? The brief assumes lock wins and the order gets flagged — the same treatment as the existing capacity-exceeded branch. --- ## 15. Effort | | | |---|---| | Commit 1 (service + exception + the locked mint/join) | ~5-6 hrs — the `forUpdate()` work is most of it | | Commit 2 (inject two services into seven classes + re-key + both display blocks + `hook_theme()`) | ~6-7 hrs | | Commit 3 (the flip: `invite()` + four accept paths, incl. restructuring `createSeasonRegistration()`) | ~5-6 hrs | | Commit 4 (E) | ~1 hr | | Commit 5 (CF11) | ~2-3 hrs | | LOCAL testing §12 | ~6-7 hrs — 35 cases; three need two browsers and a constructed race | | **Total** | **~25-30 hrs** | Excludes the §5.7 `groups_locked` work (~1 hr, folded into commit 3 if Q12 says yes) and excludes the plan §10.8 testing backlog, which is a **prerequisite**, not part of this. **Roughly double the ~14-18 in the first draft.** Two review passes found six blockers whose fixes are not free: the atomic commit 3, the `forUpdate()` locking (twice), moving the acceptor's write inside the lock, the `manage()` display block, `CancelRegistrationForm`, and restructuring `createSeasonRegistration()`. **Cases 24-26 (the concurrency tests) are the least fun and the most important** — they are the only way to find out whether §5.4 is right, and they must run on MySQL/MariaDB. --- ## Appendix A — `Invitation.group_id` site inventory Verified against the working tree, August 5, 2026, and re-verified by an independent reviewer against the same tree. **Every site that reads, writes or filters `Invitation.group_id` appears here.** A handful of `Registration`/`Team` rows are included where they are the immediate consumer of an invitation's value; **this is not a complete `Registration.group_id` inventory** — that field has ~60 further sites, listed in `ROSTER_RECONCILIATION_PLAN.md` §5. | # | File:line | Kind | Entity | Method | Verdict under D | |---|---|---|---|---|---| | I-1 | `Entity/Invitation.php:95-101` | DEF | Invitation | `baseFieldDefinitions()` | keep | | I-2 | `GroupController.php:155-160` | FILTER | Invitation | `myRegistrations()` | **re-key** — §6.3 | | I-3 | `GroupController.php:243` | READ | Invitation | `myRegistrations()` | NULL while pending — §6.5 | | I-4 | `GroupController.php:270-274` | FILTER | Invitation | `myRegistrations()` | **delete** — dead data, §6.5 | | I-5 | `GroupController.php:510-515` | FILTER | Invitation | `manage()` | **drop `group_id`, and fix the gate at 501** — §6.4 | | I-6 | `GroupController.php:590-594` | FILTER | Invitation | `manage()` | **re-key** — §6.4 | | I-7 | `GroupController.php:836` | READ | Invitation | `invitationHasMatchingRegistration()` | safe — accepted only | | I-8 | `GroupController.php:838-841` | FILTER | Registration | `invitationHasMatchingRegistration()` | safe — accepted only | | I-9 | `GroupController.php:1054-1059` | FILTER | Invitation | `invite()` | **re-key** | | I-10 | `GroupController.php:1115` | **WRITE** | Invitation | `invite()` | **DELETE — this is D** | | I-11 | `GroupController.php:1422` | READ | Invitation | `acceptSeasonInvitation()` | **replace with inviter derivation** | | I-12 | `GroupController.php:1459` | WRITE | Registration | `acceptSeasonInvitation()` | source changes; **+ stamp the invitation** | | I-13 | `GroupController.php:1861-1867` | FILTER | Invitation | `removeMember()` | safe — accepted only | | I-14 | `GroupController.php:1931-1936` | FILTER | Invitation | `getGroupSize()` | **delete the method** — §5.2 | | I-15 | `GroupController.php:1990-1996` | FILTER | Invitation | `leaveGroup()` | safe — accepted only | | I-16 | `RegistrationController.php:475` | READ | Invitation | `acceptSeasonInvitationDirectly()` | **replace** | | I-17 | `RegistrationController.php:484` | WRITE | Registration | `acceptSeasonInvitationDirectly()` | source changes; **+ stamp** | | I-18 | `OrderCompleteSubscriber.php:496` | READ | Invitation | `createSeasonRegistration()` | **replace** | | I-19 | `OrderCompleteSubscriber.php:508-515` | FILTER | Registration | `createSeasonRegistration()` | **re-key** — §C3-d | | I-20 | `OrderCompleteSubscriber.php:520,526` | READ | Invitation | `createSeasonRegistration()` | add `inviter_uid` to the flag | | I-21 | `OrderCompleteSubscriber.php:535,557` | WRITE | Registration | `createSeasonRegistration()` | source changes | | I-22 | `GroupInvitationsForm.php:341-345` | FILTER | Invitation | `loadPendingInvitations()` | **re-key**, season branch only | | I-23 | `TeamBalancerService.php:1085-1087` | READ | Invitation | `getRosterState()` | **re-key via inviter; move below 1119** — §6.8 | | I-24 | `GroupPane.php:245` | READ | Invitation | `submitPaneForm()` | **remove the key** — §6.6 | | I-25 | `RosterBuilderController.php:385-389` | FILTER | Invitation | `mergeToGroup()` | **re-key** | | I-26 | `RosterBuilderController.php:394-405` | **WRITE** | Invitation | `mergeToGroup()` | **KEEP** — accept-time audit record | | I-27 | `RosterBuilderController.php:569-580` | **WRITE** | Invitation | `createGroup()` | **KEEP** — same | | I-28 | `Entity/Team.php:682-690` | FILTER | Invitation | `getRosterStats()` | verify — probably inert (Q5) | | I-29 | `GroupDissolveService.php:191-194` | FILTER | Invitation | `dissolveGroup()` | **add pending sweep** — §6.7 | **Three write sites, not one.** I-10 is the one D deletes; I-26 and I-27 are admin audit records created already-`accepted` and must keep writing `group_id`. **Not an `Invitation.group_id` site, but required by this change** (all found by review, all missed by the first pass): `CancelRegistrationForm.php:386-408` (§6.9), `GroupController.php:144` (§6.3), `GroupController.php:475, 501, 602` (§6.4), `GroupController.php:74-95` (constructor — neither `ccsoccer.group_membership` nor `ccsoccer.group_dissolve` is injected today), `templates/ccsoccer-my-registrations.html.twig:32-46` (§6.5), `ccsoccer.module:80-102` (`hook_theme()` — `can_disband`, §9), and `OrderCompleteSubscriber`'s `services.yml` definition plus `use LiveRegistrationTrait;` (§C3-d). --- ## Appendix B — cross-references to update when this ships - `ROSTER_RECONCILIATION_PLAN.md` §7 **D-8** → DECIDED, pointing here - `ROSTER_RECONCILIATION_PLAN.md` §10.3 **CF10** → SUPERSEDED by this brief - `ROSTER_RECONCILIATION_PLAN.md` §10.3 **CF11** → amended: route 4 only; `notify => FALSE`; §9 here is authoritative - `ROSTER_RECONCILIATION_PLAN.md` §7 **D-9** → resolves itself for routes 1-3; still needs the widened gate for route 4 - `ROSTER_RECONCILIATION_PLAN.md` §5 **CF3** → site 1 (`getGroupSize()`) is absorbed here; do not filter it twice - `ROSTER_DATA_AUDIT.sql` → add §12.4 query E (manager-less group detection) - `SESSION_HANDOFF.md` "Andrew and Caleb need to decide" → Decision 1 closed - `OUTSTANDING_ISSUES.md` P5 *"Accept-from-My-Registrations has no capacity check"* → now has a home (§5.6) and a service to build on - `ARCHITECTURE_DECISIONS.md` → add the §3 invariant --- ## Appendix C — adversarial review log **This brief was drafted, then reviewed line-by-line against the working tree by two independent reviewers, each instructed to assume the author was sloppy.** The first reviewed the draft; the second reviewed the corrections. ~140 citations were spot-checked across the two passes. Both are summarised here so Caleb can see what was already caught and where the errors clustered — that pattern is itself information about where the implementation risk sits. **Headline: six blockers total, four from the first pass and two from the second. All folded in. The second pass found that one of the first pass's fixes was itself wrong.** Do not read the code samples in §5.4 and §6 as settled; read them as the third attempt. ### Pass 1 — four blockers | # | First draft said | Truth | Where fixed | |---|---|---|---| | B1 | `loadByProperties(['group_id' => NULL])` silently matches every NULL row — a data leak | It **throws `InvalidQueryException`** (`Condition.php:106-108`). Six sites become 500s, one of them post-payment. The two `getQuery()` sites behave differently again. | §4.2 rewritten | | B2 | Six commits; "each leaves the site working"; "only commit 3 changes behaviour" | Splitting "stop minting" from "mint at accept" makes every accept write `group_id = NULL` **with** `invited_by` set — manufacturing the orphan state D exists to prevent | §4.3, §6 — merged into one commit | | B3 | A per-inviter lock closes the race | It closes race A only. D newly permits **mutual invitations** (Check 1 no longer fires), and two simultaneous accepts produce **two manager-less groups** neither player can escape | §5.4 rewritten; §5.5 added; §12.2 case 25 | | B4 | `\Drupal::lock()` + `loadUnchanged()` | The lock's semaphore row is invisible inside `OrderCompleteSubscriber`'s open transaction; `loadUnchanged()` does not escape InnoDB's REPEATABLE-READ snapshot. **Both fail exactly where the money is.** | §5.4 — replaced with `SELECT … FOR UPDATE` on both rows in id order | **Nine majors from pass 1, all folded in:** "exactly cap-neutral" was false and one of its bullets was backwards (§5.2, three real divergences now stated); `manage()` needed six coupled lines fixed, not one, and would have reproduced a bug already documented in its own source comments (§6.4); the invitation-card change would have rendered *"Already in group: Alice"* about a group that does not exist (§6.5); `CancelRegistrationForm` was missed entirely and would have left invitations pending against cancelled inviters (§6.9); the pending-invitation reparenting consequence was unanalysed (§11.3); `OrderCompleteSubscriber` does not use `LiveRegistrationTrait` and the draft's pseudo-code called `pickLiveRegistration()` on it — a fatal in the post-payment path (§C3-d); `RegistrationController` **does** use it, contrary to the draft; the §C3-e justification cited `RosterBuilderForm` as requiring two members for a group container, which is false and contradicted §11.1; and the "four near-identical copies" list had two wrong entries (§6.9). ### Pass 2 — two more blockers, in the pass-1 corrections | # | Corrected draft said | Truth | Where fixed | |---|---|---|---| | B5 | Take the row locks, then re-read through `loadUnchanged()` | A locking read **does not refresh the transaction's read view**. The subsequent ordinary read returns the pre-lock snapshot, so the mint decision is made on stale data — reintroducing the exact staleness the lock was taken to prevent. The `fetchAssoc()` result was being discarded. | §5.4 — decide from the locked read | | B6 | `resolveGroupForInviter()` returns a string; the caller writes the acceptor's row | Opens **race C**: one invitee accepting two invitations from two inviters simultaneously. Both requests lock disjoint row pairs, both read the acceptor as ungrouped, both mint. **One inviter is left alone in a group of one with an empty `invited_by`** — the exact shape D exists to remove. | §5.4 — renamed `joinInviterGroup()`, acceptor's write moved inside the lock; §12.2 case 26 added | **Six more majors from pass 2:** `GroupController` has neither `ccsoccer.group_dissolve` nor `ccsoccer.group_membership` injected today, and §8/§9 both called `$this->groupDissolve` (§6 commit 2); `can_disband` must be declared in `ccsoccer.module`'s `hook_theme()` or the Disband button **silently never renders, with no error** (§9); `GroupPane` was in Appendix A but in no commit (§6 commit 2); four test-case cross-references pointed at the wrong case; §6.5 asked for `is_manager` per member but the service only returned uids (`getMemberRegistrations()` added); and §6.2 scheduled the dissolve's pending sweep at lines 190-201 — *after* the clear loop that erases the membership it needs, which would have reproduced the exact bug §6.7 exists to prevent. **~23 minors and nits across both passes**, mostly line-number drift and internal cross-reference inconsistency, all corrected in place. Notable ones: four early returns not six (§2); `ccsoccer.install:4678` not 4684; `nudge()` ends at **1329**; `Invitation::accept()` has **two** callers; `forUpdate()` is a **no-op on SQLite**, so the concurrency tests prove nothing there (§5.4, §12.2); `deleteInvitation()` already emits a status message, so E would have shown two (§8); §12.4 query E's comment overstated what it detects; and §6.9's snippet used an undefined `$uid`. **What both reviews confirmed as correct**, and therefore does not need re-checking: the Check 1 loose-comparison analysis (`"uuid" == NULL` is FALSE in PHP 8, so the guard genuinely needs no change); the race-B analysis (both mutual invitations really would be created under D); `Select::forUpdate()` is a real Drupal 11 API and the `startTransaction()`/`rollBack()` semantics are as described; `invited_by` stores NULL and never 0, so §12.4 query E's predicate is sound; all four §5.3 queries; Appendix A's completeness and its Registration-vs-Invitation-vs-Team labelling across 382 `group_id` occurrences in 24 files; and no breakage in `nudge()`'s throttle, `available()`, `userSearch()`, the waitlist flow, views config, or the two JS call sites. ### The pattern worth noting Across both passes the errors clustered in three places, and **the pure data-flow analysis — which sites read what, what breaks under the new invariant — held up in every pass.** What did not: 1. **Concurrency.** Wrong three times: the lock scope, then the read primitive, then the write scope. Every fix closed one race and revealed another. 2. **Drupal API semantics** — what `loadByProperties`, the `lock` service, `loadUnchanged()` and `forUpdate()` actually do, and what `hook_theme()` requires. 3. **Display code** — `manage()`, `myRegistrations()`, the Twig templates. Consistently under-analysed because it has no data consequences, which is exactly why it is easy to skip and exactly what a player sees. **That is a reasonable prior for where the implementation will go wrong too.** If review time is limited, spend it on §5.4 and §6.4 and skim the rest.