-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 share activity dispatch #27514
Fix share activity dispatch #27514
Conversation
b8faf10
to
07bee47
Compare
I'd say we get this in as a fix. Dropping the legacy hooks would be a bigger change that we should rather do early in the release cycle so we catch possibly missed usages early. Edit: #14552 for reference on the legacy event methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beside the nitpick this looks fine 👍
07bee47
to
e50e0d8
Compare
Signed-off-by: Louis Chemineau <louis@chmn.me>
e50e0d8
to
be42a13
Compare
Comment added. Ready to get merged :) |
A regression was introduced by #26727. This reverts one of the changes that causes share creation activities to not be dispatched.
This was caught by https://github.com/nextcloud/activity/pull/593/checks?check_run_id=2822075000.
Another solution would be to update the code to drop legacy API, but I don't want to dive into that without a more informed opinion.
FYI: It is probably due to a linter removing the variable because it is unused. We only need the side-effects from the constructor.
@icewind1991, @juliushaertl or @MorrisJobke ?