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

Correctly gets new hooks after a reload. #14

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

BradBouquio
Copy link

The reload command instantiated a new HookManager without setting up new hooks. This was causing challenges to not award money after a reload.

@Muspah
Copy link
Member

Muspah commented Jan 29, 2021

I usually don't test reload cases because it's strongly advised to not use this function at all, but does this really fix the issue? The hooks should be set-up in the scheduled task just like a regular onEnable call (server startup). If that isn't the case, the other stuff handled in that task isn't called either, right?

Almost 4AM here, will look into it further later.

@BradBouquio
Copy link
Author

Yes the hooks are set up in a task in the onEnable call. When the reload command is run, everything else was handled properly like re-registering event and commands, but for whatever reason the hookManager was just set to a new blank instance. This definitely fixed the issue for me. Before this, economy integration stopped working completely after a reload and it's working fine now.

@Muspah
Copy link
Member

Muspah commented Jan 29, 2021

I'd prefer to do some additional tests to see what actually happens, it sounds a bit strange to me at first sight. Not a big fan of just duplicating these lines, especially because it adds the risk of forgetting to add new hooks in one of two places in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants