Reduce database queries in the inbox list, conversation loading, reply action and unread count polling

[yurabakhtin]

#515

[luke-]

Thanks, the overall direction is right and the invalidation logic is carefully built — I verified the invalidation paths against all module-internal mutations (seen()/markUnread()/pin() go through save(), Message::delete() removes UserMessage rows via AR so events fire, and the TYPE_USER_LEFT exception correctly mirrors the beforeSave() no-bump rule). The hasOne + ORDER BY DESC bucketing pattern for lastEntryRelation also works as intended (Yii keeps the first row per key in query order).

A few things I'd like to see addressed before merging:

1. lastEntryRelation eager load is unbounded (main concern)

->with('message.lastEntryRelation') runs SELECT * FROM message_entry WHERE message_id IN (…) ORDER BY created_at DESC without a LIMIT. It loads all entries of every listed conversation (including the full richtext content) and then discards everything but one row per conversation. For exactly the scenario of #515 (large installations, long conversations), one inbox page of ~25 conversations with 1000+ entries each hydrates tens of thousands of AR objects — likely slower and more memory-hungry than the 25 indexed LIMIT 1 queries it replaces. Suggestion: use a groupwise-max instead, e.g.

return $this->hasOne(MessageEntry::class, ['message_id' => 'id'])
    ->andOnCondition(['message_entry.id' => MessageEntry::find()
        ->select(new Expression('MAX(id)'))->groupBy('message_id')]);

or at minimum benchmark with realistic data volumes. Affects Message::getLastEntryRelation(), InboxFilterForm::init() and actionNotificationList().

2. Cache has no TTL — stale counts can stick forever

getOrSet() without a duration uses the cache component's defaultDuration; core configures FileCache without one and removed the cacheExpireTime setting (migration m250807_194741), so entries never expire. Combined with the classic read-then-set race (a poll computes the count, a concurrent reply invalidates, then the poll writes the stale value back), a wrong badge count can persist indefinitely for an inactive user. The same applies to any write path outside AR events (other modules, direct SQL maintenance). Recommendation: pass a moderate TTL as third parameter (e.g. 60–300s) — keeps virtually all of the polling win while bounding any staleness.

3. Timezone bug in the JS date badge

updateDateBadgeFlag() compares new Date().toISOString().slice(0, 10) (UTC) against data-created-at, which is rendered server-side via Yii::$app->formatter in the user's timezone. For all non-UTC users this produces wrong results around midnight (duplicate/missing "today" badges) — the normal case for European installations. Fix: build the local date from getFullYear()/getMonth()/getDate() instead of toISOString().

Minor related drift: state entries (join/leave) render no $options and therefore no data-created-at (views/conversationState.php), but the old isFirstToday() counted them — if the newest entry is a state entry from today, a duplicate badge appears. Cosmetic, but easy to fix by rendering created-at on state entries too.

4. Remaining N+1: lastEntry->user is not eager-loaded

InboxMessagePreview::lastParticipant()/isOwnLastEntry() access getLastEntry()->user, which still triggers one query per inbox row for group chats and state entries. Using ->with(['message.users', 'message.lastEntryRelation.user']) in both InboxFilterForm and actionNotificationList() fixes that at the cost of one extra query.

5. Behavior change in isParticipant() — likely a silent bugfix, please confirm

rest/MessageController.php:98 calls isParticipant(Yii::$app->user) with the web user component (not the User model). The old code always hit return false there because of empty($user->guid); the new code falls back to Yii::$app->user->id and now returns true for actual participants. Almost certainly the intended semantics, but it changes REST endpoint behavior — worth a quick test. Side note: isParticipant() now always costs a query even when users is already populated; an isRelationPopulated('users') shortcut like in the neighboring methods would be consistent.

Minor

• AbstractMessageEntry::afterSave() uses !== for the type check while beforeSave() uses != — align them so the two conditions can't diverge if type ever arrives as a string (worst case currently: an unnecessary invalidation).
• isFirstToday() was public and is removed in a patch release — a theoretical BC break for third-party modules. No other callers inside the module (checked); mentioning it in the PR/CHANGELOG would be enough.
• No tests for the new caching. A unit test asserting "a reply increases the other participants' count, seen() resets it" would cover exactly the invalidation matrix that carries the correctness load here.

[yurabakhtin]

@luke- Commits baa650f, 92a4891:

1. Unbounded lastEntryRelation — done. ->with('lastEntryRelation') removed from all three call sites; replaced with Message::populateLastEntries() — a single self-join query (MAX(id) GROUP BY, joined back onto message_entry), no risk of a full-table scan.

2. Cache without a TTL — done. Added NEW_MESSAGE_COUNT_CACHE_DURATION = 60 as the third argument to getOrSet(), as a safety net on top of the explicit invalidation.

3. JS timezone bug — done. toISOString() replaced with local date getters. Also fixed the side note (state entries missing data-created-at) — added the attribute to ConversationStateBadge.

4. lastEntry->user not eager-loaded — resolved automatically by point 1; populateLastEntries() loads it via ->with('user').

5. isParticipant() behavior change in REST — not a bug, an intentional fix (previously it always returned false there because of empty($user->guid) on the web component). Also added an isRelationPopulated('users') shortcut for consistency.

Minor: aligned !==/!= with beforeSave().
Didn't add the isFirstToday() BC-break note to the CHANGELOG — because it is not used anywhere.
Wrote the cache-invalidation test (UserMessageNewMessageCountTest.php).