This repository has been archived by the owner. It is now read-only.

CommandSender enhancement #3919

Open
PEMapModder opened this Issue Feb 3, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@PEMapModder
Collaborator

PEMapModder commented Feb 3, 2016

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.

@PEMapModder PEMapModder changed the title from CommandSender API enhancement to CommandSender enhancement Feb 3, 2016

@legoboy0215

This comment has been minimized.

Show comment
Hide comment
@legoboy0215

legoboy0215 Feb 9, 2016

Contributor

👍

Contributor

legoboy0215 commented Feb 9, 2016

👍

@PEMapModder PEMapModder added the C: API label Feb 13, 2016

@Samueljh1

This comment has been minimized.

Show comment
Hide comment
@Samueljh1

Samueljh1 Feb 24, 2016

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

Samueljh1 commented Feb 24, 2016

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

@PEMapModder

This comment has been minimized.

Show comment
Hide comment
@PEMapModder

PEMapModder Feb 24, 2016

Collaborator

@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.

Collaborator

PEMapModder commented Feb 24, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@Samueljh1

Samueljh1 Feb 24, 2016

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

Samueljh1 commented Feb 24, 2016

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

@PEMapModder

This comment has been minimized.

Show comment
Hide comment
@PEMapModder

PEMapModder Feb 24, 2016

Collaborator

@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.

Collaborator

PEMapModder commented Feb 24, 2016

@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 comment has been minimized.

Show comment
Hide comment
@SOF3

SOF3 Oct 12, 2016

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.

SOF3 commented Oct 12, 2016

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.