fix(Scripts/Commands): stop .mail return sending empty mails for online players (#26485)

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

src/server/scripts/Commands/cs_mail.cpp

@@ -243,74 +243,19 @@ public:
 
         Player* player = target.GetConnectedPlayer();
 
-        // Run the same script hook as the client return handler, failing early before any deletions
-        if (player)
-        {
-            Mail* m = player->GetMail(mailId);
-            if (m)
-            {
-                ObjectGuid senderGuid = ObjectGuid(HighGuid::Player, sender);
-
-                if (m->HasItems())
-                {
-                    for (auto const& itemInfo : m->items)
-                    {
-                        Item* item = player->GetMItem(itemInfo.item_guid);
-                        if (item && !sScriptMgr->OnPlayerCanSendMail(player, senderGuid, ObjectGuid::Empty, subject, body, money, 0, item))
-                        {
-                            handler->SendErrorMessage(LANG_MAIL_RETURN_HOOK_BLOCKED);
-                            return true;
-                        }
-                    }
-                }
-                else if (!sScriptMgr->OnPlayerCanSendMail(player, senderGuid, ObjectGuid::Empty, subject, body, money, 0, nullptr))
-                {
-                    handler->SendErrorMessage(LANG_MAIL_RETURN_HOOK_BLOCKED);
-                    return true;
-                }
-            }
-        }
-
-        // Same logic as WorldSession::HandleReturnToSender
         CharacterDatabaseTransaction trans = CharacterDatabase.BeginTransaction();
 
-        CharacterDatabasePreparedStatement* stmt = CharacterDatabase.GetPreparedStatement(CHAR_DEL_MAIL_BY_ID);
+        // Collect attachments before the script-hook check and any deletion, so a hook
-        stmt->SetData(0, mailId);
+        // veto can still abort the command with nothing changed.
-        trans->Append(stmt);
+        // mail_items is flushed to the DB on every item take, so it is authoritative even
+        // for online players; mails the session never loaded (expired at login, delivered
+        // mid-login, inserted externally) are returned with their items too. Same query
+        // shape as CHAR_SEL_MAILITEMS (LEFT JOIN to handle dangling mail_items) and same
+        // logic as Player::_LoadMailedItem. Items the session already has loaded are
+        // reused and stay owned by the session; the bool marks objects loaded (and owned)
+        // here.
+        std::vector<std::pair<Item*, bool /*loadedFromDb*/>> attachments;
 
-        stmt = CharacterDatabase.GetPreparedStatement(CHAR_DEL_MAIL_ITEM_BY_ID);
-        stmt->SetData(0, mailId);
-        trans->Append(stmt);
-
-        MailDraft draft(subject, body);
-        if (mailTemplate)
-            draft = MailDraft(mailTemplate, false);
-
-        if (player)
-        {
-            // Online: same logic as WorldSession::HandleReturnToSender
-            // Get pointer before RemoveMail (which removes from deque but does not delete the object)
-            Mail* m = player->GetMail(mailId);
-
-            player->RemoveMail(mailId);
-
-            if (m && m->HasItems())
-            {
-                for (auto const& itemInfo : m->items)
-                {
-                    if (Item* item = player->GetMItem(itemInfo.item_guid))
-                        draft.AddItem(item);
-
-                    player->RemoveMItem(itemInfo.item_guid);
-                }
-            }
-
-            delete m;
-        }
-        else
-        {
-            // Offline: load Item* objects from DB using same query shape as CHAR_SEL_MAILITEMS
-            // (LEFT JOIN to handle dangling mail_items) and same logic as Player::_LoadMailedItem
         QueryResult itemResult = CharacterDatabase.Query(
             "SELECT creatorGuid, giftCreatorGuid, count, duration, charges, flags, enchantments,"
             " randomPropertyId, durability, playedTime, text, mi.item_guid, itemEntry, ii.owner_guid"
@@ -325,6 +270,13 @@ public:
                 uint32 itemGuid  = itemFields[11].Get<uint32>();
                 uint32 itemEntry = itemFields[12].Get<uint32>();
 
+                // Prefer the item object the session already has loaded over creating a duplicate
+                if (Item* item = player ? player->GetMItem(itemGuid) : nullptr)
+                {
+                    attachments.emplace_back(item, false);
+                    continue;
+                }
+
                 // Handle dangling mail_items (missing item_instance)
                 if (!itemEntry)
                 {
@@ -344,6 +296,9 @@ public:
                     CharacterDatabasePreparedStatement* delStmt = CharacterDatabase.GetPreparedStatement(CHAR_DEL_INVALID_MAIL_ITEM);
                     delStmt->SetData(0, itemGuid);
                     trans->Append(delStmt);
+
+                    // The mail is going away, so drop the unloadable item_instance row too
+                    Item::DeleteFromDB(trans, itemGuid);
                     continue;
                 }
 
@@ -360,15 +315,74 @@ public:
                     delStmt->SetData(0, itemGuid);
                     trans->Append(delStmt);
 
+                    // Queue the row cleanup on the shared transaction instead of executing it
+                    // immediately; SaveToDB in ITEM_REMOVED state also frees the object
                     item->FSetState(ITEM_REMOVED);
-                        CharacterDatabaseTransaction nullTrans = CharacterDatabaseTransaction(nullptr);
+                    item->SaveToDB(trans);
-                        item->SaveToDB(nullTrans);
+                    continue;
-                        return true;
                 }
 
-                    draft.AddItem(item);
+                attachments.emplace_back(item, true);
             } while (itemResult->NextRow());
         }
+
+        // Run the same script hook as the client return handler for online targets,
+        // covering DB-loaded attachments as well, before any deletion
+        if (player)
+        {
+            ObjectGuid senderGuid = ObjectGuid(HighGuid::Player, sender);
+            bool blocked = false;
+
+            if (attachments.empty())
+                blocked = !sScriptMgr->OnPlayerCanSendMail(player, senderGuid, ObjectGuid::Empty, subject, body, money, 0, nullptr);
+
+            for (auto const& [item, loadedFromDb] : attachments)
+            {
+                if (blocked)
+                    break;
+
+                blocked = !sScriptMgr->OnPlayerCanSendMail(player, senderGuid, ObjectGuid::Empty, subject, body, money, 0, item);
+            }
+
+            if (blocked)
+            {
+                for (auto const& [item, loadedFromDb] : attachments)
+                    if (loadedFromDb)
+                        delete item;
+
+                handler->SendErrorMessage(LANG_MAIL_RETURN_HOOK_BLOCKED);
+                return true;
+            }
+        }
+
+        // Same logic as WorldSession::HandleReturnToSender
+        CharacterDatabasePreparedStatement* stmt = CharacterDatabase.GetPreparedStatement(CHAR_DEL_MAIL_BY_ID);
+        stmt->SetData(0, mailId);
+        trans->Append(stmt);
+
+        stmt = CharacterDatabase.GetPreparedStatement(CHAR_DEL_MAIL_ITEM_BY_ID);
+        stmt->SetData(0, mailId);
+        trans->Append(stmt);
+
+        MailDraft draft(subject, body);
+        if (mailTemplate)
+            draft = MailDraft(mailTemplate, false);
+
+        if (player)
+        {
+            // Drop the mail from the session's loaded state if present.
+            // RemoveMail only erases from the deque, it does not delete the object.
+            Mail* m = player->GetMail(mailId);
+            player->RemoveMail(mailId);
+            delete m;
+        }
+
+        for (auto const& [item, loadedFromDb] : attachments)
+        {
+            if (!loadedFromDb)
+                player->RemoveMItem(item->GetGUID().GetCounter());
+
+            draft.AddItem(item);
         }
 
         uint32 accountId = sCharacterCache->GetCharacterAccountIdByGuid(ObjectGuid(HighGuid::Player, receiver));