Repository navigation
Conversation
There was a problem hiding this comment.
Thanks for this PR! :)
The original issue misconstrued the problem a bit, this isn't the recommended nor documented way to unset flags on a region, but worked due to some legacy parsing code that happened to line up with command handling. As far as I can tell, some LLMs hallucinated none as the correct keyword to unset a flag, which led to people starting to use the wrong command.
IMO the goal here should be to prevent it from actually erroring when using this alternate command, rather than changing any behaviour beyond that point. Currently this PR uses different messaging when setting a flag to the none value compared to unsetting a flag, and ideally the none value should be treated the same as the intended command to unset a flag. I've left a comment on a way that I feel would fix that side of things :)
| try { | ||
| value = setFlag(existing, foundFlag, sender, value).toString(); | ||
| Object parsedValue = setFlag(existing, foundFlag, sender, value); | ||
| value = parsedValue == null ? "none" : parsedValue.toString(); |
There was a problem hiding this comment.
This change is treating it like the value "none" is being set on the region, whereas the flag is being removed. This should ideally trigger a message such as the one on like 621 (https://github.com/EngineHub/WorldGuard/pull/2307/changes#diff-cd6887d878ad3f9bebfe64edf07696bb25bb4f33a91877bf814acefd800243caL621). So maybe rather than just subbing in the word "none", setting value to null so that it can be better handled with proper messaging later on.
There was a problem hiding this comment.
Updated in 79560ed. A null parse result now goes through the existing unset branch, including the removed message and group cleanup. Explicit -g and -h behavior is preserved. All 177 core tests and Checkstyle pass.
|
Thanks, that distinction makes sense. |
7cb9473 to
79560ed
Compare
When flag parsing returns null (for example,
pvp none), keep the value null and use the existing unset branch. This avoids the exception and gives the same feedback and group cleanup as omitting the value.Fixes #2305.