From 588e6783d29759b66d22651285b9e2c49640b345 Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Fri, 27 Dec 2013 11:23:31 +0000 Subject: [PATCH] Issue #2146733 by andypost, chx, InternetDevels, bohart: Select queries should not use rowCount() to calculate number of rows. --- core/lib/Drupal/Core/Database/Connection.php | 1 + .../Core/Database/Driver/pgsql/Connection.php | 1 + .../Core/Database/RowCountException.php | 13 ++++++++ core/lib/Drupal/Core/Database/Statement.php | 21 +++++++++++++ .../Drupal/Core/Database/StatementEmpty.php | 12 +++++++- .../Core/Database/StatementInterface.php | 5 +++- .../Core/Database/StatementPrefetch.php | 11 ++++++- .../Tests/LocaleImportFunctionalTest.php | 2 +- core/modules/locale/locale.translation.inc | 6 ++-- .../Drupal/migrate/Tests/FakeStatement.php | 3 +- .../system/Tests/Database/FetchTest.php | 19 +++++++++++- .../Tests/Database/SelectComplexTest.php | 30 +++++++++++++++---- 12 files changed, 110 insertions(+), 14 deletions(-) create mode 100644 core/lib/Drupal/Core/Database/RowCountException.php diff --git a/core/lib/Drupal/Core/Database/Connection.php b/core/lib/Drupal/Core/Database/Connection.php index 3b194f65a4f..6e873038827 100644 --- a/core/lib/Drupal/Core/Database/Connection.php +++ b/core/lib/Drupal/Core/Database/Connection.php @@ -544,6 +544,7 @@ abstract class Connection implements \Serializable { case Database::RETURN_STATEMENT: return $stmt; case Database::RETURN_AFFECTED: + $stmt->allowRowCount = TRUE; return $stmt->rowCount(); case Database::RETURN_INSERT_ID: return $this->connection->lastInsertId(); diff --git a/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php index 68e9c985aa7..9b9cf97d09f 100644 --- a/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Connection.php @@ -130,6 +130,7 @@ class Connection extends DatabaseConnection { switch ($options['return']) { case Database::RETURN_STATEMENT: + $stmt->allowRowCount = FALSE; return $stmt; case Database::RETURN_AFFECTED: return $stmt->rowCount(); diff --git a/core/lib/Drupal/Core/Database/RowCountException.php b/core/lib/Drupal/Core/Database/RowCountException.php new file mode 100644 index 00000000000..6f4a34a61b8 --- /dev/null +++ b/core/lib/Drupal/Core/Database/RowCountException.php @@ -0,0 +1,13 @@ +dbh = $dbh; $this->setFetchMode(\PDO::FETCH_OBJ); @@ -106,4 +113,18 @@ class Statement extends \PDOStatement implements StatementInterface { // Call \PDOStatement::fetch to fetch the row. return $this->fetch(\PDO::FETCH_ASSOC); } + + /** + * {@inheritdoc} + */ + public function rowCount() { + // SELECT query should not use the method. + if ($this->allowRowCount) { + return parent::rowCount(); + } + else { + throw new RowCountException(); + } + } + } diff --git a/core/lib/Drupal/Core/Database/StatementEmpty.php b/core/lib/Drupal/Core/Database/StatementEmpty.php index 7a09add85d2..e6cd08dc5fe 100644 --- a/core/lib/Drupal/Core/Database/StatementEmpty.php +++ b/core/lib/Drupal/Core/Database/StatementEmpty.php @@ -21,6 +21,13 @@ namespace Drupal\Core\Database; */ class StatementEmpty implements \Iterator, StatementInterface { + /** + * Is rowCount() execution allowed. + * + * @var bool + */ + public $allowRowCount = FALSE; + public function execute($args = array(), $options = array()) { return FALSE; } @@ -30,7 +37,10 @@ class StatementEmpty implements \Iterator, StatementInterface { } public function rowCount() { - return 0; + if ($this->allowRowCount) { + return 0; + } + throw new RowCountException(); } public function setFetchMode($mode, $a1 = NULL, $a2 = array()) { diff --git a/core/lib/Drupal/Core/Database/StatementInterface.php b/core/lib/Drupal/Core/Database/StatementInterface.php index 18c29233eab..e93e9163c1d 100644 --- a/core/lib/Drupal/Core/Database/StatementInterface.php +++ b/core/lib/Drupal/Core/Database/StatementInterface.php @@ -74,7 +74,10 @@ interface StatementInterface extends \Traversable { * * @return * The number of rows affected by the last DELETE, INSERT, or UPDATE - * statement executed. + * statement executed or throws \Drupal\Core\Database\RowCountException + * if the last executed statement was SELECT. + * + * @throws \Drupal\Core\Database\RowCountException */ public function rowCount(); diff --git a/core/lib/Drupal/Core/Database/StatementPrefetch.php b/core/lib/Drupal/Core/Database/StatementPrefetch.php index bedf12eb123..c32d94ab8ee 100644 --- a/core/lib/Drupal/Core/Database/StatementPrefetch.php +++ b/core/lib/Drupal/Core/Database/StatementPrefetch.php @@ -128,6 +128,13 @@ class StatementPrefetch implements \Iterator, StatementInterface { 'column' => 0, ); + /** + * Is rowCount() execution allowed. + * + * @var bool + */ + public $allowRowCount = FALSE; + public function __construct(\PDO $dbh, Connection $connection, $query, array $driver_options = array()) { $this->dbh = $dbh; $this->connection = $connection; @@ -174,9 +181,11 @@ class StatementPrefetch implements \Iterator, StatementInterface { $this->throwPDOException(); } + if ($options['return'] == Database::RETURN_AFFECTED) { + $this->rowCount = $statement->rowCount(); + } // Fetch all the data from the reply, in order to release any lock // as soon as possible. - $this->rowCount = $statement->rowCount(); $this->data = $statement->fetchAll(\PDO::FETCH_ASSOC); // Destroy the statement as soon as possible. See // DatabaseConnection_sqlite::PDOPrepare() for explanation. diff --git a/core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.php b/core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.php index d523bd065e1..5d746f30a92 100644 --- a/core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.php +++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleImportFunctionalTest.php @@ -150,7 +150,7 @@ class LocaleImportFunctionalTest extends WebTestBase { // The database should now contain 6 customized strings (two imported // strings are not translated). - $count = db_query('SELECT lid FROM {locales_target} WHERE customized = :custom', array(':custom' => 1))->rowCount(); + $count = db_query('SELECT COUNT(*) FROM {locales_target} WHERE customized = :custom', array(':custom' => 1))->fetchField(); $this->assertEqual($count, 6, 'Customized translations successfully imported.'); // Try importing a .po file with overriding strings, and ensure existing diff --git a/core/modules/locale/locale.translation.inc b/core/modules/locale/locale.translation.inc index dc62a1936bf..5ebe4279264 100644 --- a/core/modules/locale/locale.translation.inc +++ b/core/modules/locale/locale.translation.inc @@ -54,17 +54,17 @@ function locale_translation_get_projects($project_names = array()) { if (empty($projects)) { // Get project data from the database. - $result = db_query('SELECT name, project_type, core, version, server_pattern, status FROM {locale_project}'); + $row_count = db_query_range('SELECT 1 FROM {locale_project}', 0, 1)->fetchField(); // http://drupal.org/node/1777106 is a follow-up issue to make the check for // possible out-of-date project information more robust. - if ($result->rowCount() == 0 && \Drupal::moduleHandler()->moduleExists('update')) { + if ($row_count == 0 && \Drupal::moduleHandler()->moduleExists('update')) { module_load_include('compare.inc', 'locale'); // At least the core project should be in the database, so we build the // data if none are found. locale_translation_build_projects(); - $result = db_query('SELECT name, project_type, core, version, server_pattern, status FROM {locale_project}'); } + $result = db_query('SELECT name, project_type, core, version, server_pattern, status FROM {locale_project}'); foreach ($result as $project) { $projects[$project->name] = $project; diff --git a/core/modules/migrate/tests/Drupal/migrate/Tests/FakeStatement.php b/core/modules/migrate/tests/Drupal/migrate/Tests/FakeStatement.php index 9088c50bd2f..fdcf11310f5 100644 --- a/core/modules/migrate/tests/Drupal/migrate/Tests/FakeStatement.php +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/FakeStatement.php @@ -7,6 +7,7 @@ namespace Drupal\migrate\Tests; +use Drupal\Core\Database\RowCountException; use Drupal\Core\Database\StatementInterface; /** @@ -32,7 +33,7 @@ class FakeStatement extends \ArrayIterator implements StatementInterface { * {@inheritdoc} */ public function rowCount() { - return $this->count(); + throw new RowCountException(); } /** diff --git a/core/modules/system/lib/Drupal/system/Tests/Database/FetchTest.php b/core/modules/system/lib/Drupal/system/Tests/Database/FetchTest.php index fe087b599bd..e96cfbea65c 100644 --- a/core/modules/system/lib/Drupal/system/Tests/Database/FetchTest.php +++ b/core/modules/system/lib/Drupal/system/Tests/Database/FetchTest.php @@ -2,11 +2,12 @@ /** * @file - * Definition of Drupal\system\Tests\Database\FetchTest. + * Contains \Drupal\system\Tests\Database\FetchTest. */ namespace Drupal\system\Tests\Database; +use Drupal\Core\Database\RowCountException; use Drupal\Core\Database\StatementInterface; /** @@ -136,4 +137,20 @@ class FetchTest extends DatabaseTestBase { $this->assertIdentical($record->name, $column[$i++], 'Column matches direct accesss.'); } } + + /** + * Tests that rowCount() throws exception on SELECT query. + */ + public function testRowCount() { + $result = db_query('SELECT name FROM {test}'); + try { + $result->rowCount(); + $exception = FALSE; + } + catch (RowCountException $e) { + $exception = TRUE; + } + $this->assertTrue($exception, 'Exception was thrown'); + } + } diff --git a/core/modules/system/lib/Drupal/system/Tests/Database/SelectComplexTest.php b/core/modules/system/lib/Drupal/system/Tests/Database/SelectComplexTest.php index d76469638a9..7e7d24d1ae2 100644 --- a/core/modules/system/lib/Drupal/system/Tests/Database/SelectComplexTest.php +++ b/core/modules/system/lib/Drupal/system/Tests/Database/SelectComplexTest.php @@ -2,11 +2,13 @@ /** * @file - * Definition of Drupal\system\Tests\Database\SelectComplexTest. + * Contains \Drupal\system\Tests\Database\SelectComplexTest. */ namespace Drupal\system\Tests\Database; +use \Drupal\Core\Database\RowCountException; + /** * Tests more complex select statements. */ @@ -156,9 +158,9 @@ class SelectComplexTest extends DatabaseTestBase { $query->addField('test', 'name'); $query->addField('test', 'age', 'age'); $query->range(0, 2); - $result = $query->execute(); + $query_result = $query->countQuery()->execute()->fetchField(); - $this->assertEqual($result->rowCount(), 2, 'Returned the correct number of rows.'); + $this->assertEqual($query_result, 2, 'Returned the correct number of rows.'); } /** @@ -168,9 +170,9 @@ class SelectComplexTest extends DatabaseTestBase { $query = db_select('test_task'); $query->addField('test_task', 'task'); $query->distinct(); - $result = $query->execute(); + $query_result = $query->countQuery()->execute()->fetchField(); - $this->assertEqual($result->rowCount(), 6, 'Returned the correct number of rows.'); + $this->assertEqual($query_result, 6, 'Returned the correct number of rows.'); } /** @@ -363,4 +365,22 @@ class SelectComplexTest extends DatabaseTestBase { $pos2 = strpos($str, 'db_condition_placeholder_0', $pos + 1); $this->assertFalse($pos2, 'Condition placeholder is not repeated.'); } + + /** + * Tests that rowCount() throws exception on SELECT query. + */ + function testSelectWithRowCount() { + $query = db_select('test'); + $query->addField('test', 'name'); + $result = $query->execute(); + try { + $result->rowCount(); + $exception = FALSE; + } + catch (RowCountException $e) { + $exception = TRUE; + } + $this->assertTrue($exception, 'Exception was thrown'); + } + }