# Season group cleanup — CF10 + B + E — Implementation Brief **Decision:** D-8, resolved as **CF10 + B + E** (Andrew, August 5 2026) **Date written:** August 5, 2026 **Status:** ◐ **PARTIALLY IMPLEMENTED, August 5. NOT YET REVIEWED BY CALEB.** **Branch:** **`fix/solo_group_auto_delete`** — *not* the `fix/D8_group_cleanup` proposed in §14. | Commit | Contents | State | |---|---|---| | `dde90b5` | §4 — `clearGroupIfOrphaned()` + `clearInviterGroupIfOrphaned()` on `GroupDissolveService`, four injections, `services.yml` | ✅ committed, linted, `drush cr` run | | `995e286` | §5 — **CF10** | ✅ committed, linted. Click-tests §9.2 cases 1-4 not yet run | | *(uncommitted)* | §6 — **B + E**, the thirteen call sites across six files | ⬜ **written, NOT linted, NOT tested** | **The remaining work is §9, not §6.** The code is written; what has not happened is lint, `drush cr`, the 29 test cases, and adding §9.4 query B to `ROSTER_DATA_AUDIT.sql`. **Review:** one adversarial pass against the working tree. **Six blockers and nine majors found and folded in, including two missed call sites and one pre-existing bug the design's metric depends on.** See Appendix C. **Scope:** season groups only. The tournament flow is untouched throughout. **Supersedes:** **D-8's option D.** `GROUP_ID_AT_ACCEPT_BRIEF.md` is **HELD** — see `D8_OPTIONS_COMPARISON.md` for why. **Reinstates:** **CF10**, which option D had made unnecessary. **Amends:** **CF11** — under this package route 4 clears itself, so the Disband button becomes optional insurance rather than the fix. §10. **Depends on:** CF8's `GroupDissolveService` (`b22734f`), CF4's `LiveRegistrationTrait` (`b46c28b`) — both merged to `main`, ~~both still unexecuted~~ **both LOCAL-tested Aug 5** (plan §10.8). --- > ### ▶ READ THIS FIRST > > **Line numbers were verified against the working tree on August 5, 2026, after PR #124 (`1eb1a15`).** > They will 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. > > ~~**Hard prerequisite:** …do not start until `ROSTER_RECONCILIATION_PLAN.md` §10.8 passes.~~ > **✅ SATISFIED — §10.8 was worked through on LOCAL and passed (Andrew, Aug 5)**, which is what > unblocked this work. PR #124's ~1,100 lines are linted and exercised, and registration 5088 is > cleaned up. The reasoning still stands for whoever comes next: this change calls > `GroupDissolveService` from **thirteen** places, so a bug in that service now has **fourteen** callers > to bisect rather than one. That is precisely why §10.8 came first. > > **This is a low-risk change with one high-risk property: it is easy to miss a call site.** The code is > simple. The discipline is in §6 being complete and in §9's audit query being run afterwards. **The > review of this document's first draft found two call sites it had missed and one pre-existing bug that > would have kept the audit query from ever reaching zero.** Assume there is a third. > > **`Invitation.group_id` keeps its current meaning and is never re-keyed.** This change *reads* it — > §4.2's carve-out and §4.5's helper both query it, and that is correct, because it is the authoritative > record of which group an invitation belongs to. What it must not do is change *when* the field is > written or what it means. **If you find yourself replacing a `group_id` filter with an `inviter` > filter, stop — you are building option D by accident.** --- ## 1. What this document is `SESSION_HANDOFF.md` recorded decision **D-8** with five candidate answers. Caleb proposed **D** (never mint a `group_id` until an invitation is accepted); Andrew proposed **E** (deleting the last pending invitation clears a now-empty group). D was specified in full in `GROUP_ID_AT_ACCEPT_BRIEF.md`, reviewed twice, and came out at ~25-30 hours with six blockers and three new concurrency races. **Andrew's call, August 5: build the cleanup instead.** `D8_OPTIONS_COMPARISON.md` is the reasoning. This document is the implementation spec for what was chosen. **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 as they are until someone touches them — though **§9.4's audit query counts them**, and once this ships, most will clear themselves the next time anything happens to that group. ### One framing correction carried over from the comparison memo **E is not a peer of B. E is one of B's thirteen call sites.** B is *"clear the group when nobody is left in it"*; E is that check, run from the Delete Invitation button. They are specified together here because they are the same fifteen lines. --- ## 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 } $current_size = $this->getGroupSize($season->id(), $group_id); } ``` **Four early `return`s sit between that `save()` and the invitation being created** — 945, 961, 1034, 1089. None of them unwinds the save. (Two more, at 989 and 1008, are inside the `if ($is_tournament)` arm and unreachable here.) Once a player holds a `group_id` with nobody else in it, they are stuck: - `invite()`'s Check 1 (**1025-1034**) blocks anyone from inviting them — *"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 managers by design. - `manage()` gates invitations-addressed-to-you on `if (!$group_id)` (**602**), so they cannot even see one waiting. **Only an admin using Dissolve Group can free them.** That is the ticket load this change removes. ### What is already handled — do not re-fix it All three player-facing season accept paths **already decline competing invitations** for that season: `acceptSeasonInvitation()` **1486-1500**, `acceptSeasonInvitationDirectly()` **509-523**, and `OrderCompleteSubscriber` **570-572** → `declinePendingInvitationsForRegistration()` **1029-1043**. The admin form does the same in `declineOtherInvitations()` **1462-1509**. So in the P1/P3 scenario — P1 invites P2, P3 invites P2, P2 accepts P1 — **P3's invitation is already auto-declined today.** What is missing is that nothing then clears P3's `group_id`. **That single gap is the headline case, and §6.4-§6.7 close it.** --- ## 3. The four routes, and what closes each | # | How a player ends up a permanent "group of one" | Closed by | |---|---|---| | 1 | Invite fails validation; `group_id` was already saved | **CF10** | | 2 | Invitee declines, or accepts a rival group | **B** (§6.2-§6.7) | | 3 | Invitee never answers; inviter deletes the invitation | **E** = B (§6.1) | | 4 | Group had members; all left or were removed | **B** (§6.8-§6.10) | **Route 1 is why CF10 is back in scope.** No decline happens, no delete happens, no member departs — so no B call site ever fires for that player. B genuinely cannot reach route 1; only moving the mint does. CF10 is a six-line move (§5). --- ## 4. The design: one predicate, one place Everything in §6 is a call to one function. ### 4.1 Where it lives **On `GroupDissolveService` (`src/Service/GroupDissolveService.php`), not a new service.** Three reasons: 1. It already exists, already does the clearing (`dissolveGroup()`), and already `use LiveRegistrationTrait;` (**:42**) so "live" has one definition (D-4 / D-13). 2. It is **already injected** into `CancelRegistrationForm` and `GroupInvitationsForm` — two of the thirteen call sites need no wiring at all. 3. *"Dissolve this group if nobody is left in it"* is cohesive with *"dissolve this group"*. A separate service would be a second home for group lifecycle logic, which is the drift shape this whole document cluster is about. **Cost of the choice:** three new injections — `GroupController`, `RegistrationController`, `OrderCompleteSubscriber`. See §6.0. ### 4.2 The method ```php /** * Dissolve a season group that has nothing left holding it together. * * D-8, option B. A season group_id is minted the moment someone clicks Invite * (GroupController::invite(), season branch), before the invitee has done * anything. If that invitation is then declined, ignored, deleted, or beaten * by a rival group — or if a real group's members all leave — the inviter is * left as a permanent "group of one" who cannot be invited by anyone, cannot * be merged in by an admin, and cannot leave. Only an admin Dissolve frees * them. This method is what every one of those paths calls afterwards so that * state never persists. * * Call it AFTER the mutation that might have emptied the group, never before — * it reads current state and decides. It is idempotent and safe to call on a * group that is fine, on a NULL group_id, and twice in a row. * * @param int $season_id * The season the group belongs to. * @param string|null $group_id * The group to test. NULL/'' is a no-op returning FALSE. * @param string $reason * Short phrase for the watchdog line, so a dissolve months from now can be * traced to the path that caused it. This is the only observability this * change gets — pass something specific. * * @return bool * TRUE if the group was dissolved. */ public function clearGroupIfOrphaned(int $season_id, ?string $group_id, string $reason = 'group emptied (D-8/B)'): bool { if (empty($group_id)) { return FALSE; } // Live members only — D-4/D-13, via LiveRegistrationTrait. A cancelled row // still carrying this group_id (audit class SB) must not keep the group // alive; that is half the reason groups look occupied when they are not. $registrations = $this->entityTypeManager->getStorage('ccsoccer_registration') ->loadByProperties(['group_id' => $group_id]); // Nothing carries this group_id at all — it is already gone. Return before // dissolveGroup(), which has no empty-set early exit and would write a // "Group @gid dissolved" watchdog line every time it was called // (GroupDissolveService.php:203). Without this the method is not idempotent // and every repeat call reports success. if (empty($registrations)) { return FALSE; } $live = 0; foreach ($registrations as $reg) { if (!$this->isDeadRegistrationStatus($reg)) { $live++; } } if ($live > 1) { return FALSE; } // THE CARVE-OUT. Without this, a manager who sent two invitations and had // one declined would have their group dissolved out from under the other — // which is the objection that kept option B off the table in July. Any // outstanding pending invitation means the group is still trying to form. $pending = $this->entityTypeManager->getStorage('ccsoccer_invitation') ->loadByProperties([ 'group_id' => $group_id, 'season' => $season_id, 'status' => 'pending', ]); if (!empty($pending)) { return FALSE; } // $live is 0 or 1 here. Zero is legitimate — every member cancelled — and // dissolveGroup() will clear whatever dead rows still carry the group_id, // which is exactly audit class SC. $this->dissolveGroup($group_id, [ 'notify' => FALSE, 'reason' => $reason, ]); return TRUE; } ``` ### 4.3 Three properties that make this safe **Idempotent — given the empty-set early return above.** Two members leaving simultaneously produces at worst a *missed* cleanup: both requests see two live members and neither dissolves. The group stays as it is until the next event. Nothing is corrupted. **Note the qualifier.** A second call on an already-dissolved group returns FALSE at the empty-set check, which is the intended no-op. **Without that check it would not be a no-op** — `dissolveGroup()` has no early exit for an empty result set, would write a watchdog line every time (`GroupDissolveService.php:203`), and the method would keep returning TRUE, so §6.1 would keep telling the player their group had been dissolved. It is also not quite true that re-clearing already-clear fields costs nothing: `Registration` uses `EntityChangedTrait` (`Registration.php:45`) and `dissolveGroup()` saves unconditionally (`:157`), so a redundant call bumps `changed` and fires presave/update hooks. **No new races.** The mint stays exactly where it is — one user, one request. This change adds no locking, no transactions, and no new concurrent write paths. *(That is the single biggest difference from option D, which introduced three races.)* **Fails into the status quo.** If a call site is missed, someone ends up in a group of one — today's behaviour, which admins already recognise and Dissolve already fixes. Compare with option D, whose failure modes were new states nobody has seen. ### 4.4 `notify => FALSE` everywhere — and the person it leaves in the dark `dissolveGroup()`'s notification tells a member *"your group was dissolved, you are still registered"* (CF8's `sendGroupDissolved()`). At the moment `clearGroupIfOrphaned()` fires there is **at most one live member**, so the notification loop would reach one person at most. **Be honest about who that person is.** Of the thirteen call sites in §7: | Whose group is being cleared | Sites | Did they cause it? | |---|---|---| | The current user's | 4 (§6.1, §6.11, §6.12, §6.13) | **yes** — they clicked | | **A third party's — the inviter of a declined invitation** | **7** (§6.2-§6.7, §6.9b) | **no** | | The administered group's | 2 (§6.8, §6.9a) | no — an admin did | *(An earlier draft of this section said "nine of eleven" clicked it themselves. That was wrong by a wide margin — it is four of thirteen.)* **So in nine of thirteen cases someone's group disappears without them doing anything.** `notify => FALSE` is still right, but not for the reason "they caused it". It is right because **`sendGroupDissolved()` is the wrong message** — it describes losing a group they had, when what actually happened is that the group they were *trying* to form never came together. **The right message already exists and mostly is not sent.** `declineInvitation()` **1658-1663** calls `sendInvitationDeclined()` — *"P2 declined your invitation"* — which is exactly what P3 needs to hear. But **five other decline sites set `status` directly and never notify**: `acceptSeasonInvitation()` **1453** and **1496**, `acceptSeasonInvitationDirectly()` **519**, `OrderCompleteSubscriber` **1040**, and `GroupInvitationsForm::declineOtherInvitations()` **1498**. **So today P3 is never told P2 chose someone else, and after this change P3's group also quietly disappears.** That is a worse silence than before, and it is the one user-visible regression in this change. **Q-B2 (§11) — recommend fixing it as a small follow-up, not in this commit.** ### 4.5 The second method — for the seven third-party sites Seven of the thirteen sites decline *someone else's* invitation and then need to clean up *that person's* group. Write it once, on the same service: ```php /** * Clear the group an invitation belonged to, if declining it emptied that group. * * D-8/B. For the paths that decline a THIRD PARTY's invitation — the rival * auto-declines that fire when a player accepts one invitation out of several. * The inviter is not otherwise involved in the request, so nothing else in * those code paths is looking at their group. * * Reads the group straight off the invitation. Invitation.group_id is the * authoritative record of which group the invitation was for, and it is still * written at invite time — CF10 moves when the REGISTRATION is saved, not the * invitation. Resolving the inviter's registration instead would be a query * per loop iteration, and would silently do nothing if the inviter has since * re-registered (their live row carries no group_id), leaving the stale group * standing. * * Season invitations only; tournament groups are Team entities. */ public function clearInviterGroupIfOrphaned(Invitation $invitation, string $reason): bool { if ($invitation->isTeamInvite()) { return FALSE; } return $this->clearGroupIfOrphaned( (int) $invitation->get('season')->target_id, $invitation->get('group_id')->value, $reason ); } ``` **Type-hint the concrete `\Drupal\ccsoccer\Entity\Invitation`.** There is no `InvitationInterface` — `Invitation extends ContentEntityBase` and implements no custom interface (`Invitation.php:44`). `ContentEntityInterface` also works if you prefer looseness, but then `isTeamInvite()` is not on the contract. **Call this *after* the invitation has been saved as `declined`**, never before — `clearGroupIfOrphaned()` reads current state, and a still-`pending` invitation trips its own carve-out and refuses. --- ## 5. CF10 — do not mint before validating **File:** `src/Controller/GroupController.php`, `invite()`, season branch. **Reinstated.** Option D had superseded it; with D held, it is back and it is the only thing that closes route 1. The season branch needs `$group_id` as a *value* for the checks below it — Check 1 compares the invitee's group to it (**1026**), Check 2 keys the duplicate lookup on it (**1058**). So keep generating it, just do not persist it until the invitation is about to be created. **Change, in four steps:** 1. At **931-937**, generate the UUID into a local **without** `set()`/`save()`, and track whether it is new: ```php // CF10 (D-8): do NOT persist the group_id here. Four early returns sit // between this point and the invitation being created (945, 961, 1034, // 1089) and none of them unwinds a save — so a mistyped address, or an // invitee who turns out to be in another group, would leave this player a // permanent "group of one" that only an admin can clear. // // The value is still needed below: Check 1 compares the invitee's group to // it (1026) and Check 2 keys the duplicate-invitation lookup on it (1058). $group_id = $registration->get('group_id')->value; $group_is_new = FALSE; if (!$group_id) { $group_id = $this->uuid->generate(); $group_is_new = TRUE; } $current_size = $this->getGroupSize($season->id(), $group_id); ``` 2. **The size check at 943-946 needs one line — its arithmetic shifts by one.** `getGroupSize()` (**1922-1939**) counts *registrations carrying the group_id* plus *pending invitations*. **Today the save at 936 happens before the call at 940, so a brand-new group returns 1** — the inviter is already a member of their own group, which is correct. **Under CF10 nothing has been saved yet, so it returns 0**, and the group appears one smaller than it is. ```php // CF10: the registration has not been saved with this group_id yet, so // getGroupSize() cannot see the inviter. Count them explicitly — they ARE // a member of the group they are creating. $current_size = $group_is_new ? 1 : $this->getGroupSize($season->id(), $group_id); ``` **Blast radius without this is narrow but real:** `getMaxGroupSize()` falls back to the league default (`Season.php:394-407`), so with a max of 3 the off-by-one only bites at the boundary. But it is a silent cap change, and *"the numbers on the Manage Group page moved by one after the deploy"* is exactly the kind of report nobody connects back to this. 3. Immediately **before** the invitation entity is created at **1118** — after every check has passed — persist it: ```php // CF10: the invitation is definitely being created now, so the group may // exist. Save the registration BEFORE the invitation, so there is never a // pending invitation pointing at a group_id no registration holds (audit // classes IA/SC). if ($group_is_new) { $registration->set('group_id', $group_id); $registration->save(); } $invitation = $this->entityTypeManager->getStorage('ccsoccer_invitation')->create($invitation_data); $invitation->save(); ``` 4. **Order matters and is not cosmetic.** Registration save first, then invitation. The reverse creates the IA/SC shape this whole cluster exists to remove. **Do not touch the tournament branch (891-925).** There `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. **Housekeeping in the same commit:** the paste artifact inside `invite()` is **three** blocks, not one — delete all three or none: **965-975** (*"UPDATED CODE for GroupController.php - invite() method"*, referencing dead line numbers ~583-605), **977-979** (`// ==== DUPLICATE INVITATION CHECKS ====`), and **1092-1094** (`// END OF DUPLICATE CHECKS - Continue with existing code below`). **Risk:** low, but re-read the method end to end after moving the block. Nothing between the old and new positions reads `$registration->get('group_id')` from the **entity** rather than the local `$group_id` — verified August 5. **But `getGroupSize()` reads it from the database**, which is step 2 above and is the one thing the move does change. **What CF10 does not do:** it does not help anyone already stranded, and it does not help routes 2-4. That is B's job. --- ## 6. The thirteen call sites **This section is the change.** Everything else is scaffolding. > **An earlier draft of this brief said eleven.** Review found two more: the *third* pass of > `GroupInvitationsForm::submitForm()` (§6.8), and `GroupInvitationsForm::acceptInvitation()`, which > overwrites a player's existing `group_id` with no guard (§6.9a). **Both are in the admin form.** If a > fourteenth exists it is probably there too — that method is the least-reviewed code in this cluster. Every site follows the same shape: **do the existing mutation, save, then call `clearGroupIfOrphaned()` on the group that might now be empty.** The only thing that varies is *whose* group that is. > **The distinction that causes all the difficulty:** in **four** sites the affected group belongs to the > **current user** (§6.1, §6.11, §6.12, §6.13). In **seven** it belongs to **the inviter of an invitation > being declined** — a third party not otherwise involved in the request. In **two** it is the group an > admin is editing. **The seven third-party sites are where the bugs will be**, and they all go through > the §4.5 helper for exactly that reason. ### 6.0 Wiring | Class | Has `ccsoccer.group_dissolve` today? | Do | |---|---|---| | `GroupInvitationsForm` | **yes** (`:104`) | nothing | | `CancelRegistrationForm` | **yes** (`:58-72`) | nothing | | `GroupController` | **no** — constructor takes six args at `:74` | inject: constructor + `create()` at `:86` | | `RegistrationController` | **no** — has no `__construct`, only `create()` at `:51` | use `\Drupal::service('ccsoccer.group_dissolve')`, matching its existing `\Drupal::service('ccsoccer.notification')` at `:495` | | `OrderCompleteSubscriber` | **no** | add `'@ccsoccer.group_dissolve'` to `ccsoccer.services.yml:109-117` + constructor arg | | `RosterBuilderController` | **no** | inject — needed by §6.9b | **`ddev drush cr` is mandatory** after this — three constructor changes and a `services.yml` edit. --- ### 6.1 `GroupController::deleteInvitation()` — **route 3. This is E.** **Today** (**1334-1384**): verifies the caller is the inviter, refuses anything not `pending`, hard-deletes, redirects. Never touches `group_id`. `$season_id` is already read at **1348**, before the delete, for the redirect lookup. Keep that ordering. **Put the call below the existing `$reg = $this->pickLiveRegistration($my_reg);` at :1378**, not straight after the delete — the method already loads the caller's registration at **1364-1370** and resolves it at **1378** for its own redirect. Reuse it; do not run the same two queries twice. ```php // (replacing the redirect block at 1379-1381) $reg = $this->pickLiveRegistration($my_reg); if ($reg) { // D-8/E: that may have been the last thing holding this group together. if (!$is_team_invite && $season_id && $this->groupDissolve->clearGroupIfOrphaned( (int) $season_id, $reg->get('group_id')->value, '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.group', ['registration' => $reg->id()]); } ``` **The message is inside the `if`** — only shown when a dissolve actually happened. The existing *"Invitation deleted."* at **1354** stays; the two read fine together. **No manager check is needed.** `clearGroupIfOrphaned()` tests live-member count, not role. A *non-manager* member who deletes an invitation they sent leaves a group with ≥2 members, so the count check refuses. That is correct and requires no extra guard. **§9.3 case 5 proves it.** --- ### 6.2 `GroupController::declineInvitation()` — **route 2a, the invitee declines** **Today** (**1625-1674**): sets `declined` + `responded_at`, saves, notifies the inviter via `sendInvitationDeclined()` (**1658-1663**), redirects. The affected group is **the inviter's**, not the current user's — so this is a §4.5 call, one line: ```php // (after the existing notification block, ~1664) // D-8/B: the inviter may now have nothing left in their group. $this->groupDissolve->clearInviterGroupIfOrphaned($invitation, 'invitation declined by invitee (D-8/B)'); ``` **The inviter is already notified here** by the existing `sendInvitationDeclined()` call at **1658-1663** — this is the one decline path that does. §4.4. **Watch out:** `declineInvitation()` has **no `status === 'pending'` guard** (compare `deleteInvitation()` **1342**). It declines whatever it loads, so a repeated POST re-fires this call. Harmless given §4.2's empty-set early return, but it is why that early return matters. **All seven third-party sites are this same one line** — §6.2, §6.3, §6.4, §6.5, §6.6, §6.7, §6.9b. Only the `$reason` string differs. --- ### 6.3 `GroupController::acceptSeasonInvitation()` :1450-1456 — **"you are already in a group"** This branch declines the invitation the player just tried to accept, because they are already grouped. The **inviter** of that invitation may now be orphaned. ```php // Check if already in a group if ($registration->get('group_id')->value) { $this->messenger()->addError($this->t('You are already in a group for this season.')); $invitation->set('status', 'declined'); $invitation->save(); // D-8/B: this decline may have emptied the inviter's group. $this->groupDissolve->clearInviterGroupIfOrphaned($invitation, 'accept refused, invitee already grouped (D-8/B)'); return $this->redirect('ccsoccer.group', ['registration' => $registration->id()]); } ``` **`RegistrationController::acceptSeasonInvitationDirectly()` :478-481 is NOT a twin — it does not decline.** Verified: ```php // RegistrationController.php:478-481 if ($registration->get('group_id')->value) { $this->messenger()->addWarning($this->t('You are already in a group for this season.')); return $this->redirect('ccsoccer.group', ['registration' => $registration->id()]); } ``` It warns and returns, leaving the invitation `pending`. **So there is nothing to clean up and it is not a call site.** *(That the two Accept buttons disagree about whether to decline is arguably a bug — one leaves a pending invitation the player can retry, the other kills it. Not this change's bug; recorded in §11 Q-B5 as answered-and-deferred.)* --- ### 6.4 `GroupController::acceptSeasonInvitation()` :1486-1500 — **rival declines (loop)** **This is the P1/P3 scenario. It is the single highest-value site in the change.** ```php // Decline other pending invitations for this season $other_invitations = $this->entityTypeManager->getStorage('ccsoccer_invitation') ->loadByProperties([ 'invitee_email' => $this->currentUser->getEmail(), 'season' => $season_id, 'status' => 'pending', ]); foreach ($other_invitations as $other) { if ($other->id() != $invitation->id()) { $other->set('status', 'declined'); $other->set('responded_at', \Drupal::time()->getRequestTime()); $other->save(); // D-8/B: P3's group exists only because P3 invited this player. Now that // they have joined someone else, P3 has nothing left. THIS is the case // that used to require an admin Dissolve or a Disband button. $this->groupDissolve->clearInviterGroupIfOrphaned($other, 'rival invitation auto-declined (D-8/B)'); } } ``` **Duplicate `// Decline other pending invitations for this season` comment at 1484 and 1486** — delete one while you are here. --- ### 6.5 `RegistrationController::acceptSeasonInvitationDirectly()` :509-523 — **rival declines (loop)** Identical block, identical addition. **Both Accept buttons on the site route through different controllers** — My Registrations → `GroupController`, the Register page → `RegistrationController` — and both were broken by the Aug 4 bug for exactly this reason. **Do not fix one and assume the other.** Use `\Drupal::service('ccsoccer.group_dissolve')`, consistent with **:495**. > #### ⚠ The notification gap these four loops expose > > `declineInvitation()` notifies the inviter (`sendInvitationDeclined()`, **1658-1663**). **None of the > four auto-decline loops does** — they set `status` and `responded_at` directly and never notify. > > So today, when P2 accepts P1's invitation, **P3 is never told their invitation was declined.** After > this change P3's group also silently disappears. P3 gets no message either way. > > **This is pre-existing and out of scope**, but it becomes more noticeable: previously P3 at least > still had a (broken) group; now they have nothing and no explanation. **Q-B2 (§11)** — > recommend adding `sendInvitationDeclined()` to the four loops as a small follow-up, not in this > commit. --- ### 6.6 `OrderCompleteSubscriber::declinePendingInvitationsForRegistration()` :1029-1043 — **season arm, loop** ```php else { $season_id = $registration->get('season')->target_id; $invitations = $this->entityTypeManager->getStorage('ccsoccer_invitation') ->loadByProperties([ 'invitee_email' => $email, 'season' => $season_id, 'status' => 'pending', ]); foreach ($invitations as $invitation) { $invitation->decline(); $invitation->save(); // D-8/B: each declined invitation may leave its inviter with an empty // group. Must not throw — payment has already been captured. try { $this->groupDissolve->clearInviterGroupIfOrphaned($invitation, 'rival invitation declined at checkout (D-8/B)'); } catch (\Throwable $e) { $this->logger->error('Order completion: group cleanup for invitation @inv failed: @msg', [ '@inv' => $invitation->id(), '@msg' => $e->getMessage(), ]); } } } ``` **The `try/catch` is not optional.** This runs inside Commerce's order transition, after payment has been captured. A stranded group is a support ticket; an exception escaping the subscriber is a failed order. **Catch `\Throwable`, log, carry on.** **Note this method is called only when `$invitation_status === 'accepted'`** (**570-572**) — i.e. the player actually accepted an invitation during checkout. That is the correct trigger. --- ### 6.7 `GroupInvitationsForm::declineOtherInvitations()` :1487-1505 — **admin accept, rival declines (loop)** Season branch only (**1462-1468** builds the season set; **1469-1485** the tournament set). The loop at **1488-1505** is shared, so gate the addition on `$context_type === 'season'`: ```php $other_invitation->set('status', 'declined'); $other_invitation->set('responded_at', \Drupal::time()->getRequestTime()); $other_invitation->save(); $declined_count++; // D-8/B — season only; tournament groups are Team entities. if ($context_type === 'season') { $this->groupDissolve->clearInviterGroupIfOrphaned($other_invitation, 'rival invitation auto-declined by admin (D-8/B)'); } ``` `$this->groupDissolve` is already injected (**:104**). No wiring needed. --- ### 6.8 `GroupInvitationsForm::submitForm()` — **admin edits, and there are THREE passes** **This was wrong in the first draft and is the most important correction in this document.** `submitForm()` does not have one loop, it has three, and the group can shrink in two of them: | Pass | Lines | What it writes | |---|---|---| | 1 | ~930-1000 | captain / co-captain rows | | **2** | **1001-1072** | **member registration rows** — `:1037-1040` clears `group_id` + `invited_by` on a `declined` row | | **3** | **1074-1108** | **invitation rows** (`inv_*`) — `:1104-1105` writes `$invitation->set('status', $new_status); $invitation->save();` | **Pass 3 is a route-3 site by another door.** An admin flipping a solo manager's last pending invitation to `declined` removes the very thing §4.2's carve-out was protecting. **A call placed after pass 2 runs before pass 3 and misses it entirely.** **So: one call, after pass 3, before the `$changes_made` block at :1110.** ```php // D-8/B: run ONCE, after all three passes have saved. Pass 2 can remove the // last member and pass 3 can decline the last pending invitation, so a call // placed after either one alone tests a half-applied state and misses the // other. Do not move this into a loop. if ($context_type === 'season') { $this->groupDissolve->clearGroupIfOrphaned( (int) $form_state->get('context_id'), $group_id, 'admin edited group rows (D-8/B)' ); } ``` **`$context_id` is NOT a local in this method.** `submitForm()` pulls exactly five values from form state at **:924-929** — `group_id`, `context_type`, `team_id`, `captain_reg_id`, `co_captain_reg_id`. The season id must be fetched: `$form_state->get('context_id')`, matching **:868** and **:1350**. *(The first draft used a bare `$context_id` and would have been an undefined-variable notice at best.)* `$group_id` **is** a local, from **:924**. `$this->groupDissolve` is already injected (**:104**). --- ### 6.9 The admin placement paths — two more sites, both found in review #### 6.9a `GroupInvitationsForm::acceptInvitation()` :1321-1328 — **no "already in a group" guard** ```php // GroupInvitationsForm.php:1321-1328 $registration = $this->pickLiveRegistration($existing_regs); if ($registration) { // Update existing registration. $registration->set('group_id', $group_id); $registration->set('invited_by', $inviter_id); $registration->set('invitation_status', 'accepted'); $registration->save(); ``` **There is no check for an existing `group_id`** — unlike `GroupController::acceptSeasonInvitation()` (**1450-1456**) and `RegistrationController::acceptSeasonInvitationDirectly()` (**478-481**), both of which refuse. So an admin accepting an invitation for a player who is **currently in group X** silently overwrites their `group_id`, and **X can drop to one live member with nothing cleaning it up.** ```php if ($registration) { // D-8/B: capture the group being left BEFORE it is overwritten. This // path has no "already in a group" guard, so an admin accept can move a // player out of a group they were already in. $old_group_id = $registration->get('group_id')->value; $registration->set('group_id', $group_id); … $registration->save(); if ($old_group_id && $old_group_id !== $group_id) { $this->groupDissolve->clearGroupIfOrphaned((int) $season_id, $old_group_id, 'player moved out by admin accept (D-8/B)'); } ``` **The missing guard is itself arguably a bug** — the two player-facing paths refuse this and the admin path does it silently, with no confirmation and no way back (registrations are not revisionable). That is the same shape as **D2 bug 3**, which CF2 fixed on the *tournament* side of this very method (**1383-1391**) by refusing when the player is already on another team. **The season side never got the equivalent.** Out of scope here — recorded as **Q-B7** — but worth knowing it is why 6.12a exists. #### 6.9b `RosterBuilderController::mergeToGroup()` / `createGroup()` — **the reason the metric would never reach zero** **This one is not a missing cleanup call. It is a missing decline, and it breaks §9.4's safety net.** Every accept path in the module declines the invitee's *other* pending invitations — `GroupController` **1494-1500**, `RegistrationController` **517-523**, `OrderCompleteSubscriber` **1039-1042**, `GroupInvitationsForm` **1488-1505**. **The two Roster Builder placement paths do not:** `mergeToGroup()` (**392-417**) and `createGroup()` (**567-586**) place the player, mint an `accepted` audit invitation, and leave every other pending invitation to them untouched. **Consequence under this design:** those orphaned pending invitations keep their inviter's solo group alive **forever**, via §4.2's carve-out. No call site is missing — the predicate is just permanently satisfied. **§9.4 query B would never settle at zero, and §12 would diagnose it wrongly** as a missed call site. **Fix, in both methods, after the placement saves:** mirror what every other accept path does. ```php // D-8/B: an admin placing this player into a group is an accept. Decline // their other pending invitations for this season, exactly as every // player-facing accept path does — otherwise those invitations keep their // inviters' now-pointless groups alive indefinitely. $others = $invitation_storage->loadByProperties([ 'invitee_email' => $player_user->getEmail(), 'season' => $season, 'status' => 'pending', ]); foreach ($others as $other) { $other->decline(); $other->save(); $this->groupDissolve->clearInviterGroupIfOrphaned($other, 'player placed into a group by admin (D-8/B)'); } ``` `RosterBuilderController` needs `ccsoccer.group_dissolve` — a fourth injection. **`$player_user` is already loaded** in `mergeToGroup()` at **:400**; check `createGroup()` for an equivalent. **This is the one place this change fixes a bug it did not create.** It is in scope because without it the design has no working health metric. --- ### 6.10 Three paths deliberately left uncovered — and why that is a decision, not an oversight Admin entity forms can shrink a group without going through any of the thirteen sites: | Path | Route / evidence | Effect | |---|---|---| | **Delete a registration** | `ccsoccer.routing.yml:603-606`, `ContentEntityDeleteForm` (`Registration.php:26`) | a live member vanishes; no cleanup | | **Edit a registration's `status`** to `cancelled`/`expired` | `ccsoccer.routing.yml:595-598`; `status` is an `options_select` (`Registration.php:108`) | a live member becomes dead; no cleanup | | **Delete an invitation** from the admin collection | `Invitation.php:36-41`, `/admin/ccsoccer/invitations` | the last pending invitation vanishes; no cleanup | **There is no `hook_entity_delete` anywhere in the module** — verified. *(`ccsoccer_registration_delete()` exists but is misnamed and has never fired: for a module named `ccsoccer` and an entity type named `ccsoccer_registration`, `hook_ENTITY_TYPE_delete` is `ccsoccer_ccsoccer_registration_delete()`. That double prefix is exactly the trap the plan's Aug 4 audit recorded.)* **Reassuringly, `Registration.group_id` itself has no form widget** (`Registration.php:156-159` sets no `setDisplayOptions('form', …)`), so an admin cannot hand-edit a group membership through the UI. Only `status` and outright deletion are reachable. **Recommendation: leave these uncovered and rely on §9.4 query B.** All three are rare, admin-only, and deliberate actions; an admin who deletes a registration can dissolve the group in the same sitting. The alternative — `hook_ccsoccer_registration_delete()` / `_update()` — is the entity-hook variant discussed in `D8_OPTIONS_COMPARISON.md` §8, which closes these *and* every future path at the cost of recursion guards and harder debugging. **Q-B8.** If the audit query shows these actually happen, that is the trigger to build it. --- ### 6.11 `GroupController::removeMember()` :1830-1833 — **route 4, manager removes a member** ```php // Remove member from group $member_registration->set('group_id', NULL); $member_registration->set('invited_by', NULL); $member_registration->set('invitation_status', 'none'); $member_registration->save(); ``` `$group_id` is read at **1731** from the *manager's* registration — the group being emptied. Add after the existing accepted-invitation decline block (**1858-1874**), so pending invitations have been accounted for: ```php // D-8/B: removing the last other member leaves the manager alone. if (!$is_tournament) { $this->groupDissolve->clearGroupIfOrphaned( (int) $registration->get('season')->target_id, $group_id, 'last member removed by manager (D-8/B)' ); } ``` **Read `$group_id` from the local at 1731, not from the entity** — by this point the member's row has been cleared, and re-reading would give NULL. --- ### 6.12 `GroupController::leaveGroup()` :1983-1986 — **route 4, member leaves** ```php $group_id = $registration->get('group_id')->value; // :1980 — read BEFORE the clear $registration->set('group_id', NULL); … $registration->save(); ``` The local at **1980** is already captured before the clear. Add after the accepted-invitation decline block (**1989-2002**), before the redirect: ```php // D-8/B: the last member leaving leaves the manager alone. $this->groupDissolve->clearGroupIfOrphaned((int) $season->id(), $group_id, 'last member left group (D-8/B)'); ``` Tournaments already return early at **1963-1966**, so no branch is needed. --- ### 6.13 `CancelRegistrationForm::submitForm()` :389-410 — **route 4, a member cancels** The existing code handles the *manager* cancelling (dissolve, D-2). Its own comment at **399-400** says what is missing: > *"An ordinary member leaving does nothing further — the group survives with one fewer member."* **That is true and now incomplete** — if the member who cancelled was the last one, the manager is stranded. ```php if ($was_manager) { $this->groupDissolve->dissolveGroup($group_id, [ … ]); // existing, unchanged } else { // D-8/B: an ordinary member cancelling can still leave the manager // alone. The group survives only if someone is left in it. $this->groupDissolve->clearGroupIfOrphaned((int) $season->id(), $group_id, 'member cancelled registration (D-8/B)'); } ``` `$group_id` is captured at **386** before the clear, and `$was_manager` at **387** — the ordering trap the existing comment at **382-385** warns about. **Do not disturb it.** `$this->groupDissolve` is already injected. **Update the comment at 399-400** so it stops describing the old behaviour. --- ## 7. Call-site summary | # | § | Site | Route | Whose group | Wiring | |---|---|---|---|---|---| | 1 | 6.1 | `GroupController::deleteInvitation()` — after `:1378` | 3 (**E**) | current user | inject | | 2 | 6.2 | `GroupController::declineInvitation()` — after `:1664` | 2a | **inviter** | inject | | 3 | 6.3 | `GroupController::acceptSeasonInvitation()` — after `:1454` | 2 | **inviter** | inject | | 4 | 6.4 | `GroupController::acceptSeasonInvitation()` — after `:1498` **(loop)** | 2b | **inviter** ×N | inject | | 5 | 6.5 | `RegistrationController::acceptSeasonInvitationDirectly()` — after `:521` **(loop)** | 2b | **inviter** ×N | `\Drupal::service()` | | 6 | 6.6 | `OrderCompleteSubscriber` — after `:1041` **(loop)** | 2b | **inviter** ×N | services.yml | | 7 | 6.7 | `GroupInvitationsForm::declineOtherInvitations()` — after `:1498` **(loop)** | 2b | **inviter** ×N | already injected | | 8 | 6.8 | `GroupInvitationsForm::submitForm()` — **after `:1108`**, not after the member loop | 2c + 3 | administered group | already injected | | 9 | **6.9a** | `GroupInvitationsForm::acceptInvitation()` `:1321-1328` — **found in review** | — | the group being **left** | already injected | | 10 | **6.9b** | `RosterBuilderController::mergeToGroup()` `:392-417` — **found in review**; adds a missing decline | 2b | **inviter** ×N | **inject (4th)** | | 11 | **6.9b** | `RosterBuilderController::createGroup()` `:567-586` — same | 2b | **inviter** ×N | inject | | 12 | 6.11 | `GroupController::removeMember()` — after `:1874` | 4 | current user | inject | | 13 | 6.12 | `GroupController::leaveGroup()` — after `:2002` | 4 | current user | inject | | 14 | 6.13 | `CancelRegistrationForm::submitForm()` — `else` at `:401-409` | 4 | current user | already injected | **Fourteen rows, thirteen sites** — §6.9b is one fix applied to two methods. Plus **CF10** (§5) in `GroupController::invite()` `:931-937`, `:940` and `:1118`. **Four injections, not three:** `GroupController`, `OrderCompleteSubscriber`, `RosterBuilderController` (constructor + `create()`), and `RegistrationController` via `\Drupal::service()`. **Verified clean, and worth recording so nobody re-checks:** - **No direct SQL writes anywhere.** No `->update('ccsoccer_registration')` / `ccsoccer_invitation` or their `_field_data` variants. Every mutation goes through the entity API. - **The complete inventory of season `group_id = NULL` writes is exactly five:** `GroupInvitationsForm:1039`, `CancelRegistrationForm:393`, `GroupController:1830`, `GroupController:1983`, `GroupDissolveService:153`. §6.8, §6.13, §6.11 and §6.12 cover four; the fifth is the dissolve itself. **On that axis §6 is complete** — the gaps found in review were all on the other three axes (status writes, invitation writes, entity deletes). - **Drush commands** (`CcsoccerCommands.php` 1445, 1489, 1951, 1962) only *create* group membership. None removes it. - **`ccsoccer.install`'s `group_id` writes** (2714, 2733, 4726, 4735) are tournament-only — `_ccsoccer_repair_legacy_team_group_ids()` (**4678-4745**) is guarded by `STARTS_WITH 'team_'` at **4687**. - **`TeamBalancerService` never writes `group_id`.** It reads it for unit-building (**380-416**), and `syncGroupToRoster()` (**1381-1425**) writes only `team`. - **`TournamentRosterBuilderController:210`** and **`TournamentTeamManager:623`** are the only other `group_id = NULL` writes and are both tournament-scoped. **Deliberately NOT call sites:** - **`GroupDissolveService::dissolveGroup()`** — it *is* the dissolve. Calling the check from inside it would recurse. - **Anything in the tournament flow.** Tournament groups are Team entities; `Team.group_id` is authoritative and none of this applies. - **`GroupController::acceptTeamInvitation()`, `RegistrationController::acceptTeamInvitationDirectly()`, `OrderCompleteSubscriber`'s tournament arm (:1023), `removeMember()`'s tournament arm (:1852)** — all tournament. - **`invite()` itself.** CF10 stops the state being created; nothing to clean afterwards. - **The three admin entity-form paths in §6.10** — a decision, not an omission. Read that section. --- ## 8. What this change does NOT do - **No data repair.** Existing stranded groups survive the deploy. Most will clear themselves the next time anything happens to them; the rest need an admin Dissolve or CF11. §9.4 counts them. - **`Invitation.group_id` is untouched.** All 29 sites documented in `GROUP_ID_AT_ACCEPT_BRIEF.md` Appendix A keep working exactly as they do now. - **`getGroupSize()` is untouched.** No cap arithmetic changes. **CF3 still owns its ten status filters, including `getGroupSize()`'s** — that is a separate fix and this change does not pre-empt it. - **No concurrency changes.** No locking, no transactions, no new races. - **No template changes.** Nothing a player or admin looks at moves. - **The four auto-decline loops still do not notify.** §6.5, Q-B2. - **`groups_locked` is still only enforced on one accept path out of four** (`GROUP_ID_AT_ACCEPT_BRIEF.md` §5.7). **B does not make this worse** — unlike option D, which turned it into "a new group can appear after the roster is locked". Still wrong, no longer urgent. - **CF11 (Disband) is not included.** §10. - **The three admin entity-form paths in §6.10 stay uncovered.** A decision, not an oversight — Q-B8. --- ## 9. Test plan ### 9.1 Pre-flight - [x] `ROSTER_RECONCILIATION_PLAN.md` §10.8 passes **first** — **DONE Aug 5** - [x] Registration **5088** cleaned up (carried bad state from the Aug 4 reproduction) — **DONE Aug 5** - [x] `ddev php -l` + **`ddev drush cr`** for commits 1 and 2 — **DONE Aug 5**. Three constructor changes (`GroupController`, `OrderCompleteSubscriber`, `RosterBuilderController`) plus the `services.yml` edit all landed in `dde90b5`. `RegistrationController` uses `\Drupal::service()` and needed no constructor change. See §6.0 - [ ] **`ddev php -l` on commit 3's six files** — `GroupController`, `RegistrationController`, `RosterBuilderController`, `OrderCompleteSubscriber`, `GroupInvitationsForm`, `CancelRegistrationForm` — then **`ddev drush cr`** again - [ ] `drush config:status` clean - [ ] **§9.4 query A run and its count recorded**, so the before/after comparison is possible ### 9.2 The four routes — the acceptance tests Check `Registration.group_id` in the database after each, not just the screen. 1. **Route 1 (CF10) — bad email.** Registered player with no group invites an address that resolves to nobody → error, **and their `group_id` is still NULL.** 2. **Route 1 — invitee already in another group.** → *"already in another group for this season"*, **inviter's `group_id` still NULL.** 3. **Route 1 — empty search box.** → *"Please select a player or enter an email"*, **`group_id` NULL.** 4. **Route 1 — happy path unchanged.** Valid invite → `group_id` set exactly as today, invitation created carrying it, invitee sees it. **Then a second valid invite from the same manager reuses the same `group_id`** — no new UUID. 5. **Route 3 (E) — delete the only invitation.** Manager invites, then deletes → **`group_id` cleared**, message shown, and the manager can now be invited into someone else's group. 6. **Route 3 — the carve-out.** Manager sends **two** invitations, deletes one → **group NOT cleared**, the other still pending. *This is the test that stops B dissolving groups out from under people.* 7. **Route 2a — invitee declines.** P1 invites P2; P2 declines from My Registrations → **P1's `group_id` cleared**, P1 receives the existing "declined" notification. 8. **Route 2b — the P1/P3 scenario. THE headline test.** P1 invites P2; P3 invites P2; P2 accepts P1's from My Registrations. → P3's invitation auto-declined (as today) **and P3's `group_id` is cleared**. Then **P1 invites P3 → succeeds**, where before it said *"already in another group"*. P3 accepts → group of 3. *This used to require an admin.* 9. **Route 2b via the other Accept button.** Repeat case 8 but have P2 accept from the **Register page** (`RegistrationController` path, magic link). Same assertions. **Both buttons must be tested.** 10. **Route 2b via checkout.** P2 is not yet registered; P1 and P3 both invite. P2 registers and pays with P1's invitation selected in `GroupPane`. → P3's invitation declined **and P3's group cleared**, order completes normally. 11. **Route 2c — admin accept.** P1 and P3 both invite P2. Admin accepts P1's from the Group Invitations page → P3's invitation declined **and P3's group cleared**. 12. **Route 2 — the carve-out again.** P3 has invited **two** people, one of whom accepts P1's group instead. → P3's group is **NOT** cleared; the other invitation is still pending. 13. **Route 4 — manager removes the last member.** Group of 2 → manager removes the member → **manager's `group_id` cleared**, manager can now accept another invitation. 14. **Route 4 — member leaves.** Group of 2 → member clicks Leave Group → **manager's `group_id` cleared**. 15. **Route 4 — member cancels.** Group of 2 → the *member*'s registration is cancelled → **manager's `group_id` cleared** (§6.13). *Do not confuse with the manager cancelling, which is CF8's existing dissolve — test that too and confirm it is unchanged.* **Note this is an admin action**: cancellation is `administrator`-only and there is no player-facing cancel route (Q-B9). 16. **Route 4 — group of 3.** Remove one member → group of 2 survives, **nothing cleared**. Remove the second → cleared. 17. **Route 4 with a pending invitation.** Group of 2 with one invitation outstanding → member leaves → **group NOT cleared** (carve-out), manager still sees the pending invitation and can still nudge or delete it. 18. **Admin per-row decline — pass 2.** Group of 2 → admin flips the member's row to `declined` and saves → **manager's group cleared** (§6.8). **Assert the SCREEN, not just the data:** the admin is **redirected to the Roster Builder** and sees *"Only the group manager was left, so the group was dissolved…"* alongside *"Updated 1 member(s)."* > **Found in LOCAL testing, Aug 5 — this case as originally written passed while the page was > broken.** It asserted the cleared `group_id` and stopped there. In reality the form rebuilt > against a `group_id` no registration carried any more, so `buildForm()`'s empty-state branch > fired — and that branch returns **before** `$form['actions']` is built, leaving a page with no > Save, no Dissolve Group and **no Back to Roster Builder**. The admin was stranded. > > The empty state is pre-existing and was written for a genuinely empty admin-created team. **B is > what made it reachable:** before this change, declining the last member left the manager's own > `group_id` set, so there was always at least one row to render. Fixed by redirecting on dissolve, > mirroring what the manual Dissolve Group button already did. > > **The lesson generalises: a case that asserts only a database outcome can pass over a broken > screen.** Cases 13-15 and 19 have the same shape — check what the user is looking at. 19. **Admin declines the last pending invitation — pass 3.** Solo manager with one pending invitation → admin flips the **invitation** row to `declined` and saves → **group cleared**, and the same redirect and message as case 18. *This is the site the first draft missed. A call placed after the member loop instead of after the third pass passes case 18 and fails this one — which is exactly why it is a separate case.* 19a. **The group survives — no redirect.** Group of **3** → admin declines one member → group of 2 survives, **nothing dissolved, no redirect**, admin stays on the Group Invitations page with Save, Dissolve Group and Back to Roster Builder all present. *The negative half of case 18, and the state Andrew was in after declining the first of two members.* 20. **Admin accept moves a player out of another group.** P4 is in a group with P5. Admin opens P1's group and accepts an invitation for P4 → P4 joins P1's group, **and P5's group is cleared** (§6.9a). *Also note what the UI let you do here — see Q-B7.* 21. **Admin Roster Builder merge declines rival invitations.** P3 has invited P6. Admin drags P6 into P1's group in the season Roster Builder → **P3's invitation is declined and P3's group cleared** (§6.9b). Repeat for the "create a new group from two players" drag. 22. **Cancelled member does not keep a group alive.** Group of 2 where the member's registration is `cancelled` but still carries the `group_id` (audit class SB — construct it by hand). Manager deletes their last pending invitation → **group cleared**, because the cancelled row is not live. ### 9.3 Guards and non-events 23. **Non-manager deletes an invitation they sent.** Member of a group of 3 sends an invitation, then deletes it → **nothing cleared**, group intact. *(No manager check exists in §6.1 by design; the live-count test is what refuses. This proves it.)* 24. **Tournament regression.** Captain invites → accept → remove → member leaves → cancel. **`Team.group_id` still authoritative, nothing dissolved, no behaviour change anywhere.** Run the full tournament lifecycle; this change must be invisible to it. 25. **`groups_locked` ON — the player-facing paths.** Invite refused, leave refused, remove refused — as today. No dissolve fires. 26. **`groups_locked` ON — the two ADMIN paths that ignore it. Dissolving is the expected result; assert it rather than treating it as a finding.** `CancelRegistrationForm` has no `groups_locked` guard and neither does `GroupInvitationsForm`. With the lock on: **(a)** an admin cancels a member's registration → §6.13 fires and the group dissolves; **(b)** an admin declines a member row → §6.8 fires, same. **Both are correct.** Both forms are `administrator`-only (`manage seasons` / `generate rosters`, held by no other role), there is **no player-facing cancel route in the module at all**, and `groups_locked` is enforced only on the four player-facing paths in `GroupController` — which is exactly what `GroupDissolveService`'s docblock (**:23-25**) says the convention is. **Q-B9, ANSWERED — see §11 for the full evidence.** An earlier draft of this case described (a) as *"a member cancels"* and flagged the dissolve as a possible regression; that was wrong on the facts. **What to actually check here:** that case 25's player-facing refusals still hold with the lock on, and that these two admin paths still complete — a future "fix" that adds a `groups_locked` guard to either form would break an admin's ability to clean up a group they just emptied, and this case is what should catch it. 27. **Idempotency.** Trigger a dissolve, then trigger the same path again → no error, **no second "Group @gid dissolved" watchdog line**, no `changed` bump on any registration, and §6.1 does **not** re-show *"You are no longer in a group."* *(This case only passes because of §4.2's empty-set early return. It was added specifically to catch that being dropped.)* 28. **Two members leave at once** (two browsers). → At worst the group survives with one member (a *missed* cleanup, which is acceptable); never a corrupted or half-cleared group. Then trigger any other B path → it clears. *Documents the known-benign race in §4.3.* 29. **`Registration.team` unchanged** across every one of cases 1-28. Check before and after. Disbanding a friend group must never un-assign anyone from a season team — CF6's recorded decision, and `GroupDissolveService` already honours it (**:156**). ### 9.4 Read-only SQL **Confirm column names first** — `drush sqlq "DESCRIBE ccsoccer_registration"`. This codebase has been bitten by assumed names. ```sql -- A. BEFORE and AFTER: how many solo season groups exist? -- Run before the deploy to size the backlog; run weekly afterwards. The -- count should trend to zero and then stay there. A rise means a call site -- was missed — see §12. 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, which have NO pending invitation? Those are the genuinely -- stranded ones. A solo group WITH a pending invitation is legitimate — -- someone mid-invite — and the §4.2 carve-out protects it deliberately. -- THIS IS THE REAL METRIC. Run it weekly for a month after the deploy. -- Season-scoped to match §4.2's carve-out exactly: the predicate filters on -- season as well as group_id, and CcsoccerCommands.php:1951 seeds non-UUID -- 'testgroup_N' ids that can collide across seasons on LOCAL. SELECT g.group_id FROM ( SELECT r.group_id, MIN(r.season) AS season 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 WHERE NOT EXISTS ( SELECT 1 FROM ccsoccer_invitation i WHERE i.group_id = g.group_id AND i.season = g.season AND i.status = 'pending' ); -- C. Groups with zero live members but rows still carrying the group_id -- (audit class SC). -- DO NOT expect zero. There is no data repair in this change (§1, §8), so -- pre-existing SC rows for groups nobody touches again will survive -- indefinitely — clearGroupIfOrphaned() only fires when something triggers -- it. The useful reading is "no NEW rows since the deploy": record the count -- before, and check it is not RISING afterwards. SELECT r.group_id, COUNT(*) AS dead_rows FROM ccsoccer_registration r WHERE r.registration_type = 'season' AND r.group_id IS NOT NULL AND r.group_id <> '' GROUP BY r.group_id HAVING SUM(CASE WHEN r.status NOT IN ('cancelled','expired') THEN 1 ELSE 0 END) = 0; -- D. Pending invitations whose group has no live member (audit class IA). -- Should trend to zero: clearGroupIfOrphaned() only fires when there are no -- pending invitations, so any row here means the group emptied while an -- invitation was outstanding — which the carve-out permits. Worth watching, -- not an error. SELECT i.id, i.inviter, i.season, i.group_id FROM ccsoccer_invitation i WHERE i.status = 'pending' AND i.season IS NOT NULL AND i.group_id IS NOT NULL AND i.group_id <> '' AND NOT EXISTS ( SELECT 1 FROM ccsoccer_registration r WHERE r.group_id = i.group_id AND r.status NOT IN ('cancelled','expired') ); ``` **Query B is the one to keep.** Add it to `ROSTER_DATA_AUDIT.sql`. It is how a missed call site gets found, and it is the entire safety net for this design. **It will not reach zero unless §6.9b ships.** The two Roster Builder placement paths never decline the placed player's other pending invitations, so those invitations keep their inviters' solo groups alive permanently via the §4.2 carve-out — with no call site missing. **If query B plateaus above zero, check §6.9b landed before concluding a site was missed.** --- ## 10. CF11 — optional, and now genuinely optional `ROSTER_RECONCILIATION_PLAN.md` §10.3 CF11 specifies a "Disband Group" button for a solo manager. Under this package **route 4 clears itself**, so the button is no longer the fix for anything. **It is still decent insurance for B's one weakness.** If a call site is missed — now or in code written next year — the player unsticks themselves instead of emailing an admin. That is a neat inverse of the failure mode. **Recommendation: do not build it in this change.** Ship CF10 + B + E, run §9.4 query B weekly for a month, and build CF11 only if the count does not settle at zero. If it does settle, the button would be dead code guarding a state that no longer occurs. **If it is built later:** use `GROUP_ID_AT_ACCEPT_BRIEF.md` §9, which is more current than the plan's CF11 brief (whose line numbers are stale and whose `notify => TRUE` is wrong — it should be `FALSE`, there is nobody but the clicker to notify). The **D-7 decision — solo only, not a general manager-dissolve — still stands.** **Related, and now free: D-9.** `manage()` gates invitations-addressed-to-you on `if (!$group_id)` (**:602**), so a stranded solo manager cannot see an invitation already waiting. Under this change they stop being stranded, so **most of D-9 resolves itself** — but a manager who is *legitimately* alone with a pending invitation out still cannot see incoming ones. Widening the gate to *"no group, or manager of a group of one"* is a one-line display change. **Not in this commit** — it is a display gate, not a data one, and the plan's own caution says to check it against the Roster Builder screens first. --- ## 11. Open questions Fewer than option D's twelve, and **none of them blocks starting work** — they can be answered while writing. **Q-B9, the one that had to be answered before *shipping*, is now answered (Aug 5) and the answer is "change nothing"** — so nothing in this section blocks shipping either. **Q-B1 — should the carve-out count *all* pending invitations for the group, or only the manager's?** §4.2. Members can send invitations: `invite()` checks registration ownership (**:875**), not manager-ness — the manager-only rule lives in Twig (`ccsoccer-group-manage.html.twig:107`). Counting all of them is safer and matches `getGroupSize()`'s existing behaviour. **Recommend: all** (as specified). **Q-B2 — should the five silent decline sites notify the inviter?** §4.4, §6.5. `declineInvitation()` notifies; `acceptSeasonInvitation()` **1453** and **1496**, `acceptSeasonInvitationDirectly()` **519**, `OrderCompleteSubscriber` **1040** and `GroupInvitationsForm` **1498** do not. **Today P3 is never told P2 chose someone else**; after this change P3's group also quietly disappears. That is the one user-visible regression here. **Recommend: yes, as a separate small follow-up** — five notification calls inside this commit would change email volume and behaviour at the same time. **Q-B3 — build CF11?** §10. **Recommend: decide after four weeks of §9.4 query B.** **Q-B4 — should a dissolve on route 4 notify the surviving manager?** Cases 13-15: the member left or was removed, and the manager's group silently disappears. For *removal* the manager did it themselves. For *leave* and *cancel* they did not. `sendPlayerRemoved()` / the cancel notifications may already cover it. **Recommend: check what the manager is told today, and change nothing if they already get a message.** **Q-B5 — ANSWERED.** `RegistrationController::acceptSeasonInvitationDirectly()` **478-481** does **not** decline — it warns and returns, leaving the invitation `pending`. So it is not a call site. **The remaining question is whether the two Accept buttons should disagree**: `GroupController` **1450-1456** kills the invitation, this one leaves it retryable. **Recommend: leave it, record it, fix separately.** **Q-B6 — ANSWERED.** §6.8's call goes **after `:1108`** — after the third pass (invitation rows, **1074-1108**), not after the member-row pass (**1001-1072**). A call placed after the member loop misses the "admin declined the last pending invitation" case entirely. Use `$form_state->get('context_id')` for the season; **`$context_id` is not a local in that method.** **Q-B7 — should `GroupInvitationsForm::acceptInvitation()` refuse when the player is already in a group?** §6.9a. Both player-facing accept paths refuse; the admin path silently overwrites, with no confirmation and no way back (registrations are not revisionable). CF2 fixed exactly this shape on the *tournament* side of the same method (**1383-1391**); the season side never got it. **Not in scope here** — §6.9a cleans up after the overwrite rather than preventing it — but it is a real gap and probably belongs in the same cluster as CF2. **Q-B8 — cover the three admin entity-form paths, or rely on the audit query?** §6.10. Deleting a registration, hand-editing its `status`, and deleting an invitation from the admin collection can all shrink a group with no cleanup. **Recommend: rely on §9.4 query B**, and treat a non-zero trend as the trigger to build the entity-hook variant (`D8_OPTIONS_COMPARISON.md` §8). **Q-B9 — should `groups_locked` block a dissolve? — ANSWERED (Andrew + Claude, Aug 5): NO. The premise was wrong.** **The question as originally posed was:** `CancelRegistrationForm` and `GroupInvitationsForm` have no `groups_locked` guard, so after this change *a member cancelling* — or an admin declining a row — dissolves a group after the roster has been locked, which is what the lock exists to prevent. **A member cannot cancel.** Verified in `ccsoccer.routing.yml` and `config/sync`, not from memory: | Form | Route | Permission | Roles holding it | |---|---|---|---| | `CancelRegistrationForm` | `/admin/ccsoccer/registration/{registration}/cancel` | `manage seasons` | **`administrator` only** | | `GroupInvitationsForm` | `/admin/ccsoccer/group/{group_id}/invitations` | `generate rosters` | **`administrator` only** | | `TournamentCancelRegistrationForm` | `/admin/ccsoccer/registration/{registration}/tournament-cancel` | `manage tournaments` | `administrator`, `tournament_director` | **No role in `config/sync` grants `manage seasons` or `generate rosters`** — not `board_member`, not `tournament_director`, not `beta_tester`. Only `administrator`, and only by way of `is_admin: true`. The `CancelRegistrationForm` and `TournamentCancelRegistrationForm` routes also carry `_admin_route: TRUE`. This confirms the plan's Aug 4 finding (`ROSTER_RECONCILIATION_PLAN.md` §10.3, CF1) that **the only two code paths anywhere that write `status = 'cancelled'` to a Registration are the two cancel forms** — re-verified by grep across the module: `CancelRegistrationForm:360` and `TournamentCancelRegistrationForm:281`, and nothing else. *(The plan cites these as `:349` and `:271`; they moved with CF1 and CF8. The claim holds, the coordinates drifted — the usual rule applies.)* **So the real scenario is not "a member cancels after lock". It is "an admin cancels a registration, or edits a member row, after lock" — with the admin on an admin form, having navigated there deliberately.** **And that is the convention the module already follows.** `groups_locked` is enforced in exactly four places, all player-facing, all in `GroupController` — verified by locating each guard's enclosing method, not by line number alone: `invite()` **:885** (method at :864), `acceptSeasonInvitation()` **:1426** (:1420), `removeMember()` **:1719** (:1680), `leaveGroup()` **:1975** (:1947). `SeasonController:446` describes the flag to admins in as many words — *"Players cannot modify groups."* And `GroupDissolveService`'s own class docblock (**:23-25**) already states the rule this question was asking us to invent: > *"Does NOT check `groups_locked`. `dissolveGroupConfirmed()` deliberately does not (it is an admin > path) while `leaveGroup()` and `removeMember()` do. The check stays with the callers, exactly as > before this extraction."* **Therefore: add no guard.** `groups_locked` has always meant *players* cannot modify groups; admin paths have always bypassed it by design. Adding a check to `CancelRegistrationForm` or `GroupInvitationsForm` would be a **new restriction on admins**, not the closing of a gap — and it would leave an admin unable to clean up a group they had just emptied, which is the opposite of what the lock is for. **Record it as "the premise was wrong", not as "we accepted the risk."** The distinction matters: the next reader who finds two dissolve paths with no lock check will otherwise re-open this. **One caveat, cheap to close.** The evidence above is `config/sync`. `SESSION_HANDOFF.md` records unexplained role drift on LOCAL (June 16 — `anonymous`, `board_member`, `tournament_director`, cause never found, reverted via `drush cim`). **Run `drush role:list` on PROD** and confirm no role has picked up `manage seasons` or `generate rosters` in active config. That is the only way this answer is wrong. --- ## 12. The known weakness, stated plainly **B maintains the invariant; it does not guarantee it.** Thirteen call sites means thirteen chances to miss one, and a code path written next year that reduces group membership without calling `clearGroupIfOrphaned()` silently reintroduces the bug. **The first draft of this document missed two of the thirteen and one prerequisite bug** — all three in `GroupInvitationsForm` and `RosterBuilderController`, the admin surfaces. That is the weakness demonstrating itself before a line of code was written, and it is the honest argument for the entity-hook variant. It is also why §9.4 query B is not optional. This was the plan's original objection to option B, and it is fair. Three things make it acceptable: 1. **It cannot *drift*.** There is one implementation of the predicate. What has bitten this codebase is duplicated *logic* — `reset()` written out 21 times (CF4), status filters ten times (CF3), four hand-rolled copies of "clear three fields". Thirteen calls to one function is a different, smaller risk: omission, not divergence. 2. **It is cheap to detect.** §9.4 query B, run weekly, finds a missed site as a count that stops trending to zero. 3. **It fails into the status quo.** A missed site means someone is stranded — today's behaviour, which admins recognise and Dissolve fixes. **Compare option D**, whose failure modes were a manager-less group or a player silently ungrouped after payment: states nobody has seen, nothing detects, and no playbook covers. **That asymmetry is why this option was chosen** (`D8_OPTIONS_COMPARISON.md` §6.1). **If the count in §9.4 query B does not settle at zero within a month, the answer is not to abandon B** — it is to add the missed call site, and to consider the entity-hook variant in `D8_OPTIONS_COMPARISON.md` §8, which collapses thirteen call sites to one at the cost of recursion guards and harder debugging. --- ## 13. Rollback **Code-only. No data step.** This is the sharpest contrast with option D. Everything this change writes is a *clearing* of fields — `group_id`, `invited_by`, `invitation_status` — to the values they hold for any player not in a group. Revert the code and those players are simply not in a group, which is a state the pre-change code handles correctly and has always handled. - **Revert CF10 alone** and invite-time minting returns. Nothing else depends on it. - **Revert B alone** and the cleanup stops happening. Groups already cleared stay cleared, correctly. - **Revert both** and you are exactly where you started. **Nothing needs repairing on the way out.** *(Option D's rollback required backfilling every in-flight invitation — see `GROUP_ID_AT_ACCEPT_BRIEF.md` §13.)* --- ## 14. Branch, commits and deploy - ~~**Branch `fix/D8_group_cleanup` off `main`**~~ — **the actual branch is `fix/solo_group_auto_delete`** (Andrew, Aug 5). The proposed name was not used. **Recorded because getting this wrong has already cost this project once:** `fix/roster_sync` never existed and is still cited in several documents as though it did. - **Three commits** — commits 1 and 2 landed Aug 5 as `dde90b5` and `995e286`; commit 3 is written and uncommitted: | # | Contents | Files | |---|---|---| | 1 | `clearGroupIfOrphaned()` + `clearInviterGroupIfOrphaned()` on `GroupDissolveService`; wire the four new injections | `GroupDissolveService.php`, `GroupController.php`, `OrderCompleteSubscriber.php`, `RosterBuilderController.php`, `ccsoccer.services.yml` | | 2 | **CF10** — move the mint below the checks; the `$group_is_new ? 1 : …` size fix; delete the three stray comment blocks | `GroupController.php` | | 3 | **B + E** — the thirteen call sites | `GroupController.php`, `RegistrationController.php`, `OrderCompleteSubscriber.php`, `GroupInvitationsForm.php`, `CancelRegistrationForm.php`, `RosterBuilderController.php` | Commit 1 is additive and changes no behaviour. Commits 2 and 3 are independent of each other and independently revertable — **keep them separate.** **§6.9b adds a decline that does not exist today**, so commit 3 is not purely additive on the Roster Builder paths. Call it out in the commit message. - **Archive before editing**, per repo rule: `archive/_.php`, and **check the filename is not already taken** — that bit us on July 27. - **`ddev drush cr` after commit 1.** **Three** constructor changes (`GroupController`, `OrderCompleteSubscriber`, `RosterBuilderController`) and a `services.yml` edit; without a rebuild the container is stale and pages will fatal. §6.0 is the authoritative wiring table. - **Deploy LOCAL → TEST → PROD**, re-running §9.2 cases 5, 8 and 13 on TEST before PROD. - **`drush config:status` before and after.** No config changes are expected; anything that appears is drift from something else. --- ## 15. Definition of done **Code:** ✅ all three commits written · **Testing:** ⬜ not started · **Docs:** ✅ done - [ ] **Lint commit 3's six files and `ddev drush cr`** — the immediate next step - [ ] All of §9.2 and §9.3 pass on LOCAL, **including cases 19, 20, 21** (the sites review found) and **24** (tournament regression) - [ ] §9.4 query C's count recorded before, and **not rising** after. *(It will not be zero — there is no data repair.)* - [ ] §9.4 query B's count recorded before, and after a week on PROD - [ ] Query B added to `ROSTER_DATA_AUDIT.sql` — **still outstanding, and it is the only thing that will reveal a missed fourteenth call site** - [ ] `drush config:status` clean - [ ] No new `watchdog` errors across a full register → invite → accept → remove → cancel cycle - [x] **Q-B9 answered before shipping** — **ANSWERED Aug 5: no guard, the premise was wrong** (§11). Remaining: confirm `drush role:list` on PROD matches `config/sync`. The other eight can be answered while writing - [x] The nine Q-B questions in §11 have recorded answers **in this file**, not in a chat log — Q-B5, Q-B6 and Q-B9 answered; the rest carry recommendations - [x] `ROSTER_RECONCILIATION_PLAN.md` §7 **D-8** marked DECIDED, pointing here — **done Aug 5** - [x] **CF10 marked implemented**, and its "superseded by option D" note removed — **done Aug 5**, `995e286`; its brief now points here for the implementation - [ ] `GROUP_ID_AT_ACCEPT_BRIEF.md`'s HELD banner cross-references this file - [x] `SESSION_HANDOFF.md`'s "Andrew and Caleb need to decide" — Decision 1 closed **Aug 5** - [ ] Caleb has reviewed, or has explicitly declined to --- ## 16. Effort | | | |---|---| | Commit 1 — two service methods + four injections | ~1.5 hrs | | Commit 2 — CF10 + the size-check fix | ~0.75 hr | | Commit 3 — four current-user sites + seven one-line third-party sites | ~1.5 hrs | | Commit 3 — §6.8's three-pass placement and §6.9a/§6.9b (the review finds) | ~2 hrs | | LOCAL testing §9 — 29 cases, several needing a constructed multi-player setup | ~3-3.5 hrs | | **Total** | **~9-11 hrs** | **Up from the first draft's ~7.5-9**, because review added two call sites, one prerequisite fix (§6.9b), a fourth injection, and four test cases. **Still roughly a third of option D's ~25-30.** The testing line is not padding: cases 8-12 each need three players and a specific accept order, case 20 needs two separate groups, and case 22 needs a hand-built cancelled row. --- ## Appendix A — cross-references to update when this ships > **[Aug 5] The plan-side cross-references below were done immediately rather than held until the code > ships.** The plan is the document implementers are told to start from, and until these landed its > ▶ START HERE block told them D-8 was still undecided. The ones that genuinely depend on shipped code — > the audit script, and marking CF10 implemented — are still open and marked ⬜. - ✅ `ROSTER_RECONCILIATION_PLAN.md` §7 **D-8** → DECIDED as CF10 + B + E, pointing here - ✅ `ROSTER_RECONCILIATION_PLAN.md` START HERE → D-8 removed from "open decisions blocking work" - ✅ `ROSTER_RECONCILIATION_PLAN.md` §10.3 **CF10** → **no longer superseded**; banner points here. Its own brief's line numbers are stale (CF4 edited the file) — **§5 of this document is authoritative**, and it carries the `getGroupSize()` off-by-one fix that brief does not have. ⬜ Still to do once shipped: mark it **implemented** - ✅ `ROSTER_RECONCILIATION_PLAN.md` §10.3 **CF11** → optional, deferred pending §9.4 query B. `notify` corrected to `FALSE` - ✅ `ROSTER_RECONCILIATION_PLAN.md` §7 **D-9** → mostly resolves itself; one-line display change deferred - ✅ `ROSTER_RECONCILIATION_PLAN.md` §10.4 → live constraints and the commit-8 row unblocked - `ROSTER_RECONCILIATION_PLAN.md` §5 **CF3** → **unaffected.** `getGroupSize()`'s missing status filter is still CF3's to fix; this change does not touch it - `ROSTER_DATA_AUDIT.sql` → add §9.4 query B - `SESSION_HANDOFF.md` → Decision 1 closed - `GROUP_ID_AT_ACCEPT_BRIEF.md` → HELD banner points here - `ARCHITECTURE_DECISIONS.md` → record the invariant: *a season group with one live member and no pending invitations is dissolved automatically* --- ## Appendix B — why the D research still matters `GROUP_ID_AT_ACCEPT_BRIEF.md` is held, not deleted. Three parts of it are accurate regardless of which option ships and are worth keeping to hand: - **§4 and Appendix A** — the full inventory of all 29 `Invitation.group_id` sites, with each labelled Registration vs Invitation vs Team. Verified twice. Useful for any future work in this area. - **§5.7** — `groups_locked` is enforced on one accept path out of four. A real, separate bug, found during that research and unaffected by this change. - **Appendix C** — the adversarial review log, which documents where this part of the codebase is sharp: concurrency, Drupal API semantics (`loadByProperties` with NULL **throws**), and display code. --- ## Appendix C — adversarial review log **This brief was drafted, then reviewed line-by-line against the working tree by an independent reviewer instructed to assume the author was sloppy.** ~35 citations were spot-checked; **no line numbers were wrong** beyond three off-by-one imprecisions. The defects were all in substance. ### Six blockers, all folded in | # | First draft said | Truth | Fixed in | |---|---|---|---| | B1 | `clearGroupIfOrphaned(int, ?string)` | The body used an undeclared `$reason`, and all call sites passed three arguments | §4.2 signature | | B2 | `$context_id` is in scope in `submitForm()` | It is not. The method pulls five values from form state at **:924-929**; `context_id` is not among them | §6.8 | | B3 | Type-hint `InvitationInterface` | **Does not exist.** `Invitation extends ContentEntityBase` and implements no custom interface | §4.5 | | B4 | `submitForm()` has one relevant loop | **It has three.** The third (**1074-1108**) writes invitation statuses — an admin declining a solo manager's last pending invitation. A call after the member loop misses it entirely | §6.8, case 19 | | B5 | `GroupInvitationsForm::acceptInvitation()` is not a call site | **It overwrites `group_id` with no "already in a group" guard** (**1321-1328**), unlike both player-facing paths. An admin accept can silently empty another group | §6.9a, case 20 | | B6 | The method is idempotent | Not as written. With zero registrations it fell through to `dissolveGroup()`, which has no empty-set exit and logs unconditionally (**:203**) — so repeat calls returned TRUE and re-logged | §4.2 early return, case 27 | ### Nine majors **The one that matters most: M6 — `RosterBuilderController::mergeToGroup()`/`createGroup()` never decline the placed player's other pending invitations**, unlike all four accept paths. Those orphaned invitations keep their inviters' solo groups alive forever via §4.2's carve-out, with **no call site missing** — so §9.4 query B would have plateaued above zero and §12 would have diagnosed it wrongly. **The design's only health metric was broken before it shipped.** Now §6.9b. The rest: the three admin entity-form paths are uncovered and there is no `hook_entity_delete` anywhere (§6.10); `getGroupSize()` returns **1** today and **0** under CF10, so the size check needed a line (§5 step 2); `groups_locked` is not enforced on the two new dissolve paths (§9.3 case 26, **Q-B9 — since ANSWERED: both paths are admin-only and the lack of a guard is the module's deliberate convention, not a gap**); query C can never return zero given no data repair (§9.4, §15); `clearInviterGroupIfOrphaned()` should read the invitation's own `group_id` rather than round-tripping through the inviter's registration — simpler, one fewer query per loop iteration, and it does not silently no-op when the inviter has re-registered (§4.5); and §6.3's "twin" claim about `RegistrationController` was false — **it does not decline**, so it is not a call site (Q-B5, now answered). ### Twelve minors and nits Corrected in place. Notable: the current-user/third-party split was **4/9**, not the 7/4 the draft claimed — and §4.4 used the wrong number to justify `notify => FALSE`; there are **five** silent decline sites, not four; §6.1 duplicated a query that already exists eight lines below; `declineInvitation()` has no `pending` guard so it can re-fire; the stray paste artifact in `invite()` is three blocks, not one; query B was not season-scoped while the predicate is. ### What the review confirmed **On the axis of `group_id = NULL` writes, §6 was already complete** — all five season sites were covered. Every gap was on a different axis: status writes, invitation writes, and entity deletes. Also clean: no direct SQL writes anywhere; Drush commands only create membership; `ccsoccer.install`'s `group_id` work is tournament-only; `TeamBalancerService` never writes `group_id`; and `mergeToGroup()`/`createGroup()` cannot orphan a *source* group because both refuse a player who already holds one (**319**, **534**, **541**). ### The pattern worth noting **Every missed site was in the admin surfaces** — `GroupInvitationsForm` and `RosterBuilderController` — and every one of them was a place where admin code does something the player-facing equivalent refuses to do, or skips something the player-facing equivalent always does. That is where a fourteenth site would be, and it is where implementation review time is worth spending.