Skip to content

Fix CommandListener ignoring Plugin_Handled#1819

Merged
peace-maker merged 1 commit intoalliedmodders:masterfrom
sirdigbot:cmdlistener-handled
Apr 19, 2023
Merged

Fix CommandListener ignoring Plugin_Handled#1819
peace-maker merged 1 commit intoalliedmodders:masterfrom
sirdigbot:cmdlistener-handled

Conversation

@sirdigbot
Copy link
Contributor

@sirdigbot sirdigbot commented Aug 11, 2022 •
edited
Loading

Resolves #1363 (I think?)

I'm not entirely sure if this is the correct way to fix this.
But it works, and I couldn't find any other instances where RETURN_META(MRES_SUPERCEDE); or RETURN_META_VALUE(MRES_SUPERCEDE, ... were used that were using Pl_Stop instead of Pl_Handled

Tested with the same code as the issue:

#include <sourcemod>

public void OnPluginStart()
{
  AddCommandListener(CommandListener_Noclip, "sm_noclip");
}

Action CommandListener_Noclip(int client, const char[] command, int argc)
{
  if (IsPlayerAlive(client) && argc < 1) {
  	ReplyToCommand(client, "[SM] aaaa");
  	return Plugin_Handled;
  }
  return Plugin_Continue;
}

fix

@sirdigbot sirdigbot mentioned this pull request Aug 11, 2022
Closed
6 tasks
@asherkin
Copy link
Member

asherkin commented Mar 13, 2023

Sorry for the delay, not sure how this one snuck by! As a code fix, this is definitely correct, and is what the behaviour should be.

I'm wary of the bcompat change, it'll need a quick validation on the corpus and I'm not sure how difficult that'll be to do. That said, I could easily be persuaded to just land it for the next release from master, the doc behaviour is sane and any incorrect plugins will fail-closed, so there should be no security impact.

@peace-maker
Copy link
Member

peace-maker commented Apr 19, 2023

I'd expect plugin authors to trust the documentation and not verifying if Plugin_Handled actually prevented it. If they did check and switched to Plugin_Stop, this change won't hurt them. If you rely on Plugin_Handled still calling the original command dispite the docs telling otherwise, it's not really something we should support. The docs should be the accountable source here.

🚢

@peace-maker peace-maker merged commit 426c1a0 into alliedmodders:master Apr 19, 2023
@sirdigbot sirdigbot deleted the cmdlistener-handled branch May 27, 2023 05:30
StarterX4 pushed a commit to StarterX4/sourcemod that referenced this pull request Aug 2, 2023
@sirdigbot @StarterX4
Fix CommandListener ignoring Plugin_Handled (alliedmodders#1819)
0e136e4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asherkin asherkin asherkin approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CommandListener documentation LIES about Plugin_Handled

3 participants

@sirdigbot @asherkin @peace-maker