Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "cancel on error" option for event listeners #11691

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NewwindServer
Copy link
Contributor

@NewwindServer NewwindServer commented Dec 1, 2024

Adds the option to cancel an event if an error is thrown inside its handler
useful for mission critical listeners, for example, preventing players from breaking blocks in an area
or preventing players from modifying certain items inside an anvil.

Of course, ideally, your plugin shouldn't be throwing errors in listeners, but mistakes can happen, this feature adds a way for plugin authors to have a fail-safe for important listeners, for example:

Before:

@EventHandler(ignoreCancelled = true)
    public void onBlockPlace(BlockPlaceEvent event) {
        try {
            if (isSpecialItem(event.getItemInHand())) {
                event.setCancelled(true);
            } else if (isProtectedArea(event.getBlock().getLocation())) {
                event.getPlayer().sendMessage(getLocalized("cant.use.here")); // Maybe at some point this will fail
                event.setCancelled(true);
            }
        } catch (Throwable throwable) { // fail safe
            event.setCancelled(true);
        }
    }

After:

@EventHandler(ignoreCancelled = true, cancelOnError = true)
    public void onBlockPlace(BlockPlaceEvent event) {
        if (isSpecialItem(event.getItemInHand())) {
            event.setCancelled(true);
        } else if (isProtectedArea(event.getBlock().getLocation())) {
            event.getPlayer().sendMessage(getLocalized("cant.use.here")); // Maybe at some point this will fail
            event.setCancelled(true);
        }
    }

Also, I didn't bother adding it for TimedEventListener since those are all behind if (false) statements and timings is about to be removed.

@NewwindServer NewwindServer requested a review from a team as a code owner December 1, 2024 01:45
@lynxplay
Copy link
Contributor

lynxplay commented Dec 1, 2024

Somewhat meh on the concept, not the biggest fan nor hater.
If people are coding such critical stuff, they should be able to just wrap in try-catch.
Like, I just don't see how often this would actually be used in the wild? Over just try-catch, which also gives you proper control of any potential exceptions.

@Machine-Maker
Copy link
Member

Machine-Maker commented Dec 7, 2024

Yeah, I think people that want this should just be more explicit about it and just use try-catch. That's always going to be more flexible than a boolean on the annotation. Like what if I want it to cancel on exceptions unless some condition is met. Using code is more flexible. So I'm against this.

@mbax
Copy link
Contributor

mbax commented Dec 8, 2024

If you're going to add an annotation that requires manually adding it, whose only purpose is to effectively be try{/*code*/}catch(Exception ignored){event.setCancelled(true);}, why aren't you just try-catching? The only way this becomes a more-useful feature would be if it were a breaking change to enable it by default. (And I also don't like that idea, to be clear.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Awaiting review
Development

Successfully merging this pull request may close these issues.

5 participants