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

@@ -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();

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);
+				$newContents[$slot] = Item::safeNbtDeserialize($itemNBT, "$errorLogContext slot $slot");
-				}catch(SavedDataLoadingException $e){
-					//TODO: not the best solution
-					\GlobalLogger::get()->logException($e);
-					continue;
-				}
 			}
 			$inventory->setContents($newContents);
 

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{
+				$slotId = $itemNBT->getByte(SavedItemStackData::TAG_SLOT);
-					$newContents[$itemNBT->getByte(SavedItemStackData::TAG_SLOT)] = Item::nbtDeserialize($itemNBT);
+				$newContents[$slotId] = Item::safeNbtDeserialize($itemNBT, "$errorLogContext slot $slotId");
-				}catch(SavedDataLoadingException $e){
-					//TODO: not the best solution
-					\GlobalLogger::get()->logException($e);
-					continue;
-				}
 			}
 			$inventory->setContents($newContents);
 

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);

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;
 			}

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;
 			}

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){
+			foreach($enderChestInventoryTag as $item){
-				$enderChestInventoryItems[$item->getByte(SavedItemStackData::TAG_SLOT)] = Item::nbtDeserialize($item);
+				$slot = $item->getByte(SavedItemStackData::TAG_SLOT);
+				$enderChestInventoryItems[$slot] = Item::safeNbtDeserialize($item, "Human ender chest slot $slot");
 			}
 			self::populateInventoryFromListTag($this->enderInventory, $enderChestInventoryItems);
 		}

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){