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

[DBAL-2644] Fix fetchAll(PDO::FETCH_COLUMN) on DB2 with Portability wrapper #2647

Merged
merged 1 commit into from
Feb 16, 2017
Merged

[DBAL-2644] Fix fetchAll(PDO::FETCH_COLUMN) on DB2 with Portability wrapper #2647

merged 1 commit into from
Feb 16, 2017

Conversation

morozov
Copy link
Member

@morozov morozov commented Feb 8, 2017

Fixes #2644:

  1. Do not enforce platform-dependent portability settings
  2. Properly handle the result of fetchAll(PDO::FETCH_COLUMN)

@morozov morozov force-pushed the portability-column branch from d4d404f to 9b923bd Compare February 8, 2017 06:34
Ocramius
Ocramius suggested changes Feb 8, 2017
View reviewed changes
lib/Doctrine/DBAL/Portability/Statement.php Outdated
@@ -175,8 +175,20 @@ public function fetchAll($fetchMode = null, $columnIndex = 0)
return $rows;
}

foreach ($rows as $num => $row) {
$rows[$num] = $this->fixRow($row, $iterateRow, $fixCase);
if ($fetchMode === 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.

@morozov can you isolate the contents of the if and else statements into private methods? Early return preferred

lib/Doctrine/DBAL/Portability/Statement.php Outdated
if ($fetchMode === PDO::FETCH_COLUMN) {
if ($iterateRow) {
foreach ($rows as $num => $value) {
if (($this->portability & Connection::PORTABILITY_EMPTY_TO_NULL) && $value === '') {
Copy link
Member

Choose a reason for hiding this comment

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

The first bitwise expression does not need re-computing within the loop, and should be moved to a variable

lib/Doctrine/DBAL/Portability/Statement.php Outdated
foreach ($rows as $num => $value) {
if (($this->portability & Connection::PORTABILITY_EMPTY_TO_NULL) && $value === '') {
$rows[$num] = null;
} elseif (($this->portability & Connection::PORTABILITY_RTRIM) && is_string($value)) {
Copy link
Member

Choose a reason for hiding this comment

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

The first bitwise expression does not need re-computing within the loop, and should be moved to a variable

@morozov morozov force-pushed the portability-column branch 2 times, most recently from 6246cfc to 6ed6465 Compare February 9, 2017 01:33
Ocramius
Ocramius suggested changes Feb 9, 2017
View reviewed changes
lib/Doctrine/DBAL/Portability/Statement.php Outdated
foreach ($rows as $num => $row) {
$rows[$num] = $this->fixRow($row, $iterateRow, $fixCase);
if ($fetchMode === PDO::FETCH_COLUMN) {
$rows = $this->fixColumn($rows, $emptyToNull, $rTrim);
Copy link
Member

Choose a reason for hiding this comment

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

return early please

lib/Doctrine/DBAL/Portability/Statement.php Outdated
$rows[$num] = $this->fixRow($row, $iterateRow, $fixCase);
if ($fetchMode === PDO::FETCH_COLUMN) {
$rows = $this->fixColumn($rows, $emptyToNull, $rTrim);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

No need for else

lib/Doctrine/DBAL/Portability/Statement.php Outdated
*/
private function fixColumn(array $values, $emptyToNull, $rTrim)
{
if (!$emptyToNull && !$rTrim) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it simpler to loop twice, and call two separate methods that do one the emptyToNull fixing, and the other the rTrim fixing?

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 think ideally (if performance allows), we could have just 2-3 lower level functions which convert keys and values, and combine them together in reusable combinations based on fetch*(PDO::FETCH_*). But let's try your suggestion first.

Copy link
Member

Choose a reason for hiding this comment

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

Another idea that comes to mind is to normalize the rows at the beginning if PDO::FETCH_COLUMN is given and denormalize again at the end? Like expand the rows to a multi-dimensional array at the beginning and flatten again after portability adjustments? Not sure if that might be a better solution.

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 think it'd look hacky but definitely save some lines of code.

Copy link
Member

Choose a reason for hiding this comment

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

The most important thing is it saves a lot of complexity and function calls which only serve for 1 use-case. But as I said, not sure about that, it is just an idea that came to my mind.

@morozov morozov force-pushed the portability-column branch 4 times, most recently from 539788d to caaedea Compare February 10, 2017 16:09
@morozov
Copy link
Member Author

morozov commented Feb 15, 2017

@Ocramius are you okay with the current approach?

Off-topic: I tried putting together an alternative implementation which uses a functional approach to converting data and avoids code duplication, but… for some reason, there's even more code there than in the original one 😕

@Ocramius
Copy link
Member

@morozov I'm fine with it, and merging :-)

Ocramius
Ocramius reviewed Feb 16, 2017
View reviewed changes
lib/Doctrine/DBAL/Portability/Connection.php
@@ -71,11 +71,11 @@ public function connect()
} elseif ($this->getDatabasePlatform()->getName() === "sqlite") {
$params['portability'] = $params['portability'] & self::PORTABILITY_SQLITE;
} elseif ($this->getDatabasePlatform()->getName() === "drizzle") {
Copy link
Member

Choose a reason for hiding this comment

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

Note for the next person that has to maintain this: we're sorry about this massive chain of doom and elseif :-(

@Ocramius Ocramius added this to the 2.5.13 milestone Feb 16, 2017
@Ocramius Ocramius assigned Ocramius and unassigned morozov Feb 16, 2017
@Ocramius Ocramius merged commit c7c3d8b into doctrine:master Feb 16, 2017
Ocramius added a commit that referenced this pull request Feb 16, 2017
@Ocramius
Merge branch 'fix/#2644-#2647-fix-fetchAll-with-pdo-fetch-column-on-d…
5a4ff46 
…b2-with-portability-wrapper' into 2.5

Backport #2644 to 2.5.x
Backport #2647 to 2.5.x
@Ocramius
Copy link
Member

Backported to 2.5 via 5a4ff46

@morozov morozov deleted the portability-column branch February 16, 2017 20:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Ocramius Ocramius Ocramius requested changes

@deeky666 deeky666 deeky666 left review comments

Assignees

@Ocramius Ocramius

Labels
Bug IBM DB2 ibm_db2 PDO Portability Prepared Statements
Projects
None yet
Milestone
2.5.13
Development

Successfully merging this pull request may close these issues.
3 participants
@morozov @Ocramius @deeky666