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

refactor: migrate OC_EventSource to dependency injection #38386

Merged
merged 1 commit into from Jun 3, 2023

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented May 21, 2023

  • Resolves: #

Summary

Test: Increase the version number in version.php and visit Nextcloud.
The update should work as before.

Tip: Set a breakpoint in OC_EventSource.send and watch the request in your browser.

image

TODO

  • ...

Checklist

@kesselb kesselb added this to the Nextcloud 28 milestone May 21, 2023
@kesselb kesselb requested review from a team, ArtificialOwl, icewind1991, Fenn-CS and ChristophWurst and removed request for a team May 21, 2023 21:15
@kesselb kesselb added 3. to review Waiting for reviews technical debt labels May 21, 2023
@kesselb kesselb force-pushed the dept-use-di-for-event-source branch from 77fdb97 to cab73bc Compare May 21, 2023 21:23
@kesselb kesselb self-assigned this May 21, 2023
@kesselb kesselb mentioned this pull request May 24, 2023
5 tasks
@kesselb kesselb requested a review from come-nc May 25, 2023 21:12
@kesselb kesselb force-pushed the dept-use-di-for-event-source branch from cab73bc to 08fd49b Compare May 25, 2023 21:13
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label May 26, 2023
* @since 8.0.0
* @deprecated 20.0.0 have it injected or fetch it through \Psr\Container\ContainerInterface::get
*/
public function createEventSource();
Copy link
Member

Choose a reason for hiding this comment

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

Fine to be removed because deprecated but needs docs

@kesselb kesselb force-pushed the dept-use-di-for-event-source branch from 08fd49b to 22e5898 Compare May 26, 2023 15:22
@kesselb
Copy link
Contributor Author

kesselb commented May 26, 2023

I overlooked that createEventSource always creates a new object.
Updated the service definition and set $shared = false for IEventSource.

Can't say if a new instance makes sense, but that was the old behavior.

@kesselb kesselb force-pushed the dept-use-di-for-event-source branch from 22e5898 to 922ea47 Compare May 26, 2023 15:41
@kesselb
Copy link
Contributor Author

kesselb commented May 26, 2023

PR for docs: nextcloud/documentation#10479

@kesselb
Copy link
Contributor Author

kesselb commented May 26, 2023

Example: Rello/audioplayer#586

Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

In this instance I find it clearer to use createEventSource, as it makes it clear this is not a singleton. What are the pros of DI for non-singletons?

@kesselb
Copy link
Contributor Author

kesselb commented May 30, 2023

In this instance I find it clearer to use createEventSource, as it makes it clear this is not a singleton. What are the pros of DI for non-singletons?

I find that a valid point.
My motivation was to inject IRequest via DI and replace OC::$server->getRequest().

@ChristophWurst you deprecated it a while ago. Are you okay to keep createEventSource?

Should we also add createEventSource to https://github.com/nextcloud/server/blob/master/lib/public/Server.php so apps can use it without using a private api?

@icewind1991
Copy link
Member

Might be worth adding an EventSourceFactory that can be injected. Same pattern as we use for http clients and others

@ChristophWurst
Copy link
Member

Yes let's go with a injectable and mockable factory instead of a static function

@kesselb kesselb force-pushed the dept-use-di-for-event-source branch 2 times, most recently from 581e7d8 to 43d7ab6 Compare June 1, 2023 16:10
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Clean

@kesselb kesselb force-pushed the dept-use-di-for-event-source branch from 43d7ab6 to e6073f1 Compare June 2, 2023 16:01
Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb kesselb force-pushed the dept-use-di-for-event-source branch from e6073f1 to a2afc7b Compare June 3, 2023 19:43
@icewind1991 icewind1991 merged commit f5932eb into master Jun 3, 2023
36 of 37 checks passed
@icewind1991 icewind1991 deleted the dept-use-di-for-event-source branch June 3, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews pending documentation This pull request needs an associated documentation update technical debt
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants