From 7b02cc3efdadb6a25882dccc4e84536ada6b9b63 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Sat, 26 Sep 2020 14:31:56 +0100 Subject: [PATCH] Implemented #3836: Replace setCancelled() in events with cancel() and uncancel() The motivation for this is to prevent passing a dynamic argument to cancellation, which in almost all cases is a bug in user code. This same mistake also appears in a few places in the PM core (as seen in this commit), but in those cases the mistakes were mostly harmless since they were taking place before the event was actually called. closes #3836 --- src/Server.php | 4 +++- src/block/Fire.php | 2 +- src/entity/Living.php | 6 +++--- src/entity/effect/EffectManager.php | 4 +++- src/entity/projectile/Arrow.php | 2 +- src/event/CancellableTrait.php | 8 ++++++-- src/event/entity/EntityEffectRemoveEvent.php | 4 ++-- src/item/Bow.php | 2 +- src/player/Player.php | 18 ++++++++++-------- src/world/World.php | 14 +++++++++----- src/world/WorldManager.php | 2 +- .../src/pmmp/TesterPlugin/Main.php | 2 +- 12 files changed, 41 insertions(+), 27 deletions(-) diff --git a/src/Server.php b/src/Server.php index 1a9dd31e9..0d13ba043 100644 --- a/src/Server.php +++ b/src/Server.php @@ -549,7 +549,9 @@ class Server{ public function saveOfflinePlayerData(string $name, CompoundTag $nbtTag) : void{ $ev = new PlayerDataSaveEvent($nbtTag, $name); - $ev->setCancelled(!$this->shouldSavePlayerData()); + if(!$this->shouldSavePlayerData()){ + $ev->cancel(); + } $ev->call(); diff --git a/src/block/Fire.php b/src/block/Fire.php index be94f8ab3..4e31baf82 100644 --- a/src/block/Fire.php +++ b/src/block/Fire.php @@ -74,7 +74,7 @@ class Fire extends Flowable{ $ev = new EntityCombustByBlockEvent($this, $entity, 8); if($entity instanceof Arrow){ - $ev->setCancelled(); + $ev->cancel(); } $ev->call(); if(!$ev->isCancelled()){ diff --git a/src/entity/Living.php b/src/entity/Living.php index 85dbc6a05..89d26927f 100644 --- a/src/entity/Living.php +++ b/src/entity/Living.php @@ -356,7 +356,7 @@ abstract class Living extends Entity{ public function applyDamageModifiers(EntityDamageEvent $source) : void{ if($this->lastDamageCause !== null and $this->attackTime > 0){ if($this->lastDamageCause->getBaseDamage() >= $source->getBaseDamage()){ - $source->setCancelled(); + $source->cancel(); } $source->setModifier(-$this->lastDamageCause->getBaseDamage(), EntityDamageEvent::MODIFIER_PREVIOUS_DAMAGE_COOLDOWN); } @@ -437,7 +437,7 @@ abstract class Living extends Entity{ public function attack(EntityDamageEvent $source) : void{ if($this->noDamageTicks > 0){ - $source->setCancelled(); + $source->cancel(); } if($this->effectManager->has(VanillaEffects::FIRE_RESISTANCE()) and ( @@ -446,7 +446,7 @@ abstract class Living extends Entity{ or $source->getCause() === EntityDamageEvent::CAUSE_LAVA ) ){ - $source->setCancelled(); + $source->cancel(); } $this->applyDamageModifiers($source); diff --git a/src/entity/effect/EffectManager.php b/src/entity/effect/EffectManager.php index 842447ae3..d5343b777 100644 --- a/src/entity/effect/EffectManager.php +++ b/src/entity/effect/EffectManager.php @@ -146,7 +146,9 @@ class EffectManager{ } $ev = new EntityEffectAddEvent($this->entity, $effect, $oldEffect); - $ev->setCancelled($cancelled); + if($cancelled){ + $ev->cancel(); + } $ev->call(); if($ev->isCancelled()){ diff --git a/src/entity/projectile/Arrow.php b/src/entity/projectile/Arrow.php index a5961dc2c..39f62fa91 100644 --- a/src/entity/projectile/Arrow.php +++ b/src/entity/projectile/Arrow.php @@ -178,7 +178,7 @@ class Arrow extends Projectile{ $ev = new InventoryPickupArrowEvent($playerInventory, $this); if($this->pickupMode === self::PICKUP_NONE or ($this->pickupMode === self::PICKUP_CREATIVE and !$player->isCreative())){ - $ev->setCancelled(); + $ev->cancel(); } $ev->call(); diff --git a/src/event/CancellableTrait.php b/src/event/CancellableTrait.php index c4eae4290..313c49928 100644 --- a/src/event/CancellableTrait.php +++ b/src/event/CancellableTrait.php @@ -39,7 +39,11 @@ trait CancellableTrait{ return $this->isCancelled; } - public function setCancelled(bool $value = true) : void{ - $this->isCancelled = $value; + public function cancel() : void{ + $this->isCancelled = true; + } + + public function uncancel() : void{ + $this->isCancelled = false; } } diff --git a/src/event/entity/EntityEffectRemoveEvent.php b/src/event/entity/EntityEffectRemoveEvent.php index 56099e958..afd2ef631 100644 --- a/src/event/entity/EntityEffectRemoveEvent.php +++ b/src/event/entity/EntityEffectRemoveEvent.php @@ -27,10 +27,10 @@ namespace pocketmine\event\entity; * Called when an effect is removed from an entity. */ class EntityEffectRemoveEvent extends EntityEffectEvent{ - public function setCancelled(bool $value = true) : void{ + public function cancel() : void{ if($this->getEffect()->getDuration() <= 0){ throw new \InvalidStateException("Removal of expired effects cannot be cancelled"); } - parent::setCancelled($value); + parent::cancel(); } } diff --git a/src/item/Bow.php b/src/item/Bow.php index 2c660354e..d57bff29b 100644 --- a/src/item/Bow.php +++ b/src/item/Bow.php @@ -80,7 +80,7 @@ class Bow extends Tool implements Releasable{ $ev = new EntityShootBowEvent($player, $this, $entity, $baseForce * 3); if($baseForce < 0.1 or $diff < 5 or $player->isSpectator()){ - $ev->setCancelled(); + $ev->cancel(); } $ev->call(); diff --git a/src/player/Player.php b/src/player/Player.php index 1a00dc2c2..d7f1c1786 100644 --- a/src/player/Player.php +++ b/src/player/Player.php @@ -1428,7 +1428,7 @@ class Player extends Human implements CommandSender, ChunkListener, IPlayer{ $ev = new PlayerItemUseEvent($this, $item, $directionVector); if($this->hasItemCooldown($item) or $this->isSpectator()){ - $ev->setCancelled(); + $ev->cancel(); } $ev->call(); @@ -1462,7 +1462,7 @@ class Player extends Human implements CommandSender, ChunkListener, IPlayer{ if($slot instanceof ConsumableItem){ $ev = new PlayerItemConsumeEvent($this, $slot); if($this->hasItemCooldown($slot)){ - $ev->setCancelled(); + $ev->cancel(); } $ev->call(); @@ -1521,7 +1521,7 @@ class Player extends Human implements CommandSender, ChunkListener, IPlayer{ $ev = new PlayerBlockPickEvent($this, $block, $item); $existingSlot = $this->inventory->first($item); if($existingSlot === -1 and $this->hasFiniteResources()){ - $ev->setCancelled(); + $ev->cancel(); } $ev->call(); @@ -1563,7 +1563,7 @@ class Player extends Human implements CommandSender, ChunkListener, IPlayer{ $ev = new PlayerInteractEvent($this, $this->inventory->getItemInHand(), $target, null, $face, PlayerInteractEvent::LEFT_CLICK_BLOCK); if($this->isSpectator()){ - $ev->setCancelled(); + $ev->cancel(); } $ev->call(); if($ev->isCancelled()){ @@ -1669,7 +1669,7 @@ class Player extends Human implements CommandSender, ChunkListener, IPlayer{ $ev = new EntityDamageByEntityEvent($this, $entity, EntityDamageEvent::CAUSE_ENTITY_ATTACK, $heldItem->getAttackPoints()); if($this->isSpectator() or !$this->canInteract($entity->getLocation(), 8) or ($entity instanceof Player and !$this->server->getConfigGroup()->getConfigBool("pvp"))){ - $ev->setCancelled(); + $ev->cancel(); } $meleeEnchantmentDamage = 0; @@ -1751,7 +1751,9 @@ class Player extends Human implements CommandSender, ChunkListener, IPlayer{ public function toggleFlight(bool $fly) : bool{ $ev = new PlayerToggleFlightEvent($this, $fly); - $ev->setCancelled(!$this->allowFlight); + if(!$this->allowFlight){ + $ev->cancel(); + } $ev->call(); if($ev->isCancelled()){ return false; @@ -2175,9 +2177,9 @@ class Player extends Human implements CommandSender, ChunkListener, IPlayer{ and $source->getCause() !== EntityDamageEvent::CAUSE_SUICIDE and $source->getCause() !== EntityDamageEvent::CAUSE_VOID ){ - $source->setCancelled(); + $source->cancel(); }elseif($this->allowFlight and $source->getCause() === EntityDamageEvent::CAUSE_FALL){ - $source->setCancelled(); + $source->cancel(); } parent::attack($source); diff --git a/src/world/World.php b/src/world/World.php index f21fdbd63..8620a55b4 100644 --- a/src/world/World.php +++ b/src/world/World.php @@ -1448,7 +1448,7 @@ class World implements ChunkManager{ $ev = new BlockBreakEvent($player, $target, $item, $player->isCreative(), $drops, $xpDrop); if($target instanceof Air or ($player->isSurvival() and !$target->getBreakInfo()->isBreakable()) or $player->isSpectator()){ - $ev->setCancelled(); + $ev->cancel(); } if($player->isAdventure(true) and !$ev->isCancelled()){ @@ -1462,7 +1462,9 @@ class World implements ChunkManager{ } } - $ev->setCancelled(!$canBreak); + if(!$canBreak){ + $ev->cancel(); + } } $ev->call(); @@ -1538,7 +1540,7 @@ class World implements ChunkManager{ if($player !== null){ $ev = new PlayerInteractEvent($player, $item, $blockClicked, $clickVector, $face, PlayerInteractEvent::RIGHT_CLICK_BLOCK); if($player->isSpectator()){ - $ev->setCancelled(); //set it to cancelled so plugins can bypass this + $ev->cancel(); //set it to cancelled so plugins can bypass this } $ev->call(); @@ -1581,7 +1583,7 @@ class World implements ChunkManager{ if($player !== null){ $ev = new BlockPlaceEvent($player, $hand, $blockReplace, $blockClicked, $item); if($player->isSpectator()){ - $ev->setCancelled(); + $ev->cancel(); } if($player->isAdventure(true) and !$ev->isCancelled()){ @@ -1595,7 +1597,9 @@ class World implements ChunkManager{ } } - $ev->setCancelled(!$canPlace); + if(!$canPlace){ + $ev->cancel(); + } } $ev->call(); diff --git a/src/world/WorldManager.php b/src/world/WorldManager.php index 974a57422..dbf1e6c7b 100644 --- a/src/world/WorldManager.php +++ b/src/world/WorldManager.php @@ -141,7 +141,7 @@ class WorldManager{ $ev = new WorldUnloadEvent($world); if($world === $this->defaultWorld and !$forceUnload){ - $ev->setCancelled(true); + $ev->cancel(); } $ev->call(); diff --git a/tests/plugins/TesterPlugin/src/pmmp/TesterPlugin/Main.php b/tests/plugins/TesterPlugin/src/pmmp/TesterPlugin/Main.php index 85af44b72..a9de8cdf5 100644 --- a/tests/plugins/TesterPlugin/src/pmmp/TesterPlugin/Main.php +++ b/tests/plugins/TesterPlugin/src/pmmp/TesterPlugin/Main.php @@ -51,7 +51,7 @@ class Main extends PluginBase implements Listener{ //be asynchronous tests running. Instead we cancel this and stop the server of our own accord once all tests //have completed. if($event->getCommand() === "stop"){ - $event->setCancelled(); + $event->cancel(); } }