Inventory: Use exceptions to report back why a transaction failed

Returning false all the time could mean any one of a range of things. Throwing exceptions is better in that it allows us to catch them and see what actually broke.

src/pocketmine/Player.php

@@ -74,6 +74,7 @@ use pocketmine\inventory\PlayerCursorInventory;
 use pocketmine\inventory\transaction\action\InventoryAction;
 use pocketmine\inventory\transaction\CraftingTransaction;
 use pocketmine\inventory\transaction\InventoryTransaction;
+use pocketmine\inventory\transaction\TransactionValidationException;
 use pocketmine\item\Consumable;
 use pocketmine\item\Item;
 use pocketmine\item\WritableBook;
@@ -2302,13 +2303,16 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{
 				//we get the actions for this in several packets, so we need to wait until we have all the pieces before
 				//trying to execute it
 
-				$result = $this->craftingTransaction->execute();
+				$ret = true;
-				if(!$result){
+				try{
-					$this->server->getLogger()->debug("Failed to execute crafting transaction from " . $this->getName());
+					$this->craftingTransaction->execute();
+				}catch(TransactionValidationException $e){
+					$this->server->getLogger()->debug("Failed to execute crafting transaction for " . $this->getName() . ": " . $e->getMessage());
+					$ret = false;
 				}
 
 				$this->craftingTransaction = null;
-				return $result;
+				return $ret;
 			}
 
 			return true;
@@ -2322,10 +2326,13 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{
 				$this->setUsingItem(false);
 				$transaction = new InventoryTransaction($this, $actions);
 
-				if(!$transaction->execute()){
+				try{
-					$this->server->getLogger()->debug("Failed to execute inventory transaction from " . $this->getName() . " with actions: " . json_encode($packet->actions));
+					$transaction->execute();
+				}catch(TransactionValidationException $e){
+					$this->server->getLogger()->debug("Failed to execute inventory transaction from " . $this->getName() . ": " . $e->getMessage());
+					$this->server->getLogger()->debug("Actions: " . json_encode($packet->actions));
 
-					return false; //oops!
+					return false;
 				}
 
 				//TODO: fix achievement for getting iron from furnace

src/pocketmine/inventory/transaction/CraftingTransaction.php

@@ -28,7 +28,6 @@ use pocketmine\inventory\CraftingRecipe;
 use pocketmine\item\Item;
 use pocketmine\network\mcpe\protocol\ContainerClosePacket;
 use pocketmine\network\mcpe\protocol\types\ContainerIds;
-use pocketmine\utils\MainLogger;
 
 class CraftingTransaction extends InventoryTransaction{
 	/** @var CraftingRecipe|null */
@@ -47,15 +46,14 @@ class CraftingTransaction extends InventoryTransaction{
 	 * @param bool   $wildcards
 	 *
 	 * @return int
-	 * @throws \InvalidStateException
+	 * @throws TransactionValidationException
-	 * @throws \InvalidArgumentException
 	 */
 	protected function matchRecipeItems(array $txItems, array $recipeItems, bool $wildcards) : int{
 		if(empty($recipeItems)){
-			throw new \InvalidArgumentException("No recipe items given");
+			throw new TransactionValidationException("No recipe items given");
 		}
 		if(empty($txItems)){
-			throw new \InvalidArgumentException("No transaction items given");
+			throw new TransactionValidationException("No transaction items given");
 		}
 
 		$iterations = 0;
@@ -80,59 +78,54 @@ class CraftingTransaction extends InventoryTransaction{
 
 			if($haveCount % $needCount !== 0){
 				//wrong count for this output, should divide exactly
-				throw new \InvalidStateException("Expected an exact multiple of required $recipeItem (given: $haveCount, needed: $needCount)");
+				throw new TransactionValidationException("Expected an exact multiple of required $recipeItem (given: $haveCount, needed: $needCount)");
 			}
 
 			$multiplier = intdiv($haveCount, $needCount);
 			if($multiplier < 1){
-				throw new \InvalidStateException("Expected more than zero items matching $recipeItem (given: $haveCount, needed: $needCount)");
+				throw new TransactionValidationException("Expected more than zero items matching $recipeItem (given: $haveCount, needed: $needCount)");
 			}
 			if($iterations === 0){
 				$iterations = $multiplier;
 			}elseif($multiplier !== $iterations){
 				//wrong count for this output, should match previous outputs
-				throw new \InvalidStateException("Expected $recipeItem x$iterations, but found x$multiplier");
+				throw new TransactionValidationException("Expected $recipeItem x$iterations, but found x$multiplier");
 			}
 		}
 
 		if($iterations < 1){
-			throw new \InvalidStateException("Tried to craft zero times");
+			throw new TransactionValidationException("Tried to craft zero times");
 		}
 		if(!empty($txItems)){
 			//all items should be destroyed in this process
-			throw new \InvalidStateException("Expected 0 ingredients left over, have " . count($txItems));
+			throw new TransactionValidationException("Expected 0 ingredients left over, have " . count($txItems));
 		}
 
 		return $iterations;
 	}
 
-
+	public function validate() : void{
-	public function canExecute() : bool{
 		$this->squashDuplicateSlotChanges();
 		if(count($this->actions) < 1){
-			return false;
+			throw new TransactionValidationException("Transaction must have at least one action to be executable");
 		}
 
 		$this->matchItems($this->outputs, $this->inputs);
 
 		$this->recipe = $this->source->getServer()->getCraftingManager()->matchRecipe($this->source->getCraftingGrid(), $this->outputs);
 		if($this->recipe === null){
-			return false;
+			throw new TransactionValidationException("Unable to match a recipe to transaction");
 		}
 
 		try{
 			$this->repetitions = $this->matchRecipeItems($this->outputs, $this->recipe->getResultsFor($this->source->getCraftingGrid()), false);
 
 			if(($inputIterations = $this->matchRecipeItems($this->inputs, $this->recipe->getIngredientList(), true)) !== $this->repetitions){
-				throw new \InvalidStateException("Tried to craft recipe $this->repetitions times in batch, but have enough inputs for $inputIterations");
+				throw new TransactionValidationException("Tried to craft recipe $this->repetitions times in batch, but have enough inputs for $inputIterations");
 			}
-
-			return true;
 		}catch(\InvalidStateException $e){
-			MainLogger::getLogger()->debug("Failed to validate crafting transaction for " . $this->source->getName() . ": " . $e->getMessage());
+			throw new TransactionValidationException($e->getMessage());
 		}
-
-		return false;
 	}
 
 	protected function callExecuteEvent() : bool{

src/pocketmine/inventory/transaction/InventoryTransaction.php

@@ -30,7 +30,6 @@ use pocketmine\inventory\transaction\action\SlotChangeAction;
 use pocketmine\item\Item;
 use pocketmine\Player;
 use pocketmine\Server;
-use pocketmine\utils\MainLogger;
 
 /**
  * This InventoryTransaction only allows doing Transaction between one / two inventories
@@ -113,16 +112,16 @@ class InventoryTransaction{
 	 * @param Item[] $needItems
 	 * @param Item[] $haveItems
 	 *
-	 * @return bool
+	 * @throws TransactionValidationException
 	 */
-	protected function matchItems(array &$needItems, array &$haveItems) : bool{
+	protected function matchItems(array &$needItems, array &$haveItems) : void{
 		foreach($this->actions as $key => $action){
 			if(!$action->getTargetItem()->isNull()){
 				$needItems[] = $action->getTargetItem();
 			}
 
 			if(!$action->isValid($this->source)){
-				return false;
+				throw new TransactionValidationException("Action " . get_class($action) . " is not valid in the current transaction");
 			}
 
 			if(!$action->getSourceItem()->isNull()){
@@ -146,8 +145,6 @@ class InventoryTransaction{
 				}
 			}
 		}
-
-		return true;
 	}
 
 	/**
@@ -159,10 +156,8 @@ class InventoryTransaction{
 	 * multiple slot changes referring to the same slot in a single transaction. These multiples are not even guaranteed
 	 * to be in the correct order (slot splitting in the crafting grid for example, causes the actions to be sent in the
 	 * wrong order), so this method also tries to chain them into order.
-	 *
-	 * @return bool
 	 */
-	protected function squashDuplicateSlotChanges() : bool{
+	protected function squashDuplicateSlotChanges() : void{
 		/** @var SlotChangeAction[][] $slotChanges */
 		$slotChanges = [];
 		/** @var Inventory[] $inventories */
@@ -189,8 +184,7 @@ class InventoryTransaction{
 
 			$targetItem = $this->findResultItem($sourceItem, $list);
 			if($targetItem === null){
-				MainLogger::getLogger()->debug("Failed to compact " . count($list) . " actions for " . $this->source->getName());
+				throw new TransactionValidationException("Failed to compact " . count($list) . " duplicate actions");
-				return false;
 			}
 
 			foreach($list as $action){
@@ -202,8 +196,6 @@ class InventoryTransaction{
 				$this->addAction(new SlotChangeAction($inventory, $slot, $sourceItem, $targetItem));
 			}
 		}
-
-		return true;
 	}
 
 	/**
@@ -233,14 +225,26 @@ class InventoryTransaction{
 	}
 
 	/**
-	 * @return bool
+	 * Verifies that the transaction can execute.
+	 *
+	 * @throws TransactionValidationException
 	 */
-	public function canExecute() : bool{
+	public function validate() : void{
 		$this->squashDuplicateSlotChanges();
 
 		$haveItems = [];
 		$needItems = [];
-		return $this->matchItems($needItems, $haveItems) and count($this->actions) > 0 and count($haveItems) === 0 and count($needItems) === 0;
+		$this->matchItems($needItems, $haveItems);
+		if(count($this->actions) === 0){
+			throw new TransactionValidationException("Inventory transaction must have at least one action to be executable");
+		}
+
+		if(count($haveItems) > 0){
+			throw new TransactionValidationException("Transaction does not balance (tried to destroy some items)");
+		}
+		if(count($needItems) > 0){
+			throw new TransactionValidationException("Transaction does not balance (tried to create some items)");
+		}
 	}
 
 	protected function sendInventories() : void{
@@ -257,13 +261,17 @@ class InventoryTransaction{
 	/**
 	 * Executes the group of actions, returning whether the transaction executed successfully or not.
 	 * @return bool
+	 *
+	 * @throws TransactionValidationException
 	 */
 	public function execute() : bool{
-		if($this->hasExecuted() or !$this->canExecute()){
+		if($this->hasExecuted()){
 			$this->sendInventories();
 			return false;
 		}
 
+		$this->validate();
+
 		if(!$this->callExecuteEvent()){
 			$this->sendInventories();
 			return false;

src/pocketmine/inventory/transaction/TransactionValidationException.php

@@ -0,0 +1,28 @@
+<?php
+
+/*
+ *
+ *  ____            _        _   __  __ _                  __  __ ____
+ * |  _ \ ___   ___| | _____| |_|  \/  (_)_ __   ___      |  \/  |  _ \
+ * | |_) / _ \ / __| |/ / _ \ __| |\/| | | '_ \ / _ \_____| |\/| | |_) |
+ * |  __/ (_) | (__|   <  __/ |_| |  | | | | | |  __/_____| |  | |  __/
+ * |_|   \___/ \___|_|\_\___|\__|_|  |_|_|_| |_|\___|     |_|  |_|_|
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * @author PocketMine Team
+ * @link http://www.pocketmine.net/
+ *
+ *
+*/
+
+declare(strict_types=1);
+
+namespace pocketmine\inventory\transaction;
+
+class TransactionValidationException extends \RuntimeException{
+
+}