Skip to content

Decouple resource provider registration #17325

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.

Sign up for GitHub

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

Jump to bottom
Merged
ChristophWurst merged 1 commit into from
Dec 9, 2019
Merged

Decouple resource provider registration #17325

ChristophWurst merged 1 commit into from
Dec 9, 2019

Conversation

@kesselb
Copy link
Contributor

@kesselb kesselb commented Sep 29, 2019 •
edited
Loading

server/apps/files/lib/AppInfo/Application.php

Lines 88 to 89 in a0d48c5

$resourceManager = $container->query(IManager::class);
$resourceManager->registerResourceProvider(ResourceProvider::class);

Apps can register their own ResourceProvider. For now they get a OC\Collaboration\Resources\Manager instance with IDBConnection as dependency. But a database connection is not required for registerResourceProvider. This pr extract the register and provider resolver functionality. I don't know if there are some naming conventions. Feel free to suggest better names ;)

Discovered this while debugging #13712.

@kesselb kesselb force-pushed the enh/decouple-register-resource-provider branch 3 times, most recently from e325b69 to 67627b7 Compare September 29, 2019 22:34
ChristophWurst
ChristophWurst reviewed Sep 30, 2019
View reviewed changes
lib/public/Collaboration/Resources/IManager.php
/**
* @param string $provider
* @since 16.0.0
* @deprecated 18.0.0 Use IProviderManager::registerResourceProvider instead
Copy link
Member

@ChristophWurst ChristophWurst Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@ChristophWurst
Copy link
Member

ChristophWurst commented Sep 30, 2019

I like the idea. Small services are good services.

You'll get bonus points if you add tests 😉

nickvergessen
nickvergessen reviewed Sep 30, 2019
View reviewed changes
@kesselb kesselb force-pushed the enh/decouple-register-resource-provider branch from 222aaed to cf37455 Compare September 30, 2019 19:43
@kesselb kesselb marked this pull request as ready for review September 30, 2019 21:47
@kesselb kesselb added 3. to review Waiting for reviews enhancement labels Sep 30, 2019
ChristophWurst
ChristophWurst reviewed Oct 1, 2019
View reviewed changes
ChristophWurst
ChristophWurst requested changes Oct 1, 2019
View reviewed changes
@kesselb kesselb added this to the Nextcloud 18 milestone Oct 1, 2019
@kesselb kesselb requested a review from ChristophWurst October 2, 2019 14:35
ChristophWurst
ChristophWurst approved these changes Oct 2, 2019
View reviewed changes
ChristophWurst
ChristophWurst approved these changes Oct 3, 2019
View reviewed changes
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.

@kesselb kesselb force-pushed the enh/decouple-register-resource-provider branch from 645c6c0 to 0a560d0 Compare October 8, 2019 21:00
@kesselb kesselb requested a review from juliusknorr October 8, 2019 21:01
@kesselb kesselb force-pushed the enh/decouple-register-resource-provider branch from 0a560d0 to f32fe02 Compare October 12, 2019 14:05
@kesselb
Copy link
Contributor Author

kesselb commented Nov 1, 2019

Hello @rullzer @juliushaertl 🏓

juliusknorr
juliusknorr approved these changes Nov 4, 2019
View reviewed changes
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Nice 👍

@kesselb kesselb force-pushed the enh/decouple-register-resource-provider branch from f32fe02 to 3a81e22 Compare November 4, 2019 13:32
@ChristophWurst
Copy link
Member

ChristophWurst commented Nov 11, 2019

CI seems happy except for then the DB dies. Let's get this in!

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 11, 2019
@ChristophWurst
Copy link
Member

ChristophWurst commented Nov 11, 2019

@kesselb mind squashing the commits?

@kesselb kesselb force-pushed the enh/decouple-register-resource-provider branch 2 times, most recently from 43da4c9 to 2dc292d Compare November 11, 2019 17:37
@kesselb kesselb force-pushed the enh/decouple-register-resource-provider branch from 2dc292d to df51338 Compare November 13, 2019 18:40
@kesselb
Copy link
Contributor Author

kesselb commented Nov 13, 2019

CI don't like me today ;) Could someone of you press merge?

@kesselb kesselb force-pushed the enh/decouple-register-resource-provider branch from df51338 to 496af16 Compare December 3, 2019 09:43
nickvergessen
nickvergessen requested changes Dec 4, 2019
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

Some small little fixes needed because we updated phpunit in the meantime

@kesselb kesselb force-pushed the enh/decouple-register-resource-provider branch 2 times, most recently from 058534d to 3ad4295 Compare December 4, 2019 08:35
@kesselb kesselb force-pushed the enh/decouple-register-resource-provider branch from 3ad4295 to be54491 Compare December 4, 2019 13:30
@skjnldsv skjnldsv force-pushed the enh/decouple-register-resource-provider branch from be54491 to 3f20653 Compare December 7, 2019 08:51
@kesselb kesselb force-pushed the enh/decouple-register-resource-provider branch from 3f20653 to 0016480 Compare December 7, 2019 21:05
@ChristophWurst ChristophWurst merged commit 8bc4295 into master Dec 9, 2019
@ChristophWurst ChristophWurst deleted the enh/decouple-register-resource-provider branch December 9, 2019 08:51
@ChristophWurst ChristophWurst mentioned this pull request Dec 9, 2019
Closed
@rullzer rullzer mentioned this pull request Dec 11, 2019
Merged
43 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessen nickvergessen nickvergessen approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@juliusknorr juliusknorr juliusknorr approved these changes

@rullzer rullzer Awaiting requested review from rullzer

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish enhancement

Projects

None yet

Milestone

Nextcloud 18.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@kesselb @ChristophWurst @nickvergessen @juliusknorr