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 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!

*/
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 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

*/
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 the values of the first column of the result
*
* @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