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

feat(database): Add replacements for deprecated fetch and fetchAll #40655

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Sep 27, 2023

Summary

DBAL deprecated these and will remove them it in the future.

TODO

  • Add missing methods
  • Delegate deprecations

Checklist

DBAL deprecated these and will remove them it in the future.

Signed-off-by: Christoph Wurst <[email protected]>
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label Sep 27, 2023
lib/private/DB/ResultAdapter.php Fixed Show fixed Hide fixed
lib/private/DB/ResultAdapter.php Fixed Show fixed Hide fixed
lib/public/DB/IResult.php Outdated Show resolved Hide resolved
*
* @since 21.0.0
*/
public function fetchFirstColumn(): array;
Copy link
Member

Choose a reason for hiding this comment

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

should be mixed?

Copy link
Member Author

@ChristophWurst ChristophWurst Oct 5, 2023

Choose a reason for hiding this comment

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

it returns the first column of all results as array<mixed>

Copy link
Member

Choose a reason for hiding this comment

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

The description is not phrased clearly about that imo, and for better consistency perhaps consider renaming the method to fetchAllFirstColumns or something like that?

lib/public/DB/IResult.php Outdated Show resolved Hide resolved
@@ -57,15 +57,37 @@ public function closeCursor(): bool;
* @return mixed
*
* @since 21.0.0
* @deprecated 28.0.0 use fetchAssociative, fetchNumeric or fetchOne
Copy link
Member

Choose a reason for hiding this comment

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

This will be painful :D
Touching all the queries yet again (after query() replacement)

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Also use the deprecation line from the docs here explaining the default replacement?

Copy link
Member

Choose a reason for hiding this comment

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

@ChristophWurst Out of context, but I'm just curious to know if there are any plans to introduce Rector to the project. Now that I've touched a bit of code from different parts of the code base, I think it would be a great addition to the mix if we could make everything more consistent across the code base by using an automated refactoring approach with Rector.

@@ -57,15 +57,37 @@ public function closeCursor(): bool;
* @return mixed
*
* @since 21.0.0
* @deprecated 28.0.0 use fetchAssociative instead of fetch(), fetchNumeric instead of fetch(\PDO::FETCH_NUM) and fetchOne instead of fetch(\PDO::FETCH_COLUMN)
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
* @deprecated 28.0.0 use fetchAssociative instead of fetch(), fetchNumeric instead of fetch(\PDO::FETCH_NUM) and fetchOne instead of fetch(\PDO::FETCH_COLUMN)
* @deprecated 28.0.0 use fetchAssociative instead of fetch(), fetchNumeric() instead of fetch(\PDO::FETCH_NUM) and fetchOne() instead of fetch(\PDO::FETCH_COLUMN)

🤪

Copy link
Member

Choose a reason for hiding this comment

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

and more brackets for lonely fetchAssociative!

/**
* @param int $fetchMode (one of PDO::FETCH_ASSOC, PDO::FETCH_NUM or PDO::FETCH_COLUMN (2, 3 or 7)
*
* @return mixed[]
*
* @since 21.0.0
* @deprecated 28.0.0 use fetchAllAssociative instead of fetchAll(), fetchAllNumeric instead of fetchAll(FETCH_NUM) and fetchOne instead of fetchAll(FETCH_COLUMN)
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
* @deprecated 28.0.0 use fetchAllAssociative instead of fetchAll(), fetchAllNumeric instead of fetchAll(FETCH_NUM) and fetchOne instead of fetchAll(FETCH_COLUMN)
* @deprecated 28.0.0 use fetchAllAssociative instead of fetchAll(), fetchAllNumeric() instead of fetchAll(FETCH_NUM) and fetchFirstColumn() instead of fetchAll(FETCH_COLUMN)

🤪

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm tired, boss

@solracsf solracsf added this to the Nextcloud 28 milestone Oct 27, 2023
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
@ChristophWurst ChristophWurst added 0. Needs triage Pending check for reproducibility or if it fits our roadmap and removed 3. to review Waiting for reviews labels Nov 2, 2023
@ChristophWurst ChristophWurst marked this pull request as draft November 2, 2023 10:08
@ChristophWurst ChristophWurst removed this from the Nextcloud 28 milestone Nov 2, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone Nov 21, 2023
@blizzz blizzz mentioned this pull request Nov 22, 2023
5 tasks
@ChristophWurst ChristophWurst removed this from the Nextcloud 28 milestone Nov 22, 2023
@blizzz blizzz added this to the Nextcloud 29 milestone Nov 23, 2023
@skjnldsv skjnldsv added 2. developing Work in progress and removed 0. Needs triage Pending check for reproducibility or if it fits our roadmap labels Feb 21, 2024
This was referenced Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants