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

fix: extension listeners added before core listeners #3373

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

SychO9
Copy link
Member

@SychO9 SychO9 commented Apr 1, 2022

Fixes #0000

Changes proposed in this pull request:
Currently, extension listeners are registered very early in the process, before core listeners, which seems unideal as extensions might expect core behavior to happen first in certain cases.

Reviewers should focus on:
That said, this may or may not break certain extension behaviors that might rely on the current order. So we might want to actually play it safe and do this in 2.0, need additional thoughts on this.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

@davwheat
Copy link
Member

davwheat commented Apr 2, 2022

If there is some use case for registering event handlers this early in the boot stage, might it be worth adding an "early listener" system which runs where this code originally was?

@SychO9
Copy link
Member Author

SychO9 commented Apr 2, 2022

If there is some use case for registering event handlers this early in the boot stage

There shouldn't be, similar cases have been switched out for extenders which is a more correct approach, unless I'm forgetting certain events, they should all be dispatched on the business logic level, where it doesn't matter when listeners are registered 🤔

@tankerkiller125
Copy link
Contributor

tankerkiller125 commented Apr 3, 2022

unless I'm forgetting certain events

Login events are the most notable along with registration events. And even then, they aren't very helpful for extension devs because if I remember correctly, we're missing some events that would be useful for things like account takeover protection were you need the results of the credentials check, but need to interfere before the user is actually signed in.

And even then I don't think we need early event registration for those?

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this since I agree that listening after boot is better than what we have now,and I also couldn't find any events that would be broken by this (and if there are any, they should be replaced with proper extenders). That being said, perhaps in the long term we should add support for listenAfter / listenBefore methods on the extender, for cases where priority does matter.

@SychO9
Copy link
Member Author

SychO9 commented Apr 6, 2022

And even then I don't think we need early event registration for those?

Indeed we don't, alright then merging.

@SychO9 SychO9 merged commit 98e607a into main Apr 6, 2022
@SychO9 SychO9 deleted the sm/event-listeners-on-boot branch April 6, 2022 15:53
@askvortsov1 askvortsov1 added this to the 1.3 milestone Apr 6, 2022
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.

4 participants