Cancel block events on low priority, and never 'uncancel'#7828
Cancel block events on low priority, and never 'uncancel'#78282008Choco wants to merge 1 commit intoTownyAdvanced:masterfrom 2008Choco:low-priority-cancellation
Conversation
|
You are correct that VeinMiner is at fault for listening too early. Towny acts in the way it does in order to work along side WorldGuard properly. We are "downstream" from WorldGuard on many servers, and this goes back to the very beginning of bukkit servers, when only WG and Towny were available for protection plugins. If we did change things around WorldGuard's regions would override Towny towns causing chaos. |
|
I still believe that the uncancelling logic is incorrect, regardless of the need for WorldGuard support. Other plugins are going to suffer similar issues as VeinMiner because the expectation is that protection plugins protect earlier in the pipeline. I can resolve VeinMiner's incompatibility easily because the listener doesn't mutate the event, so |
This is not really a valid expectation. There was never a rule placed upon protection plugins to do things earlier in line. A plugin that acts on an event without also changing the outcome of the event, should not listen to any priority other than Monitor. To do anything different is to just ignore the HIGH and HIGHEST priorities. There's nothing wrong about using HIGH and HIGHEST. As for un-cancelling cancelled events, you could make up a PR to show me what those changes look like, but there's things that Towny's own add-on plugins will uncancel Towny decisions based on Towny's API. |
Description:
Towny has a responsibility to cancel block breaks, placements, explosions, and more, to prevent players from griefing blocks in towns to which they do not belong. It does this successfully, but unconventionally compared to other world protection plugins like WorldEdit or GriefPrevention. Generally speaking, these protection plugins are intended to be run very early so that downstream listeners can ignore any cancelled events calls. Towny does the opposite. It listens like a downstream plugin and changes the cancellation state of an event later in the listener pipeline, causing plugins listening on the default event priority to mistakenly enable griefing.
Additionally, the fact that Towny has the ability to uncancel events is separately problematic. While
Cancellable#setCancelled(condition)may seem like harmless shorthand, it has the consequence of potentially invokingCancellable#setCancelled(false), which will negate other protections from other plugins. This should be avoided at all costs, and has been remedied in this pull request.This pull request was drafted after a user from VeinMiner noticed that they were able to vein mine outside of a town's borders, causing neighbouring blocks inside the town's borders to drop items, but blocks to not be destroyed, allowing for item duplication. The reason for this is because VeinMiner listens on the default event priority (which I think is also a mistake, it can safely listen on the
MONITORpriority, but that's besides the point). VeinMiner does its vein mining, then Towny handles its block protection.New Nodes/Commands/ConfigOptions:
N/A
Relevant Towny Issue ticket:
N/A
I have not tested this on a server, however I am confident that the changes made are trivial enough to not need a thorough test. This is a compatibility change so that other plugins may better support Towny. Testing this thoroughly would be extremely difficult.
By making this pull request, I represent that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the TownyAdvanced organization has the copyright to use and modify my contribution under the Towny License for perpetuity.