From d18b393ccb0f9f3f2f1d95598a20e51beed8ebb6 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Fri, 26 Sep 2025 20:04:51 +0100 Subject: [PATCH] Avoid unnecessary data loss on invalid items in inventories we were already doing this in some places but not others. So inconsistent... while this doesn't fix the root cause of #5128, it should nonetheless fix #5128 as well as not destroying player data on unknown items, which was a frequent annoyance when switching between branches during PR testing. --- src/block/tile/Campfire.php | 3 ++- src/block/tile/ChiseledBookshelf.php | 14 ++++---------- src/block/tile/ContainerTrait.php | 11 +++-------- src/block/tile/ItemFrame.php | 2 +- src/block/tile/Jukebox.php | 2 +- src/block/tile/Lectern.php | 2 +- src/entity/Human.php | 13 ++++++++----- src/item/Item.php | 18 ++++++++++++++++++ 8 files changed, 38 insertions(+), 27 deletions(-) diff --git a/src/block/tile/Campfire.php b/src/block/tile/Campfire.php index ad4a193d7..0c806c5d2 100644 --- a/src/block/tile/Campfire.php +++ b/src/block/tile/Campfire.php @@ -93,6 +93,7 @@ class Campfire extends Spawnable implements Container{ $listeners = $this->inventory->getListeners()->toArray(); $this->inventory->getListeners()->remove(...$listeners); //prevent any events being fired by initialization + $baseErrorContext = "Campfire ($this->position)"; foreach([ [0, self::TAG_FIRST_INPUT_ITEM, self::TAG_FIRST_COOKING_TIME], [1, self::TAG_SECOND_INPUT_ITEM, self::TAG_SECOND_COOKING_TIME], @@ -100,7 +101,7 @@ class Campfire extends Spawnable implements Container{ [3, self::TAG_FOURTH_INPUT_ITEM, self::TAG_FOURTH_COOKING_TIME], ] as [$slot, $itemTag, $cookingTimeTag]){ if(($tag = $nbt->getTag($itemTag)) instanceof CompoundTag){ - $items[$slot] = Item::nbtDeserialize($tag); + $items[$slot] = Item::safeNbtDeserialize($tag, "$baseErrorContext slot $slot"); } if(($tag = $nbt->getTag($cookingTimeTag)) instanceof IntTag){ $this->cookingTimes[$slot] = $tag->getValue(); diff --git a/src/block/tile/ChiseledBookshelf.php b/src/block/tile/ChiseledBookshelf.php index 90bf8f29b..0fea0767d 100644 --- a/src/block/tile/ChiseledBookshelf.php +++ b/src/block/tile/ChiseledBookshelf.php @@ -26,7 +26,6 @@ namespace pocketmine\block\tile; use pocketmine\block\utils\ChiseledBookshelfSlot; use pocketmine\data\bedrock\item\SavedItemData; use pocketmine\data\bedrock\item\SavedItemStackData; -use pocketmine\data\SavedDataLoadingException; use pocketmine\inventory\SimpleInventory; use pocketmine\item\Item; use pocketmine\math\Vector3; @@ -99,18 +98,13 @@ class ChiseledBookshelf extends Tile implements Container{ $inventory->getListeners()->remove(...$listeners); //prevent any events being fired by initialization $newContents = []; + $errorLogContext = "ChiseledBookshelf ($this->position)"; foreach($inventoryTag as $slot => $itemNBT){ - try{ - $count = $itemNBT->getByte(SavedItemStackData::TAG_COUNT); - if($count === 0){ - continue; - } - $newContents[$slot] = Item::nbtDeserialize($itemNBT); - }catch(SavedDataLoadingException $e){ - //TODO: not the best solution - \GlobalLogger::get()->logException($e); + $count = $itemNBT->getByte(SavedItemStackData::TAG_COUNT); + if($count === 0){ continue; } + $newContents[$slot] = Item::safeNbtDeserialize($itemNBT, "$errorLogContext slot $slot"); } $inventory->setContents($newContents); diff --git a/src/block/tile/ContainerTrait.php b/src/block/tile/ContainerTrait.php index 6b9158d7a..72a0dc2e6 100644 --- a/src/block/tile/ContainerTrait.php +++ b/src/block/tile/ContainerTrait.php @@ -24,7 +24,6 @@ declare(strict_types=1); namespace pocketmine\block\tile; use pocketmine\data\bedrock\item\SavedItemStackData; -use pocketmine\data\SavedDataLoadingException; use pocketmine\inventory\Inventory; use pocketmine\item\Item; use pocketmine\nbt\NBT; @@ -56,14 +55,10 @@ trait ContainerTrait{ $inventory->getListeners()->remove(...$listeners); //prevent any events being fired by initialization $newContents = []; + $errorLogContext = "Container (" . $this->getPosition() . ")"; foreach($inventoryTag as $itemNBT){ - try{ - $newContents[$itemNBT->getByte(SavedItemStackData::TAG_SLOT)] = Item::nbtDeserialize($itemNBT); - }catch(SavedDataLoadingException $e){ - //TODO: not the best solution - \GlobalLogger::get()->logException($e); - continue; - } + $slotId = $itemNBT->getByte(SavedItemStackData::TAG_SLOT); + $newContents[$slotId] = Item::safeNbtDeserialize($itemNBT, "$errorLogContext slot $slotId"); } $inventory->setContents($newContents); diff --git a/src/block/tile/ItemFrame.php b/src/block/tile/ItemFrame.php index 7d003770e..d69f533e3 100644 --- a/src/block/tile/ItemFrame.php +++ b/src/block/tile/ItemFrame.php @@ -51,7 +51,7 @@ class ItemFrame extends Spawnable{ public function readSaveData(CompoundTag $nbt) : void{ if(($itemTag = $nbt->getCompoundTag(self::TAG_ITEM)) !== null){ - $this->item = Item::nbtDeserialize($itemTag); + $this->item = Item::safeNbtDeserialize($itemTag, "ItemFrame ($this->position) framed item"); } if($nbt->getTag(self::TAG_ITEM_ROTATION) instanceof FloatTag){ $this->itemRotation = (int) ($nbt->getFloat(self::TAG_ITEM_ROTATION, $this->itemRotation * 45) / 45); diff --git a/src/block/tile/Jukebox.php b/src/block/tile/Jukebox.php index 54acd60ee..cc4448c32 100644 --- a/src/block/tile/Jukebox.php +++ b/src/block/tile/Jukebox.php @@ -44,7 +44,7 @@ class Jukebox extends Spawnable{ public function readSaveData(CompoundTag $nbt) : void{ if(($tag = $nbt->getCompoundTag(self::TAG_RECORD)) !== null){ - $record = Item::nbtDeserialize($tag); + $record = Item::safeNbtDeserialize($tag, "Jukebox ($this->position) record"); if($record instanceof Record){ $this->record = $record; } diff --git a/src/block/tile/Lectern.php b/src/block/tile/Lectern.php index 37e79b10e..2ce586252 100644 --- a/src/block/tile/Lectern.php +++ b/src/block/tile/Lectern.php @@ -45,7 +45,7 @@ class Lectern extends Spawnable{ public function readSaveData(CompoundTag $nbt) : void{ $this->viewedPage = $nbt->getInt(self::TAG_PAGE, 0); if(($itemTag = $nbt->getCompoundTag(self::TAG_BOOK)) !== null){ - $book = Item::nbtDeserialize($itemTag); + $book = Item::safeNbtDeserialize($itemTag, "Lectern ($this->position) book"); if($book instanceof WritableBookBase && !$book->isNull()){ $this->book = $book; } diff --git a/src/entity/Human.php b/src/entity/Human.php index 97ebdefca..132f971ee 100644 --- a/src/entity/Human.php +++ b/src/entity/Human.php @@ -309,9 +309,11 @@ class Human extends Living implements ProjectileSource, InventoryHolder{ if($slot >= 0 && $slot < 9){ //Hotbar //Old hotbar saving stuff, ignore it }elseif($slot >= 100 && $slot < 104){ //Armor - $armorInventoryItems[$slot - 100] = Item::nbtDeserialize($item); + $armorSlot = $slot - 100; + $armorInventoryItems[$armorSlot] = Item::safeNbtDeserialize($item, "Human armor slot $armorSlot"); }elseif($slot >= 9 && $slot < $this->inventory->getSize() + 9){ - $inventoryItems[$slot - 9] = Item::nbtDeserialize($item); + $inventorySlot = $slot - 9; + $inventoryItems[$inventorySlot] = Item::safeNbtDeserialize($item, "Human inventory slot $inventorySlot"); } } @@ -320,7 +322,7 @@ class Human extends Living implements ProjectileSource, InventoryHolder{ } $offHand = $nbt->getCompoundTag(self::TAG_OFF_HAND_ITEM); if($offHand !== null){ - $this->offHandInventory->setItem(0, Item::nbtDeserialize($offHand)); + $this->offHandInventory->setItem(0, Item::safeNbtDeserialize($offHand, "Human off-hand item")); } $this->offHandInventory->getListeners()->add(CallbackInventoryListener::onAnyChange(fn() => NetworkBroadcastUtils::broadcastEntityEvent( $this->getViewers(), @@ -331,8 +333,9 @@ class Human extends Living implements ProjectileSource, InventoryHolder{ if($enderChestInventoryTag !== null){ $enderChestInventoryItems = []; - foreach($enderChestInventoryTag as $i => $item){ - $enderChestInventoryItems[$item->getByte(SavedItemStackData::TAG_SLOT)] = Item::nbtDeserialize($item); + foreach($enderChestInventoryTag as $item){ + $slot = $item->getByte(SavedItemStackData::TAG_SLOT); + $enderChestInventoryItems[$slot] = Item::safeNbtDeserialize($item, "Human ender chest slot $slot"); } self::populateInventoryFromListTag($this->enderInventory, $enderChestInventoryItems); } diff --git a/src/item/Item.php b/src/item/Item.php index e7c86e167..6f7e010c9 100644 --- a/src/item/Item.php +++ b/src/item/Item.php @@ -775,6 +775,24 @@ class Item implements \JsonSerializable{ } } + /** + * Same as nbtDeserialize(), but purposely suppresses data errors and returns AIR if deserialization fails. + * An error will be logged to the global logger if this happens. + * + * @param string $errorLogContext Used in log messages if deserialization fails to aid debugging (e.g. inventory owner, slot number, etc.) + */ + public static function safeNbtDeserialize(CompoundTag $tag, string $errorLogContext, ?\Logger $logger = null) : Item{ + try{ + return self::nbtDeserialize($tag); + }catch(SavedDataLoadingException $e){ + //TODO: what if the intention was to suppress logging? + $logger ??= \GlobalLogger::get(); + $logger->error("$errorLogContext: Error deserializing item (item will be replaced by AIR): " . $e->getMessage()); + //no trace here, otherwise things could get very noisy + return VanillaItems::AIR(); + } + } + public function __clone(){ $this->nbt = clone $this->nbt; if($this->blockEntityTag !== null){