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

Issue warning, if possible, when behavior changes due to backend plugin handling #17

Open
A248 opened this issue Aug 31, 2023 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@A248
Copy link

A248 commented Aug 31, 2023

In the pre-existing Velocity API, canceling a chat event prevents it from being sent to the backend servers. Chat canceled on the proxy is therefore invisible to Bukkit plugins. However, using SignedVelocity fundamentally alters this behavior by forwarding all chat events, even if canceled, to the backend servers. While this makes the plugin work, it is a behavioral change from the prior status quo, and leaks the abstraction SignedVelocity employs to shore up chat handling in a post-1.19.4 world.

This can be confusing for users who are acquainted to the previous behavior. For example, it may cause certain proxy plugin features to stop working when switching from pre-1.19.4 to post-1.19.4 with SignedVelocity, since the behavior of chat events is now different.

Most of the time, however, uncanceling a canceled chat event doesn't make sense. Fortunately, SignedVelocity is in the unique position of knowing exactly when a chat event was canceled by the proxy but uncanceled by the backend server. I therefore propose SignedVelocity add another listener on EventPriority.HIGHEST to log a brief warning if behavior has fundamentally changed as a result of a backend plugin uncanceling the event. It should be relatively straightforward given you already have a queue of chat data, so no additional data structure overhead would be incurred. If you want to avoid annoying users, the brief warning can be one-time, or it can be disabled by a config option. However, I argue it should be enabled by default, because it presents a behavioral change from the typical Velocity API proxy plugins are used to.

@4drian3d 4drian3d added the bug Something isn't working label Aug 31, 2023
@4drian3d
Copy link
Owner

I missed commenting here, but the real problem here is that some plugins use deprecated chat events that are executed before Paper's AsyncChatEvent, so, it may be that a plugin handles the chat with a deprecated event before SignedVelocity checks if a message was modified from Velocity.
To mitigate this, I added a warning message in the console when starting the plugin mentioning plugins that use these legacy events.

@A248
Copy link
Author

A248 commented Sep 25, 2023

Adrian, I'd like to politely ask you to revisit my message. It's possible for plugins to listen only to the AsyncChatEvent yet still cause a behavioral inconsistency with respect to pre-1.19.4 chat handling.

An example of this would be a Paper plugin that listens to AsyncChatEvent on EventPriority.HIGHEST, cancels the event, and sends the formatted message to all players anyway. Before SignedVelocity, if the chat was canceled on the proxy, it would not reach the backend server, and no message would occur. After SignedVelocity and 1.19.4, if the chat was canceled on the proxy, it would still be sent to the Paper plugin.

@4drian3d 4drian3d reopened this Sep 25, 2023
@4drian3d
Copy link
Owner

4drian3d commented Nov 3, 2023

I will implement a system similar to the one I have in KickRedirect to detect these cases for the next version

@4drian3d 4drian3d self-assigned this Nov 3, 2023
@TheIceNinja
Copy link

TheIceNinja commented May 5, 2024

did u fix the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants