Skip to content

Conversation

@Yexeed
Copy link
Contributor

@Yexeed Yexeed commented Aug 30, 2024

This PR backports from upstream pull CloudburstMC/Nukkit#2196

tl;dr fixes command overriding which is not possible without this changes
sample code:

public static boolean unregisterCommand(Command command){
    command.setLabel(command.getName() + "__unregistered");
    return command.unregister(Server.getInstance().getCommandMap());
}

@lt-name
Copy link
Member

lt-name commented Aug 31, 2024

I think different judgments need to be made based on usage.
It should not be null when registering a command

@Yexeed
Copy link
Contributor Author

Yexeed commented Aug 31, 2024

I think different judgments need to be made based on usage.
It should not be null when registering a command

What? Just try to override a command without this PR, it wont work because this if-block was broken 8 years ago, mismatching "this.commandMap" with "commandMap". With this PR it's once again possible to override a command. However, why should we make some judjements about something THAT simple? Command SHOULD allow changes from foreign command map if:

  • the command itself doesnt have a valid command map attached to it (this.commandMap == null)
    or
  • the command has command map equal to passed command map (this.commandMap == commandMap)
    Its that simple

@Yexeed
Copy link
Contributor Author

Yexeed commented Aug 31, 2024

Also, registering a command (not overriding it) works fine

@lt-name
Copy link
Member

lt-name commented Sep 2, 2024

public boolean register(CommandMap commandMap) {
    if (this.allowChangesFrom(commandMap)) {
        this.commandMap = commandMap;
        return true;
    }
    return false;
}

I mean register(null) will return true
After this pr merge, although unregister is fixed, register will be broken

@Yexeed
Copy link
Contributor Author

Yexeed commented Sep 2, 2024

Null safety check should fix the problem?

@lt-name lt-name merged commit 671b3ed into MemoriesOfTime:master Sep 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants