[RFC] Disallow uncancelling events by default

[SOF3]

1. Introduction

The definition of "cancelling" an event is confusing enough. It is even more confusing to check why an event was cancelled. Plugins calling setCancelled(false) rarely consider the reason why an event was cancelled. According to a search on Poggit, most usages of setCancelled with non-constant-true values are either delegation from another event during creation (which could just be replaced by an if) or blind uncancelling that does not respect previous handlers. This uncooperative behaviour should not be allowed.

2. Concerns

(a) What if I really want to allow uncancelling?

If you are not using CancellableTrait, this RFC does not affect you. Events that have special cancellation logic (such as asking cancellers to pass the reason) most likely have a different implementation of Cancellable than the default CancellableTrait.

(b) I want to have the final say about whether it is cancelled

No, just no. Plugins should be cooperative by design. Plugins cancelling an event should provide their own event (or other mechanisms) to allow cancelling their cancellation. Remove the other plugin if it really bothers you and doesn't allow you to cancel their cancellation.

For example, quoting a scenario from @HimmelKreis4865#0001 on Discord:

Lets just think of myplot, doors can be opened by owner & trusted ppl not by other persons
so maybe you want to make an addon to let everyone open every door.
you would have to

1. rewrite myplot
2. uncancel the event under special conditions in another pl

The suggested solution is to modify MyPlot such that it fires a cancellable event like MyPlot\Event\RestrictedDoorEvent, which gets fired when a restricted door is opened by an unauthorized player. Cancellation of RestrictedDoorEvent shall prevent MyPlot from calling PlayerInteractEvent::setCancelled(). This is more reliable than calling PlayerInteracatEvent::setCancelled(false) blindly, which could end up allowing spectators to open doors.

(c) I just want to avoid writing if($cond) { $ev->setCancelled(); }

Causing an antipattern just because you want to save a few bytes? Rethink about your life.

3. API Changes

• setCancelled(true) must be changed to setCancelled()
• setCancelled(false) is no longer possible

4. Backwards compatibility

This plugin breaks all plugins that try to uncancel events. See the Poggit grepPlugins report linked in Section 1 for details.

5. Alternatives

Rename setCancelled() to cancel() to avoid confusion with backward compatibility, and remove the assertion check.

6. Follow-up

• Events that are cancelled by default shall be updated as suggested in Subsection 2.(b).
• A further change to remove explicit @handleCancelled could be considered since its main possible usage is to uncancel events.

[dktapps]

addendum to c): setCancelled($cond) is a bug in most cases.

[dktapps]

There are some problems with this PR primarily involving pre-cancelled events being emitted by core code. In some cases events are cancelled out of the box (for example spectator mode interactions).

[dktapps]

So I realized recently that pre-cancelled core events aren't actually a blocker, since there's no reason why the cancellation events would have to fire during the cancelled event itself. For spectator mode stuff we can just introduce a PlayerGameModeActionCancelEvent (someone else think of a better name pls). Core code behaviour is essentially the same as if there was another priority before LOWEST that was cancelling the event.

It would break BC for people who use the likes of PlayerInteractEvent for spectator players, since instead of uncancelling the event they'd now have to cancel the new event instead. However this is no different to other people whose workflows would be disrupted by a change like this.

[dktapps]

I will say I'm not quite sure I like this change.

It means that plugins which want to uncancel events would have to know which cancellation event to cancel, which means they'd have to know exactly who cancelled the event and why. I don't know if it's a good idea to impose that requirement.

[SOF3]

which means they'd have to know exactly who cancelled the event and why.

This is pretty much an objective rather than a side effect. If you just uncancel at a certain priority, the behavior is undefined if there are other plugins at the same priority also cancelling the event. Meanwhile in reality you would only ever want to uncancel a specific plugin, i.e. to cancel another cancelation.

I agree that it is not optimal for plugins to have to support other specific plugins for uncancelation. To solve this problem, we would need a more general API for users to configure plugin interaction similar to how permissions work (permission plugins don't need to be aware of the permission checking plugins, nor the other way round). I agree that the current lack of such API would be a barrier for merging this PR, but uncancelation is still an XY problem nonetheless.

[dktapps]

I think ultimately it boils down to this: We need a way for plugins to tell why an event was cancelled.

Having plugins fire off cancellation events is one way. Another way would be to pass a parameter to cancel() to pass a cause parameter. Both would achieve more or less the same result. However, I don't see a good way to generalize them in either case.

[dktapps]

I've opened an issue to discuss the core need here. I'm going to close the PR as it's probably easier to just recreate it later if the desire arises.