Always inject player for player events

[Pieter12345]

Prevent player events from firing for offline players in edge cases where players have just left the server.

[PseudoKnight]

As injection is implemented, this will likely cause more problems than fix. The only offline player errors that I'm still aware of would actually be exacerbated by this -- an event within an event. I wrote a solution that only injects on join and uninjects on quit, but there are still other issues to consider such that I never considered it a complete solution.

[Pieter12345]

As injection is implemented, this will likely cause more problems than fix. The only offline player errors that I'm still aware of would actually be exacerbated by this -- an event within an event. I wrote a solution that only injects on join and uninjects on quit, but there are still other issues to consider such that I never considered it a complete solution.

Thinking about it more broadly, do you think that storing the player reference in the event handler environment would solve these issues?

[PseudoKnight]

Thinking about it more broadly, do you think that storing the player reference in the event handler environment would solve these issues?

That would only directly fix player() and not when using player names for arguments.

The only known issue right now is teleporting players cross worlds particularly on player_quit, since this causes a player_spawn event, and will uninject the player for the remainder of that player_quit bind. (I think this should be able to be worked around for now by putting the teleport at the end of the script, until this is fixed)

[Pieter12345]

The reason for this PR was this case, where I'm assuming a player_command event was fired after the player had already been removed from the player list:

[Pieter12345]

Rebased PR on master and implemented nested event player injection support. Running live on my server to see whether it solves the issue for which this PR was made.

[PseudoKnight]

That's a good solution to the nesting issue, but I think we need to go back to the original problem, which I'll try to lay out here.

During certain events that include the 'player' key, the player object cannot be retrieved by name from the server's player list. Sometimes this can be expected, as with player_login, while other times it's unexpected, as with player_spawn. At some point we decided to use the injected players map to store these references, even though it was designed for capture_runas(). Maybe this helped back then, but today Player.isOnline() checks if the player exists in the player list, so even if we could pass this player object to functions, they'd still be considered offline and throw an exception in Static.GetPlayer(). This means the only thing injection is doing right now is helping add the player to the environment in a roundabout fashion. This could be done better by just checking if the underlying event is an MCPlayerEvent, with a fallback for other events with 'player' that don't need injection. It also turns out the player is in the player list during player_join and player_quit events these days, so injection isn't even needed there anyway.

There's one additional wrinkle. Player.isOnline() checks the player list by UUID, not name, and it turns out there's one event where these maps are desynced: player_spawn. This is the one event where injection DOES help, since the player would be online despite not being able to fetch them by name from the player list. I'm wondering if a workaround to this could be added to the abstraction implementation instead, since I would consider this a bug in the server implementation.

Regarding player_command after a player quits, I believe that was fixed in Craftbukkit.
https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/commits/01b2e1af41a8698d54437d275b2e7d41014d5d81

[PseudoKnight]

I pushed some changes based on my updated understanding of the issue. This still leaves some unresolved questions, but it does fix event injection nesting issues.

Unresolved issues:

• AbstractEvent is not a great place to insert the player into the environment as it mixes MC and MethodScript stuff. I chose to keep it here for now because it avoids other complications, including extension compatibility.
• The player_spawn event bug should maybe be worked around in the Bukkit implementation layer instead of injection, so injection can go back to a single purpose.
• Nesting could still be an issue in the event that someone uses capture_runas() inside a player_spawn event bind. This was an issue before, too, but for all injected events. Now it can only happen with player_spawn.
• It's not clear what kind of functions should work on a player that is offline, like with player_login.

[Pieter12345]

Thanks for pushing the changes. It sounds like that makes this PR obsolete. Should I close this?

[PseudoKnight]

It does make the PR obsolete, but not the previously mentioned issues surrounding this subject. If you close this, I could open up an issue to track this conversation.

[Pieter12345]

Sounds good. I'll let you create an issue for this. You can close this PR when that's done.

[PseudoKnight]

Continued in issue #1370