Bentobox: Invalid usage of event API

[TPGamesNL]

Description

Describe the bug

Because BentoBox uses the Bukkit event API wrongly, registered listeners are called for events they didn't sign up for.

Steps to reproduce the behavior

Register a listener for an event, such as BentoBoxReadyEvent, not using the @EventHandler annotation, but by registering your own EventExecutor, with this method.
This listener (/ executor) will be called for all of BentoBox's events, not just the BentoBoxReadyEvent that it was registered to listen to.
Example code:

Listener listener = new Listener() { };
EventExecutor eventExecutor = (execListener, event) -> Bukkit.broadcastMessage("Event " + event + " called");
Bukkit.getPluginManager().registerEvent(BentoBoxReadyEvent.class, listener, EventPriority.NORMAL, eventExecutor, this);

Expected behavior

The listener (/ executor) should only be called for the event it was registered to listen to.

Additional context (Optional)

https://gist.github.com/TPGamesNL/ee79fc4c348721925e2b4cfafb1a9ea0
Because BentoBox's events use the abstract class BentoBoxEvent, which contains the required HandlerList, Bukkit sees all of BentoBox's events as one event: BentoBoxEvent, therefore registered listeners will be called for all of its events, not just the event it was registered to listen to.

[tastybento]

@TPGamesNL Do you have some code that does what you say and this is why you'd like this changed? Do you have a suggestion for how the code should change or a better approach?

[BONNe]

Handler list should be per each event, instead single global in abstract bentobox event

[TPGamesNL]

Code:

Listener listener = new Listener() { };
EventExecutor eventExecutor = (execListener, event) -> Bukkit.broadcastMessage("Event " + event + " called");
Bukkit.getPluginManager().registerEvent(BentoBoxReadyEvent.class, listener, EventPriority.NORMAL, eventExecutor, this);

A broadcast will appear for every BentoBox event, not just when BentoBox is ready.

How to fix:
Each seperate event class should have it's own HanderList field and getHandlers() and getHandlerList() methods. Example:

public class BentoBoxReadyEvent extends Event {

    private static final HandlerList handlers = new HandlerList();

    @Override
    public HandlerList getHandlers() {
        return getHandlerList();
    }

    public static HandlerList getHandlerList() {
        return handlers;
    }

}

Or, if you want a superclass for all of BentoBox's events:

public class BentoBoxReadyEvent extends BentoBoxEvent {

    private static final HandlerList handlers = new HandlerList();

    @Override
    public HandlerList getHandlers() {
        return getHandlerList();
    }

    public static HandlerList getHandlerList() {
        return handlers;
    }

}

public class BentoBoxEvent extends Event { }

PS I noticed https://github.com/BentoBoxWorld/BentoBox/blob/develop/src/main/java/world/bentobox/bentobox/api/events/command/CommandEvent.java#L11 is for wrong class

[tastybento]

So the root cause of this seems to be that static field handlers. Does anyone know if the static method getHandlerList() is mandatory or just convention? If that wasn't required, then a static handlers would not have to be declared for each event.

[tastybento]

Okay, to answer my own question, apparently it does need to be included. From https://bukkit.gamepedia.com/Event_API_Reference#Custom_Event_Example

[BONNe]

Well, due to API usage, we cannot just change it directly...
It should be deprecated and used correctly and update in each addon.

I would say we need at least 1 BentoBox release with "deprecation".

[tastybento]

Well, due to API usage, we cannot just change it directly...
It should be deprecated and used correctly and update in each addon.

I would say we need at least 1 BentoBox release with "deprecation".

Yes, I'll mark the current BentoBoxEvent as deprecated for 1.15.4 and then 1.16.0 can have the new version.

[tastybento]

This issue should be resolved now with 1.15.4. If there's still a problem, let me know.