Force-close the inventory window when crafting fails to avoid desync issues

mojang, why does this have to be hard work

src/pocketmine/Player.php

@@ -2297,7 +2297,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{
 			if($this->craftingTransaction->getPrimaryOutput() !== null){
 				//we get the actions for this in several packets, so we can't execute it until we get the result
 
-				$this->craftingTransaction->execute(); //if it can't execute, no inventories will be modified
+				$this->craftingTransaction->execute();
 				$this->craftingTransaction = null;
 			}
 
@@ -2312,16 +2312,8 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{
 				$transaction = new InventoryTransaction($this, $actions);
 
 				if(!$transaction->execute()){
-					foreach($transaction->getInventories() as $inventory){
-						$inventory->sendContents($this);
-						if($inventory instanceof PlayerInventory){
-							$inventory->sendArmorContents($this);
-						}
-					}
-
 					$this->server->getLogger()->debug("Failed to execute inventory transaction from " . $this->getName() . " with actions: " . json_encode($packet->actions));
 
-					//TODO: check more stuff that might need reversion
 					return false; //oops!
 				}
 

src/pocketmine/inventory/CraftingGrid.php

@@ -23,6 +23,8 @@ declare(strict_types=1);
 
 namespace pocketmine\inventory;
 
+use pocketmine\network\mcpe\protocol\ContainerClosePacket;
+use pocketmine\network\mcpe\protocol\types\ContainerIds;
 use pocketmine\Player;
 
 class CraftingGrid extends BaseInventory{
@@ -48,6 +50,20 @@ class CraftingGrid extends BaseInventory{
 	}
 
 	public function sendContents($target) : void{
-		//we can't send the contents of a client-sided inventory window
+		if(!is_array($target)){
+			$target = [$target];
+		}
+
+		/*
+		 * TODO: HACK!
+		 * we can't resend the contents of this window, so we force the client to close it instead.
+		 * So people don't whine about messy desync issues when someone cancels CraftItemEvent, or when a crafting
+		 * transaction goes wrong.
+		 */
+		$pk = new ContainerClosePacket();
+		$pk->windowId = ContainerIds::NONE;
+		foreach($target as $player){
+			$player->dataPacket($pk);
+		}
 	}
 }

src/pocketmine/inventory/transaction/InventoryTransaction.php

@@ -25,6 +25,7 @@ namespace pocketmine\inventory\transaction;
 
 use pocketmine\event\inventory\InventoryTransactionEvent;
 use pocketmine\inventory\Inventory;
+use pocketmine\inventory\PlayerInventory;
 use pocketmine\inventory\transaction\action\InventoryAction;
 use pocketmine\inventory\transaction\action\SlotChangeAction;
 use pocketmine\item\Item;
@@ -235,9 +236,12 @@ class InventoryTransaction{
 		return $this->matchItems($needItems, $haveItems) and count($this->actions) > 0 and count($haveItems) === 0 and count($needItems) === 0;
 	}
 
-	protected function handleFailed() : void{
-		foreach($this->actions as $action){
-			$action->onExecuteFail($this->source);
+	protected function sendInventories() : void{
+		foreach($this->inventories as $inventory){
+			$inventory->sendContents($this->source);
+			if($inventory instanceof PlayerInventory){
+				$inventory->sendArmorContents($this->source);
+			}
 		}
 	}
 
@@ -247,22 +251,24 @@ class InventoryTransaction{
 	}
 
 	/**
+	 * Executes the group of actions, returning whether the transaction executed successfully or not.
 	 * @return bool
 	 */
 	public function execute() : bool{
 		if($this->hasExecuted() or !$this->canExecute()){
+			$this->sendInventories();
 			return false;
 		}
 
 		if(!$this->callExecuteEvent()){
-			$this->handleFailed();
-			return true;
+			$this->sendInventories();
+			return false;
 		}
 
 		foreach($this->actions as $action){
 			if(!$action->onPreExecute($this->source)){
-				$this->handleFailed();
-				return true;
+				$this->sendInventories();
+				return false;
 			}
 		}