[2.x] fix(messages): repair dialogs left pointing at a deleted first or last message

[karl-bullock]

Fixes #4848.

Why

Deleting the first or last message of a conversation can leave the dialog with a null first_message_id or last_message_id. After that the conversation does not render at all: MessageStream reads straight through the missing relationship and throws Cannot read properties of null (reading 'id'), which is the error in the issue. The reply box is part of the same stream, so the one thing that repairs the pointer, posting another message, is out of reach. That is the "infinite circle" the reporter describes, and why they had to repair the row by hand.

Both columns are declared with nullOnDelete(), so the database sets the column to null as part of the delete. The repair in DialogMessage's deleted handler then compares the dialog's first_message_id against the id of the message that was just removed, but by then that column can already be null, the comparison misses, and no repair runs.

It only works when the dialog happens to be in memory before the delete, which is why this looks intermittent. DialogMessagePolicy::delete() is the only thing on that path that touches $message->dialog, and only in the 'reply' branch, so PHP short-circuits past it for the other values of allow_delete_own_messages. On a bench, deleting the same message from the same dialog:

allow_delete_own_messages	result
-1 (indefinitely)	last_message_id set to null
reply	last_message_id correctly moved back one message

What

• DialogMessage::boot() treats a pointer that is missing as one that needs recomputing, rather than only matching it against the deleted id, and uses two conditions instead of an if/elseif chain so both ends can be repaired in one pass. Dialogs that are already in this state are repaired the next time a message in them is deleted.
• DialogMessageResource::created() set first_message_id to the newly created message whenever it was missing. For a new dialog that is the same thing, but for a dialog whose pointer had gone missing it recorded the newest message as the first one, leaving the stream with a "load previous messages" button that has nothing to load. It now starts from the oldest surviving message.
• The frontend read these relationships in three places without allowing for their absence (MessageStream.content, MessageStream.timeGap, and the mark-as-read button in DialogListItem), so a dialog in this state took the whole page down. They now go through Dialog.messageRelationshipId(), which returns undefined when the relationship is not there. With no first message there is nothing older to load, with no last message nothing newer, and marking as read is skipped rather than sending a request built from null. This matters for forums that are already affected, since their data is only repaired on the next deletion.

Verification

extensions/messages integration suite: 18 passing. The four new tests fail against current 2.x and pass with this change. The middle-message and only-message cases pass either way and are there to pin behaviour that should not change.

PHPStan level 6 clean on extensions/messages, check-typings clean, prettier clean. Source only, since the bot builds dist at merge.

Also checked by hand on 2.0.0-rc.5 with the frontend built from this branch: a dialog whose first message was deleted renders normally instead of throwing, a healthy dialog is unchanged (start-of-conversation gap present, no spurious load buttons), and messageRelationshipId returns the same ids the previous code read, with mark-as-read still saving.

[imorland]

Nice diagnosis — the 'reply'-branch short-circuit explanation for the intermittency is spot on, and matches what I traced through the policy.

One thing I'd like to resolve before this goes in: the switch from strict === to loose == in the deleted handler.

if ($dialog->first_message_id === null || $dialog->first_message_id == $message->id) {

This works, but it papers over the underlying cause rather than fixing it. The reason === was unreliable is that Dialog doesn't cast its id columns, so first_message_id comes back as a driver-dependent type (string under some PDO configs, int under others) while $message->id is an int — so strict comparison can miss.

Core's Discussion model casts exactly these columns:

// framework/core/src/Discussion/Discussion.php
protected $casts = [
    'first_post_id' => 'integer',
    'last_post_id'  => 'integer',
    // ...
];

Dialog has no equivalent. Could we add 'first_message_id' => 'integer' and 'last_message_id' => 'integer' to Dialog::$casts and keep ===? That fixes the type mismatch at its source, brings the model in line with core's equivalent, and makes the comparison mean what it says. If there's a reason == is preferable here, could you note it in the PR description — right now the change from === isn't mentioned, so it's easy to miss on review.