-
-
Notifications
You must be signed in to change notification settings - Fork 647
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
Allow responding to cookie requests #1453
base: dev/3.0.0
Are you sure you want to change the base?
Conversation
Need to update the toString inside of the event |
Applied the requested changes |
@@ -68,15 +69,19 @@ public String toString() { | |||
*/ | |||
public static final class ForwardResult implements Result { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would much prefer a separate RespondResult
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it wouldn't make sense with getResult(). Also, since we're responding on behalf of the player, we also answer on how this should be forwarded, as we can't allow 2 responses (and the client always responds)
Currently, if a server is awaiting the cookie response, it is impossible to block the request without forcing the server to wait indefinably, as https://jd.papermc.io/velocity/3.4.0/com/velocitypowered/api/event/player/CookieRequestEvent.ForwardResult.html#handled() just discards any requests without sending any responses and there is no api to manually send a response.
The current approach is similar to https://jd.papermc.io/velocity/3.4.0/com/velocitypowered/api/event/player/ServerLoginPluginMessageEvent.html, but if you consider that instead of this, it would be better to add a method in https://jd.papermc.io/velocity/3.4.0/com/velocitypowered/api/proxy/ServerConnection.html to send a response then I would change it