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

Decouple RegisteredListener and Plugin/PluginManager #6524

Open
dktapps opened this issue Nov 19, 2024 · 0 comments
Open

Decouple RegisteredListener and Plugin/PluginManager #6524

dktapps opened this issue Nov 19, 2024 · 0 comments
Labels
BC break Breaks API compatibility Category: API Related to the plugin API Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP
Milestone

Comments

@dktapps
Copy link
Member

dktapps commented Nov 19, 2024

Description

It's currently basically impossible to use events without a Plugin context (and therefore a Server context).
This is because RegisteredListener requires a Plugin instance, even though it's only actually needed for decoration & for HandlerList::unregister().

In addition, it's non-trivial to construct a RegisteredListener, and the code needed to do so is baked into PluginManager::registerEvent(). That's before we even talk about Listener and the magic reflection detection.

Proposed changes

  • Find a way to remove the plugin requirement from RegisteredListener, either by making it nullable, changing it to a more general ?object $owner, or possibly getting rid of it entirely. It's only needed for this.
  • Pull the logic for PluginManager::registerEvent() out into somewhere with less dependencies (e.g. a static method in RegisteredListener or similar - enables the use of closure event handlers by core code
  • Pull the logic for PluginManager::registerEvents() out into somewhere in the pocketmine\event package without Plugin or Server dependencies - enables the use of Listener magic by core code

Justification

The server core code can't listen to its own events right now without a dummy Plugin instance, which comes with a bunch of unnecessary baggage.

We also can't easily unit-test the events system, because a Plugin context and therefore a Server context is required. We could probably mock this, but it's an unnecessary inconvenience.

Alternative methods

@dktapps dktapps added Category: API Related to the plugin API Category: Core Related to internal functionality BC break Breaks API compatibility Type: Enhancement Contributes features or other improvements to PocketMine-MP labels Nov 19, 2024
@dktapps dktapps added this to the 6.0 milestone Nov 19, 2024
dktapps added a commit that referenced this issue Nov 20, 2024
this is dodgy and we shouldn't rely on it long term.

relates to #6524
dktapps added a commit that referenced this issue Nov 20, 2024
this required mocking to get around #6524. Longer term we should make improvements to avoid the need for mocking here.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC break Breaks API compatibility Category: API Related to the plugin API Category: Core Related to internal functionality Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

No branches or pull requests

1 participant