Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use StringArgumentType#string() for ServerCommand server argument #1363

Open
wants to merge 1 commit into
base: dev/3.0.0
Choose a base branch
from

Conversation

willkroboth
Copy link

Resolves #1362

Currently, the /server command uses StringArgumentType#word for its argument. This argument only accepts a limited set of characters, as defined by Brigadier's StringReader#isAllowedInUnquotedString method. It is possible to give a backend server a name that contains characters outside of this set, making that server unusable with the /server command.

This PR changes the /server command to use the StringArgumentType#string argument type. This argument accepts unquoted strings like word, so previously valid commands are still valid. However, any characters can now be entered, as long as the argument is surrounded by ".

The suggestion provider for the argument was updated to suggest quotes around server names that need to be quoted. If the user places a quote at the start of the argument, then all the server names are suggested surrounded by quotes. If the user types a character other than a quote, names that require quotes are not suggested.

I wasn't sure if I should write automated tests for this new code. I didn't see any tests for the functionality of builtin Velocity commands, so I decided not to add any suggestions tests in this PR.

I also noticed that the /glist command and the /send command also use StringArgumentType#word to accept a target server name. Should those commands be updated to use StringArgumentType#string as well? Also, would it be a good idea to somehow share the code for the server name suggestions provider, rather than duplicating (mostly, glist also suggests "all") the same logic across these classes?


As an alternative solution, I believe StringArgumentType#greedy would be appropriate here. This is the last argument in the command, and the string argument type allows the user to input arbitrary strings anyway. On the user side, using a greedy string would mean they wouldn't need to put quotes around the server name. On the code side, the suggestions provider wouldn't have to worry about handling server names that are not valid unquoted strings. However, #1362 (comment) alluded to the idea that a greedy string here may be problematic ¯\(ツ)/¯.

As another alternative solution, I suppose Velocity could prohibit server names that are not unquoted strings. #1362 (comment) suggests that this is a niche setup that often causes other issues anyway. However, I imagine this isn't really an acceptable solution for users whose language doesn't exclusively use the letters a-z.

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.

The /server command cannot be used for Chinese name servers
1 participant