Skip to content

Commit

Permalink
Refactored MySQLiStatement::$columnNames
Browse files Browse the repository at this point in the history
The `$columnNames` property has a too complex type `string[]|false|null` which leads to:

1. Non-trivial to understand conditions and assertions like `if ($columnNames === false)`, `if ($columnNames === true)` and `assert(is_array($columnNames))`.
2. Avoidable [false-positives](https://scrutinizer-ci.com/g/doctrine/dbal/inspections/20d1aea4-2217-4f38-8adc-5ce10fa9cf0b/issues/files/lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php?status=new&orderField=path&order=asc&honorSelectedPaths=0) on Scrutinizer that thinks that `$columnNames` can be TRUE.
  • Loading branch information
morozov committed Jan 12, 2020
1 parent cc38688 commit 6040522
Showing 1 changed file with 36 additions and 14 deletions.
50 changes: 36 additions & 14 deletions lib/Doctrine/DBAL/Driver/Mysqli/MysqliStatement.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,11 @@
use IteratorAggregate;
use mysqli;
use mysqli_stmt;
use stdClass;
use function array_combine;
use function array_fill;
use function array_key_exists;
use function array_map;
use function assert;
use function count;
use function feof;
Expand Down Expand Up @@ -50,7 +52,27 @@ final class MysqliStatement implements IteratorAggregate, Statement
/** @var mysqli_stmt */
private $stmt;

/** @var string[]|false|null */
/**
* Whether the statement result metadata has been fetched.
*
* @var bool
*/
private $metadataFetched = false;

/**
* Whether the statement result has columns. The property should be used only after the result metadata
* has been fetched ({@see $metadataFetched}). Otherwise, the property value is undetermined.
*
* @var bool
*/
private $hasColumns = false;

/**
* Mapping of statement result column indexes to their names. The property should be used only
* if the statement result has columns ({@see $hasColumns}). Otherwise, the property value is undetermined.
*
* @var array<int,string>
*/
private $columnNames;

/** @var mixed[] */
Expand Down Expand Up @@ -151,26 +173,27 @@ public function execute(?array $params = null) : void
throw StatementError::new($this->stmt);
}

if ($this->columnNames === null) {
if (! $this->metadataFetched) {
$meta = $this->stmt->result_metadata();
if ($meta !== false) {
$this->hasColumns = true;

$fields = $meta->fetch_fields();
assert(is_array($fields));

$columnNames = [];
foreach ($fields as $col) {
$columnNames[] = $col->name;
}
$this->columnNames = array_map(static function (stdClass $field) : string {
return $field->name;
}, $fields);

$meta->free();

$this->columnNames = $columnNames;
} else {
$this->columnNames = false;
$this->hasColumns = false;
}

$this->metadataFetched = true;
}

if ($this->columnNames !== false) {
if ($this->hasColumns) {
// Store result of every execution which has it. Otherwise it will be impossible
// to execute a new statement in case if the previous one has non-fetched rows
// @link http://dev.mysql.com/doc/refman/5.7/en/commands-out-of-sync.html
Expand Down Expand Up @@ -331,7 +354,6 @@ public function fetch(?int $fetchMode = null, ...$args)
return $values;
}

assert(is_array($this->columnNames));
$assoc = array_combine($this->columnNames, $values);
assert(is_array($assoc));

Expand Down Expand Up @@ -404,11 +426,11 @@ public function closeCursor() : void
*/
public function rowCount() : int
{
if ($this->columnNames === false) {
return $this->stmt->affected_rows;
if ($this->hasColumns) {
return $this->stmt->num_rows;
}

return $this->stmt->num_rows;
return $this->stmt->affected_rows;
}

/**
Expand Down

0 comments on commit 6040522

Please sign in to comment.