CommandSender enhancement

[PEMapModder]

Key

In this description, $sender always refers to a CommandSender object.

Fact 1

When a plugin receives a CommandSender from a command, it may have to store the instance temporarily to use it later (such as sending feedback after an AsyncTask completion, or to do other things).

Fact 2

Currently, plugins have to explicitly check if Player::isOnline() if they hold the Player instance and want to call hasPermission (and other functions too) upon the player, so as to avoid crashing.

Fact 2 is acceptable itself, but when it happens with Fact 1, this may result in a crash if the player left the server during the AsyncTask execution. To avoid this from happening, so far I have thought of two solutions.

First solution

function onCompletion(CommandSender $sender, string $myPerm, string $myMessage){
    if($sender instanceof Player and !$sender->isOnline()) return;
    // execute normal code
    if($sender->hasPermission($myPerm))
        $sender->sendMessage($myMessage);
}

Second "solution"

function onCompletion(WeakRef $senderRef, string $myPerm, string $myMessage){
    if(!$senderRef->valid()) return;
    // execute normal code
    if($sender->hasPermission($myPerm)){
        $sender->sendMessage($myMessage);
}

The problem

The second "solution" is not a real solution, because this method is highly unreliable -- if another plugin is holding a strong reference to the player instance, it would not work.

The first solution is so far the only reliable one I can think of, but it violates the principle of using interfaces. Functions that receive a parameter expecting the superclass/interface can legitimately ignore the underlying implementation, so onCompletion actually has the right to be not knowing whether $sender is a Player or not.

Moreover, since Player is implemented like this, this sets an example that future implementations of CommandSender, from PocketMine core or from plugins, can crash if they are unavailable, but obviously plugins that are only expecting a CommandSender (and maybe checking Player) would have no knowledge on that new implementation.

Suggestion

This exposes a need to properly implement a new CommandSender method, called isOnline or isAvailable or whatever, to check whether a CommandSender is still usable.

However, this breaks backwards compatibility for plugins as CommandSender is an interface that can be implemented by any plugins. Any plugins that have classes implementing CommandSender and logically don't have an isOnline method would crash.

Therefore, there may be the need of one and only one API interface, called maybe something like DisposableCommandSender, extending CommandSender and implemented by Player, that has an isOnline method (or other names that will be implemented in the Player class). In this way, plugins only have to check if $sender instanceof DisposableCommandSender and $sender->isOnline() without having to worry about further implementations.

[legoboy0215]

👍

[Samueljh1]

why not just save the senders name in the asynctask and then call $server->getPlayerExact() ?

[PEMapModder]

@Samueljh1 a command sender is not necessarily a player. For example, I would store the sender name "CONSOLE", but I cannot get the command sender instance back if I don't hold an object reference to it.

[Samueljh1]

I knew you would mention that. If it was the console, then why not do ->getLogger()->info() ?

[PEMapModder]

@Samueljh1 who told you that the command sender is either a player or console (or RCON, if you are arguing)? It can be any class, including classes that implement CommandSender, from plugins.

It can of course be done by hardcoding to detect player or console, but then the plugin won't know what to do if there is a third (OK, fourth) type of command sender. The meaning of an interface is that everyone can make a class that implements it and pass an instance of class to a function that expects an instance of that interface.

[SOF3]

This can be similarly used in contexts like Permissible. However, the condition for "disposed" may be different depending on context, so a further upper interface Disposable should or should not be created.