Allowing custom command to handle raw command line inputs

[NTT1906]

Current PMMP handles the parsing of command lines in a hardcoded way without any non-hacky method for commands to handle the raw inputs. This PR tweaks the API a little so it will allow custom commands to do such things without harming the performance.

Related issues & PRs

• Related to Allowing custom command to handle/parse the command line by itself. #6704

Changes

API changes

• Added a new method named Command->executeRaw() to Command class. This allowing custom command to handle the raw command inputs by override the parent method.

Backwards compatibility

Yes

[dktapps]

I feel like the SimpleCommandMap changes are more than what's needed. The diff shouldn't need to be so many lines.

[NTT1906]

I feel like the SimpleCommandMap changes are more than what's needed. The diff shouldn't need to be so many lines.

• Eh... Isn't the result check of preg_split return shifted a big part of the code? and I don't know if there exists any cases of it return false.
  Edit: there is :l

$cmd = "\xC3"; // invalid UTF-8 byte
var_dump(preg_split('/\s+/u', $cmd, 2)); //  false

• I did set $sentCommandLabel to "", as they got null coalesced as "" in the later. If you don't want that then I will revert it back to null.

Also, wala - PHP magic

$parts = preg_split('/\s+/u', $commandLine, 2) ?: [""]; // ensure the $parts will always be an array of string
$sentCommandLabel = $parts[0];
if(!empty($sentCommandLabel) && ($target = $this->getCommand($sentCommandLabel)) !== null){
	$timings = Timings::getCommandDispatchTimings($target->getLabel());
	$timings->startTiming();

	try{
		if($target->testPermission($sender)){
			$target->executeRaw($sender, $sentCommandLabel, $parts[1] ?? "");
		}
	}catch(InvalidCommandSyntaxException $e){
		$sender->sendMessage($sender->getLanguage()->translate(KnownTranslationFactory::commands_generic_usage($target->getUsage())));
	}finally{
		$timings->stopTiming();
	}
	return true;
}

$sender->sendMessage(KnownTranslationFactory::pocketmine_command_notFound($sentCommandLabel, "/help")->prefix(TextFormat::RED));
return false;

[dktapps]

You're overthinking it. A simple explode() call would suffice.

[NTT1906]

You're overthinking it. A simple explode() call would suffice.

The orginal code CommandStringHelper::parseQuoteAware() used regex to split what ever regex treated as space (\s) with flag unicode turned on. Using explode would be simple but that will only do that for ( , \t, manual listing...).

preg_splits was there to keep up the consistency with CommandStringHelper::parseQuoteAware()

Kinda unrelated to this PR, but if only ascii whitespace is acceptable - I would like for CommandStringHelper::parseQuoteAware() to only accept ascii whitespace. Current regex accept all ascii spaces and all unicode spaces...

[dktapps]

This is really overcomplicating it. All that's needed is to explode by spaces into 2 parts. Trim the parts and be done with it. No one is putting a tab after the command name.

[dktapps]

Looks mostly OK now

[ShockedPlot7560]

To be noted, if we changed to that, we probably need to switch execute to protected. If its public, This suggests that one or other could be called whereas the plugin can implement either.
BC is keeped with the execute (while plugins don't override executeRaw) but execute can be called alone without executeRaw.

[NTT1906]

To be noted, if we changed to that, we probably need to switch execute to protected. If its public, This suggests that one or other could be called whereas the plugin can implement either.

Well, eh... Ain't those devs decided to code it that way? Maybe adding some docs?

BC is keeped with the execute (while plugins don't override executeRaw) but execute can be called alone without executeRaw.

I don't think Command::execute() being called alone is a problem as they still provide args as array.
The whole point of this Command::executeRaw() to me is not an alternative version of execute but making the handling of raw args possible.

[ShockedPlot7560]

I share your point of you in the handling of raw args. But, we can't rely on only docs where a creepy dev can create a plugin calling execute by its way skipping all rawArgs calls
Anyway, with your implementation, for me, breaking BC is needed by switching visibility
Maybe we can find a better way because I'm not a big find breaking a such large codebase.

[NTT1906]

I share your point of you in the handling of raw args. But, we can't rely on only docs where a creepy dev can create a plugin calling execute by its way skipping all rawArgs calls

As I mentioned before, that creepy dev already coded it like that. If you want to stop such practices, you can only stop them from being released into productions or advise them using SimpleCommandMap::dispatch() or Command::executeRaw().

Anyway, with your implementation, for me, breaking BC is needed by switching visibility. Maybe we can find a better way because I'm not a big find breaking a such large codebase.

Previously in issue #6704, executeRaw() my suggestion was just a simple parse function like Command::parseQuoteAware(). It returns args from rawArgs, then passes those args to execute and still allows for custom handling of rawArgs.

And I still don't think it's a major problem, and at its worst only causing some stupid behaviors for those practicing it.
The syntax checking should be coded to execute in most cases; you cannot stop those creepy devs anyway.

[dktapps]

Well, the other option that was originally proposed is having the command expose a function that parses the args to an array. But I don't really like that approach since it still falls down to string[].

Doing it with executeRaw() would allow implementing, for example, #6518 inside a custom command type. So it definitely has its upsides.

[dktapps]

Looks OK in principle, how I envisioned it. However I don't have a good answer to the concern raised by @ShockedPlot7560 around how execute() should be treated following this change. It probably should become protected but that will break BC, so I'm not sure how it should be handled.

no longer valid

[ShockedPlot7560]

With our internal discussions against this PR, we don't really have better design than this for the moment. As it's planned to rework the command design, maybe, it should be OK to merge as it is. But I'm not a really big fan of this solution by my concern raised in the past and not a big fan to merge as it is too.
Should this PR be merged, closed or staled awaiting a rework ?

[dktapps]

Perhaps we should save it for PM6?

[dktapps]

Worth noting this is a bit of a pain for stuff like FormattedCommandAlias, since we may want to be able to pass pre-parsed arguments to commands as well. Not clear how all these pieces should fit together. See also #4193 and

PocketMine-MP/src/command/FormattedCommandAlias.php

Lines 88 to 92 in 335fcc8

	//this approximately duplicates the logic found in SimpleCommandMap::dispatch()
	//this is to allow directly invoking the commands without having to rebuild a command string and parse it
	//again for no reason
	//TODO: a method on CommandMap to invoke a command with pre-parsed arguments would probably be a good idea
	//for a future major version

[dktapps]

Superseded by #6837.

Since this would require BC break anyway (otherwise the whole execute vs executeRaw issue mentioned earlier would be a headache), it makes more sense to just go all-in on reworking the command system fully to maximise the gain for the BC breaks. There's a bunch of other issues that that solves too.

Thanks for making an effort to contribute, though!