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
42 changes: 42 additions & 0 deletions lib/private/DB/ResultAdapter.php
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@
*/
namespace OC\DB;

use Doctrine\DBAL\Exception;
use Doctrine\DBAL\Result;
use OC\DB\Exceptions\DbalException;
use OCP\DB\IResult;
use PDO;

Expand All @@ -50,6 +52,22 @@ public function fetch(int $fetchMode = PDO::FETCH_ASSOC) {
return $this->inner->fetch($fetchMode);
}

public function fetchAssociative(): array|false {
try {
return $this->inner->fetchAssociative();
} catch (Exception $e) {
throw DbalException::wrap($e);
}
}

public function fetchAllAssociative(): array {
try {
return $this->inner->fetchAllAssociative();
} catch (Exception $e) {
throw DbalException::wrap($e);
}
}

public function fetchAll(int $fetchMode = PDO::FETCH_ASSOC): array {
if ($fetchMode !== PDO::FETCH_ASSOC && $fetchMode !== PDO::FETCH_NUM && $fetchMode !== PDO::FETCH_COLUMN) {
throw new \Exception('Fetch mode needs to be assoc, num or column.');
Expand All @@ -61,10 +79,34 @@ public function fetchColumn($columnIndex = 0) {
return $this->inner->fetchOne();
}

public function fetchNumeric(): array|false {
try {
return $this->inner->fetchNumeric();
} catch (Exception $e) {
throw DbalException::wrap($e);
}
}

public function fetchAllNumeric(): array {
try {
return $this->inner->fetchAllNumeric();
} catch (Exception $e) {
throw DbalException::wrap($e);
}
}

public function fetchOne() {
return $this->inner->fetchOne();
}

public function fetchFirstColumn(): array {
Fixed Show fixed Hide fixed
try {
return $this->inner->fetchFirstColumn();
Fixed Show fixed Hide fixed
} catch (Exception $e) {
throw DbalException::wrap($e);
}
}

public function rowCount(): int {
return $this->inner->rowCount();
}
Expand Down
52 changes: 52 additions & 0 deletions lib/public/DB/IResult.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.

*/
public function fetch(int $fetchMode = PDO::FETCH_ASSOC);

/**
* Returns the next row of the result as an associative array or FALSE if there are no more rows.
*
* @return array<string,mixed>|false
* @throws Exception
*
* @since 28.0.0
*/
public function fetchAssociative(): array|false;

/**
* Returns an array containing all of the result rows represented as associative arrays
*
* @return list<array<string,mixed>>
* @throws Exception
*
* @since 28.0.0
*/
public function fetchAllAssociative(): array;

/**
* @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, fetchAllNumeric or fetchOne
*/
public function fetchAll(int $fetchMode = PDO::FETCH_ASSOC): array;

Expand All @@ -77,6 +99,26 @@ public function fetchAll(int $fetchMode = PDO::FETCH_ASSOC): array;
*/
public function fetchColumn();

/**
* Returns the next row of the result as a numeric array or FALSE if there are no more rows
*
* @return list<mixed>|false
* @throws Exception
*
* @since 28.0.0
*/
public function fetchNumeric(): array|false;

/**
* Returns an array containing all of the result rows represented as numeric arrays
*
* @return list<list<mixed>>
* @throws Exception
*
* @since 28.0.0
*/
public function fetchAllNumeric(): array;

/**
* Returns the first value of the next row of the result or FALSE if there are no more rows.
*
Expand All @@ -86,6 +128,16 @@ public function fetchColumn();
*/
public function fetchOne();

/**
* Returns an array containing all of the result rows represented as numeric arrays
ChristophWurst marked this conversation as resolved.
Show resolved Hide resolved
*
* @return list<mixed>
* @throws Exception
*
* @since 28.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?


/**
* @return int
*
Expand Down
Loading