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.

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

Refactor OC\Server::getDatabaseConnection #40121

Closed
wants to merge 1 commit into from
Closed

Refactor OC\Server::getDatabaseConnection #40121

wants to merge 1 commit into from

Conversation

summersab
Copy link
Contributor

This PR refactors the deprecated method OC\Server::getDatabaseConnection and replaces it with OC\Server::get(\OCP\IDBConnection::class) throughout the entire NC codebase (excluding ./apps and ./3rdparty).

Additionally, where necessary, the \OCP\IDBConnection class is imported via the use directive.

@summersab summersab force-pushed the refactor/OC-Server-getDatabaseConnection branch 2 times, most recently from 6866e69 to 57ab25c Compare August 30, 2023 02:29
@solracsf solracsf added the 3. to review Waiting for reviews label Aug 30, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone Aug 30, 2023
@summersab summersab force-pushed the refactor/OC-Server-getDatabaseConnection branch from 57ab25c to c613819 Compare August 30, 2023 15:01
ChristophWurst
ChristophWurst reviewed Sep 11, 2023
View reviewed changes
lib/private/Files/Cache/Cache.php Outdated
@@ -123,7 +123,7 @@ public function __construct(IStorage $storage) {

$this->storageCache = new Storage($storage);
$this->mimetypeLoader = \OC::$server->getMimeTypeLoader();
$this->connection = \OC::$server->getDatabaseConnection();
$this->connection = \OC::$server->get(IDBConnection::class);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->connection = \OC::$server->get(IDBConnection::class);
$this->connection = \OCP\Server::get(IDBConnection::class);

let's use the public server container API. same applies to most changes below

Copy link
Contributor Author

@summersab summersab Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I forgot to make this change before pushing, and I'm currently running tests locally. I don't know if you can cancel the tests that are currently running to free up resources, but feel free.

Also, why the change to the public container? In general, when should this be used?

Copy link
Member

Choose a reason for hiding this comment

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

In general, when should this be used?

In new code: always
In old code: when you touch it anyways

@summersab summersab force-pushed the refactor/OC-Server-getDatabaseConnection branch from c613819 to acaf690 Compare September 18, 2023 16:09
@summersab
Copy link
Contributor Author

I'm getting the following failure:

1) OCA\DAV\Tests\unit\Connector\Sabre\FilesReportPluginTest::testPrepareResponses
Failed asserting that null matches expected 200.

./server/apps/dav/tests/unit/Connector/Sabre/FilesReportPluginTest.php:445

Since this has to do with the dav app and not the core, how should I address this? I DO plan to refactor apps once my PRs for the core are approved. This seems a bit like a chicken and egg situation.

@summersab summersab force-pushed the refactor/OC-Server-getDatabaseConnection branch from acaf690 to 2cd272a Compare September 18, 2023 17:42
@summersab
Refactor OC\Server::getDatabaseConnection
e6f01b3 
Signed-off-by: Andrew Summers <18727110+summersab@users.noreply.github.com>

implement suggested changes from nickvergessen

Use the public server container API
@summersab summersab force-pushed the refactor/OC-Server-getDatabaseConnection branch from 2cd272a to e6f01b3 Compare September 18, 2023 17:48
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
Merged
This was referenced Nov 6, 2023
Merged
Merged
This was referenced Nov 14, 2023
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
This was referenced Mar 12, 2024
Merged
Merged
Merged
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
Merged
@Altahrim Altahrim mentioned this pull request Mar 25, 2024
Merged
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
Merged
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv
Copy link
Member

Too much conflicts, closing until this is done fresh again

@skjnldsv skjnldsv closed this May 30, 2024
@skjnldsv skjnldsv removed this from the Nextcloud 30 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickvergessen nickvergessen nickvergessen left review comments

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@miaulalala miaulalala Awaiting requested review from miaulalala miaulalala is a code owner

@st3iny st3iny Awaiting requested review from st3iny st3iny is a code owner

Assignees
No one assigned
Labels
3. to review Waiting for reviews technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@summersab @skjnldsv @nickvergessen @ChristophWurst @blizzz @solracsf