Skip to content

refactor(dbal): move from deprecated to modern calls#58891

Open
solracsf wants to merge 1 commit intomasterfrom
executeUpdateRemove
Open

refactor(dbal): move from deprecated to modern calls#58891
solracsf wants to merge 1 commit intomasterfrom
executeUpdateRemove

Conversation

@solracsf
Copy link
Copy Markdown
Member

@solracsf solracsf commented Mar 12, 2026 •
edited
Loading

Summary

Give Doctrine DBAL some love 💘 by moving away from deprecated calls and a few RAW queries.

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@solracsf solracsf added this to the Nextcloud 34 milestone Mar 12, 2026
@solracsf solracsf added the feature: database Database related DB label Mar 12, 2026
@solracsf solracsf changed the title chore(dbal): move to executeStatement chore(dbal): WiP Mar 12, 2026
@solracsf solracsf added 2. developing Work in progress ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) labels Mar 12, 2026
@solracsf solracsf force-pushed the executeUpdateRemove branch from e9754ab to 2a6989f Compare March 13, 2026 06:25
@solracsf solracsf changed the title chore(dbal): WiP chore(dbal): refactor to modern calls Mar 13, 2026
@solracsf solracsf added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 13, 2026
@solracsf solracsf changed the title chore(dbal): refactor to modern calls refactor(dbal): move from deprecated to modern calls Mar 13, 2026
@solracsf solracsf marked this pull request as ready for review March 13, 2026 07:16
@solracsf solracsf requested review from ArtificialOwl, leftybournes, nfebe and sorbaugh and removed request for a team March 13, 2026 07:16
@solracsf solracsf force-pushed the executeUpdateRemove branch 2 times, most recently from b8d7a73 to 21416b8 Compare March 13, 2026 08:20
@solracsf
refactor(dbal): move to modern calls
21416b8 
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
SebastianKrupinski
SebastianKrupinski approved these changes Mar 15, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@SebastianKrupinski SebastianKrupinski left a comment

Choose a reason for hiding this comment

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

CalDav looks fine

miaulalala
miaulalala reviewed Mar 16, 2026
View reviewed changes
lib/private/DB/ArrayResult.php
Comment on lines +68 to +69
$row = array_shift($this->rows);
return $row !== null ? current($row) : false;
Copy link
Copy Markdown
Contributor

@miaulalala miaulalala Mar 16, 2026

Choose a reason for hiding this comment

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

Why this change? Same for the other changes from current below. Is there a performance improvement?

Copy link
Copy Markdown
Member Author

@solracsf solracsf Mar 16, 2026

Choose a reason for hiding this comment

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

If a row is [0] or ['count' => '0'], the condition is false and the method returns false even though a valid row was fetched. The replacement uses !== null which correctly distinguishes "no row left" from "row with falsy content."

Copy link
Copy Markdown
Contributor

@miaulalala miaulalala Mar 17, 2026

Choose a reason for hiding this comment

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

that makes sense but seems like this constitutes a change in the current behaviour, thus I'd remove it from this refactoring.

Copy link
Copy Markdown
Member Author

@solracsf solracsf Mar 17, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

"It's not a bug, it's a feature!" :)
Please revert it, if this is expected (even if CI doesn't complain about it). 👍

Copy link
Copy Markdown
Contributor

@miaulalala miaulalala Mar 17, 2026

Choose a reason for hiding this comment

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

Fair 😉

miaulalala
miaulalala requested changes Mar 17, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@miaulalala miaulalala left a comment

Choose a reason for hiding this comment

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

as per my other comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miaulalala miaulalala miaulalala requested changes

@SebastianKrupinski SebastianKrupinski SebastianKrupinski approved these changes

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

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

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

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl is a code owner automatically assigned from nextcloud/server-backend

@nfebe nfebe Awaiting requested review from nfebe nfebe is a code owner automatically assigned from nextcloud/server-backend

@leftybournes leftybournes Awaiting requested review from leftybournes leftybournes is a code owner automatically assigned from nextcloud/server-backend

@sorbaugh sorbaugh Awaiting requested review from sorbaugh sorbaugh is a code owner automatically assigned from nextcloud/server-backend

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

3. to review Waiting for reviews feature: database Database related DB ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring)

Projects

None yet

Milestone

Nextcloud 34

Development

Successfully merging this pull request may close these issues.

3 participants

@solracsf @miaulalala @SebastianKrupinski