From 5afeeb8f89238cdd6a88a9b367d0b3269c9ec5a9 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 20 May 2023 18:55:36 +0100 Subject: [PATCH] Remove nonsensical code from block and item serializers At the time when I wrote this code, I was still in the head space of the kind of ID hijacking that PM4 plugins do to override built-in blocks. However, this kind of internal ID reuse makes no sense in PM5, since said IDs are only used in the core itself at runtime to identify types and states. Even if we were to allow overriding block deserializers, overriding serializers makes no sense whatsoever, since the original block would continue to exist and be accessible. There is a case to be made to allow overriding the deserializer, but not for the serializer. --- .../convert/BlockObjectToStateSerializer.php | 22 ++++----- src/data/bedrock/item/ItemSerializer.php | 45 ++++++++----------- 2 files changed, 26 insertions(+), 41 deletions(-) diff --git a/src/data/bedrock/block/convert/BlockObjectToStateSerializer.php b/src/data/bedrock/block/convert/BlockObjectToStateSerializer.php index fabb525d5..3680e4806 100644 --- a/src/data/bedrock/block/convert/BlockObjectToStateSerializer.php +++ b/src/data/bedrock/block/convert/BlockObjectToStateSerializer.php @@ -169,7 +169,6 @@ use pocketmine\data\bedrock\block\convert\BlockStateWriter as Writer; use pocketmine\math\Axis; use pocketmine\math\Facing; use pocketmine\utils\AssumptionFailedError; -use function class_parents; use function get_class; final class BlockObjectToStateSerializer implements BlockStateSerializer{ @@ -177,8 +176,8 @@ final class BlockObjectToStateSerializer implements BlockStateSerializer{ * These callables actually accept Block, but for the sake of type completeness, it has to be never, since we can't * describe the bottom type of a type hierarchy only containing Block. * - * @var \Closure[][] - * @phpstan-var array> + * @var \Closure[] + * @phpstan-var array */ private array $serializers = []; @@ -212,7 +211,7 @@ final class BlockObjectToStateSerializer implements BlockStateSerializer{ if(isset($this->serializers[$block->getTypeId()])){ throw new \InvalidArgumentException("Block type ID " . $block->getTypeId() . " already has a serializer registered"); } - $this->serializers[$block->getTypeId()][get_class($block)] = $serializer; + $this->serializers[$block->getTypeId()] = $serializer; } public function mapSimple(Block $block, string $id) : void{ @@ -240,21 +239,16 @@ final class BlockObjectToStateSerializer implements BlockStateSerializer{ public function serializeBlock(Block $blockState) : BlockStateData{ $typeId = $blockState->getTypeId(); - $locatedSerializer = $this->serializers[$typeId][get_class($blockState)] ?? null; - if($locatedSerializer === null){ - foreach(class_parents($blockState) as $parent){ - if(isset($this->serializers[$typeId][$parent])){ - $locatedSerializer = $this->serializers[$typeId][$parent]; - break; - } - } - } - + $locatedSerializer = $this->serializers[$typeId] ?? null; if($locatedSerializer === null){ throw new BlockStateSerializeException("No serializer registered for " . get_class($blockState) . " with type ID $typeId"); } /** + * TODO: there is no guarantee that this type actually matches that of $blockState - a plugin may have stolen + * the type ID of the block (which never makes sense, even in a world where overriding block types is a thing). + * In the future we'll need some way to guarantee that type IDs are never reused (perhaps spl_object_id()?) + * * @var \Closure $serializer * @phpstan-var \Closure(TBlockType) : Writer $serializer */ diff --git a/src/data/bedrock/item/ItemSerializer.php b/src/data/bedrock/item/ItemSerializer.php index 0b153c3da..478f9873d 100644 --- a/src/data/bedrock/item/ItemSerializer.php +++ b/src/data/bedrock/item/ItemSerializer.php @@ -33,7 +33,6 @@ use pocketmine\item\Item; use pocketmine\item\ItemBlock; use pocketmine\item\VanillaItems as Items; use pocketmine\utils\AssumptionFailedError; -use function class_parents; use function get_class; final class ItemSerializer{ @@ -41,14 +40,14 @@ final class ItemSerializer{ * These callables actually accept Item, but for the sake of type completeness, it has to be never, since we can't * describe the bottom type of a type hierarchy only containing Item. * - * @var \Closure[][] - * @phpstan-var array> + * @var \Closure[] + * @phpstan-var array */ private array $itemSerializers = []; /** * @var \Closure[][] - * @phpstan-var array> + * @phpstan-var array */ private array $blockItemSerializers = []; @@ -69,7 +68,7 @@ final class ItemSerializer{ if(isset($this->itemSerializers[$index])){ throw new \InvalidArgumentException("Item type ID " . $index . " already has a serializer registered"); } - $this->itemSerializers[$index][get_class($item)] = $serializer; + $this->itemSerializers[$index] = $serializer; } /** @@ -82,7 +81,7 @@ final class ItemSerializer{ if(isset($this->blockItemSerializers[$index])){ throw new AssumptionFailedError("Registering the same blockitem twice!"); } - $this->blockItemSerializers[$index][get_class($block)] = $serializer; + $this->blockItemSerializers[$index] = $serializer; } /** @@ -100,21 +99,16 @@ final class ItemSerializer{ }else{ $index = $item->getTypeId(); - $locatedSerializer = $this->itemSerializers[$index][get_class($item)] ?? null; - if($locatedSerializer === null){ - foreach(class_parents($item) as $parent){ - if(isset($this->itemSerializers[$index][$parent])){ - $locatedSerializer = $this->itemSerializers[$index][$parent]; - break; - } - } - } - + $locatedSerializer = $this->itemSerializers[$index] ?? null; if($locatedSerializer === null){ throw new ItemTypeSerializeException("No serializer registered for " . get_class($item) . " ($index) " . $item->getName()); } /** + * TODO: there is no guarantee that this type actually matches that of $item - a plugin may have stolen + * the type ID of the item (which never makes sense, even in a world where overriding item types is a thing). + * In the future we'll need some way to guarantee that type IDs are never reused (perhaps spl_object_id()?) + * * @var \Closure $serializer * @phpstan-var \Closure(TItemType) : Data $serializer */ @@ -156,18 +150,15 @@ final class ItemSerializer{ private function serializeBlockItem(Block $block) : Data{ $index = $block->getTypeId(); - $locatedSerializer = $this->blockItemSerializers[$index][get_class($block)] ?? null; - if($locatedSerializer === null){ - foreach(class_parents($block) as $parent){ - if(isset($this->blockItemSerializers[$index][$parent])){ - $locatedSerializer = $this->blockItemSerializers[$index][$parent]; - break; - } - } - } - + $locatedSerializer = $this->blockItemSerializers[$index] ?? null; if($locatedSerializer !== null){ - /** @phpstan-var \Closure(TBlockType) : Data $serializer */ + /** + * TODO: there is no guarantee that this type actually matches that of $block - a plugin may have stolen + * the type ID of the block (which never makes sense, even in a world where overriding block types is a thing). + * In the future we'll need some way to guarantee that type IDs are never reused (perhaps spl_object_id()?) + * + * @phpstan-var \Closure(TBlockType) : Data $serializer + */ $serializer = $locatedSerializer; $data = $serializer($block); }else{