From 16a60253721330323ae201032f0b852697ce2a00 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Thu, 21 Mar 2013 23:04:17 +0100 Subject: [PATCH 01/25] [ticket/11469] Add SQL insert buffer allowing easier handling of multi inserts. 1. Tries to prevent going over max packet size by flushing to the database after a certain number of rows have been added. 2. Because of 1., it is less likely to reach a connection timeout when inserting a huge number of rows. 3. By flushing the buffer when a certain size is reached, memory usage should be lower compared to building the whole insert row set first. PHPBB3-11469 --- phpBB/includes/db/sql_insert_buffer.php | 111 ++++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 phpBB/includes/db/sql_insert_buffer.php diff --git a/phpBB/includes/db/sql_insert_buffer.php b/phpBB/includes/db/sql_insert_buffer.php new file mode 100644 index 0000000000..8d4b03ef53 --- /dev/null +++ b/phpBB/includes/db/sql_insert_buffer.php @@ -0,0 +1,111 @@ +db = $db; + $this->db_supports_multi_insert = $db->multi_insert; + $this->table_name = $table_name; + $this->max_buffered_rows = $max_buffered_rows; + } + + /** + * Inserts a single row into the buffer if multi insert is supported by the + * database (otherwise an insert query is sent immediately). Then flushes + * the buffer if the number of rows in the buffer is now greater than or + * equal to $max_buffered_rows. + * + * @param array $row + * + * @return null + */ + public function insert(array $row) + { + if (!$this->db_supports_multi_insert) + { + $this->db->sql_multi_insert($this->table_name, array($row)); + } + + $this->buffer[] = $row; + + if (sizeof($this->buffer) >= $this->max_buffered_rows) + { + $this->flush(); + } + } + + /** + * Inserts a row set, i.e. an array of rows, by calling insert(). + * + * Please note that it is in most cases better to use insert() instead of + * first building a huge rowset. Or at least sizeof($rows) should be kept + * small. + * + * @param array $rows + * + * @return null + */ + public function insert_all(array $rows) + { + foreach ($rows as $row) + { + $this->insert($row); + } + } + + /** + * Flushes the buffer content to the DB and clears the buffer. + * + * @return null + */ + public function flush() + { + if (!empty($this->buffer)) + { + $this->db->sql_multi_insert($this->table_name, $this->buffer); + $this->buffer = array(); + } + } +} From fc8bf3f3c767067a03d240403598d62fb22ce889 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Mon, 25 Mar 2013 01:41:09 +0100 Subject: [PATCH 02/25] [ticket/11469] Add comment about using sql_multi_insert when not buffering. PHPBB3-11469 --- phpBB/includes/db/sql_insert_buffer.php | 3 +++ 1 file changed, 3 insertions(+) diff --git a/phpBB/includes/db/sql_insert_buffer.php b/phpBB/includes/db/sql_insert_buffer.php index 8d4b03ef53..fe45206893 100644 --- a/phpBB/includes/db/sql_insert_buffer.php +++ b/phpBB/includes/db/sql_insert_buffer.php @@ -65,6 +65,9 @@ class phpbb_db_sql_insert_buffer { if (!$this->db_supports_multi_insert) { + // The database does not support multi inserts. + // Pass data on to sql_multi_insert right away which will + // immediately send an INSERT INTO query to the database. $this->db->sql_multi_insert($this->table_name, array($row)); } From 8dd26dee8349a14abac00db6ad5039d7517bda57 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 27 Mar 2013 11:45:45 +0100 Subject: [PATCH 03/25] [ticket/11469] Add some basic unit tests for phpbb_db_sql_insert_buffer PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 176 ++++++++++++++++++++++++++ 1 file changed, 176 insertions(+) create mode 100644 tests/dbal/sql_insert_buffer_test.php diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php new file mode 100644 index 0000000000..a67ccbe89f --- /dev/null +++ b/tests/dbal/sql_insert_buffer_test.php @@ -0,0 +1,176 @@ +createXMLDataSet(dirname(__FILE__).'/fixtures/config.xml'); + } + + public function insert_buffer_data() + { + $db = $this->new_dbal(); + + if ($db->multi_insert) + { + // Test with enabled and disabled multi_insert + return array( + array(true), + array(false), + ); + } + else + { + // Only test with disabled multi_insert, the DB doesn't support it + return array( + array(false), + ); + } + } + + /** + * @dataProvider insert_buffer_data + */ + public function test_insert_and_flush($force_multi_insert) + { + $db = $this->new_dbal(); + $db->multi_insert = $force_multi_insert; + + $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); + + $sql = 'SELECT COUNT(*) AS num_configs + FROM phpbb_config'; + $result = $db->sql_query_limit($sql, 1); + $this->assertEquals(2, $db->sql_fetchfield('num_configs')); + $db->sql_freeresult($result); + + // This call can be buffered + $buffer->insert(array( + 'config_name' => 'name1', + 'config_value' => 'value1', + 'is_dynamic' => '0', + )); + + if ($db->multi_insert) + { + $sql = 'SELECT COUNT(*) AS num_configs + FROM phpbb_config'; + $result = $db->sql_query_limit($sql, 1); + $this->assertEquals(2, $db->sql_fetchfield('num_configs')); + $db->sql_freeresult($result); + } + else + { + $sql = 'SELECT COUNT(*) AS num_configs + FROM phpbb_config'; + $result = $db->sql_query_limit($sql, 1); + $this->assertEquals(3, $db->sql_fetchfield('num_configs')); + $db->sql_freeresult($result); + } + + // Manually flush + $buffer->flush(); + + $sql = 'SELECT COUNT(*) AS num_configs + FROM phpbb_config'; + $result = $db->sql_query_limit($sql, 1); + $this->assertEquals(3, $db->sql_fetchfield('num_configs')); + $db->sql_freeresult($result); + } + + /** + * @dataProvider insert_buffer_data + */ + public function test_insert_with_flush($force_multi_insert) + { + $db = $this->new_dbal(); + $db->multi_insert = $force_multi_insert; + + $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); + + $sql = 'SELECT COUNT(*) AS num_configs + FROM phpbb_config'; + $result = $db->sql_query_limit($sql, 1); + $this->assertEquals(2, $db->sql_fetchfield('num_configs')); + $db->sql_freeresult($result); + + $buffer->insert(array( + 'config_name' => 'name1', + 'config_value' => 'value1', + 'is_dynamic' => '0', + )); + + // This call flushes the values + $buffer->insert(array( + 'config_name' => 'name2', + 'config_value' => 'value2', + 'is_dynamic' => '0', + )); + + $sql = 'SELECT COUNT(*) AS num_configs + FROM phpbb_config'; + $result = $db->sql_query_limit($sql, 1); + $this->assertEquals(4, $db->sql_fetchfield('num_configs')); + $db->sql_freeresult($result); + } + + /** + * @dataProvider insert_buffer_data + */ + public function test_insert_all_and_flush($force_multi_insert) + { + $db = $this->new_dbal(); + $db->multi_insert = $force_multi_insert; + + $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); + + $sql = 'SELECT COUNT(*) AS num_configs + FROM phpbb_config'; + $result = $db->sql_query_limit($sql, 1); + $this->assertEquals(2, $db->sql_fetchfield('num_configs')); + $db->sql_freeresult($result); + + $buffer->insert_all(array( + array( + 'config_name' => 'name1', + 'config_value' => 'value1', + 'is_dynamic' => '0', + ), + array( + 'config_name' => 'name2', + 'config_value' => 'value2', + 'is_dynamic' => '0', + ), + array( + 'config_name' => 'name3', + 'config_value' => 'value3', + 'is_dynamic' => '0', + ), + )); + + if ($db->multi_insert) + { + $sql = 'SELECT COUNT(*) AS num_configs + FROM phpbb_config'; + $result = $db->sql_query_limit($sql, 1); + $this->assertEquals(4, $db->sql_fetchfield('num_configs')); + $db->sql_freeresult($result); + + // Manually flush + $buffer->flush(); + } + + $sql = 'SELECT COUNT(*) AS num_configs + FROM phpbb_config'; + $result = $db->sql_query_limit($sql, 1); + $this->assertEquals(5, $db->sql_fetchfield('num_configs')); + $db->sql_freeresult($result); + } +} From bf6f2c5875f2476366c1bd660506e70c0f006c9d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 27 Mar 2013 11:47:40 +0100 Subject: [PATCH 04/25] [ticket/11469] Return after sql_multi_insert when multi_insert is false PHPBB3-11469 --- phpBB/includes/db/sql_insert_buffer.php | 1 + 1 file changed, 1 insertion(+) diff --git a/phpBB/includes/db/sql_insert_buffer.php b/phpBB/includes/db/sql_insert_buffer.php index fe45206893..49cf5b8ef6 100644 --- a/phpBB/includes/db/sql_insert_buffer.php +++ b/phpBB/includes/db/sql_insert_buffer.php @@ -69,6 +69,7 @@ class phpbb_db_sql_insert_buffer // Pass data on to sql_multi_insert right away which will // immediately send an INSERT INTO query to the database. $this->db->sql_multi_insert($this->table_name, array($row)); + return; } $this->buffer[] = $row; From af9f30cd52fd7b53b17b946a0c646fd72c4a6f4f Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 27 Mar 2013 14:09:04 +0100 Subject: [PATCH 05/25] [ticket/11469] Use method to check config count, instead of repeating it PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 63 ++++++++------------------- 1 file changed, 18 insertions(+), 45 deletions(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index a67ccbe89f..e3ee767937 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -14,6 +14,15 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case return $this->createXMLDataSet(dirname(__FILE__).'/fixtures/config.xml'); } + protected function assert_config_count($db, $num_configs) + { + $sql = 'SELECT COUNT(*) AS num_configs + FROM phpbb_config'; + $result = $db->sql_query($sql); + $this->assertEquals($num_configs, $db->sql_fetchfield('num_configs')); + $db->sql_freeresult($result); + } + public function insert_buffer_data() { $db = $this->new_dbal(); @@ -45,11 +54,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); - $sql = 'SELECT COUNT(*) AS num_configs - FROM phpbb_config'; - $result = $db->sql_query_limit($sql, 1); - $this->assertEquals(2, $db->sql_fetchfield('num_configs')); - $db->sql_freeresult($result); + $this->assert_config_count($db, 2); // This call can be buffered $buffer->insert(array( @@ -60,29 +65,17 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case if ($db->multi_insert) { - $sql = 'SELECT COUNT(*) AS num_configs - FROM phpbb_config'; - $result = $db->sql_query_limit($sql, 1); - $this->assertEquals(2, $db->sql_fetchfield('num_configs')); - $db->sql_freeresult($result); + $this->assert_config_count($db, 2); } else { - $sql = 'SELECT COUNT(*) AS num_configs - FROM phpbb_config'; - $result = $db->sql_query_limit($sql, 1); - $this->assertEquals(3, $db->sql_fetchfield('num_configs')); - $db->sql_freeresult($result); + $this->assert_config_count($db, 3); } // Manually flush $buffer->flush(); - $sql = 'SELECT COUNT(*) AS num_configs - FROM phpbb_config'; - $result = $db->sql_query_limit($sql, 1); - $this->assertEquals(3, $db->sql_fetchfield('num_configs')); - $db->sql_freeresult($result); + $this->assert_config_count($db, 3); } /** @@ -95,11 +88,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); - $sql = 'SELECT COUNT(*) AS num_configs - FROM phpbb_config'; - $result = $db->sql_query_limit($sql, 1); - $this->assertEquals(2, $db->sql_fetchfield('num_configs')); - $db->sql_freeresult($result); + $this->assert_config_count($db, 2); $buffer->insert(array( 'config_name' => 'name1', @@ -114,11 +103,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case 'is_dynamic' => '0', )); - $sql = 'SELECT COUNT(*) AS num_configs - FROM phpbb_config'; - $result = $db->sql_query_limit($sql, 1); - $this->assertEquals(4, $db->sql_fetchfield('num_configs')); - $db->sql_freeresult($result); + $this->assert_config_count($db, 4); } /** @@ -131,11 +116,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); - $sql = 'SELECT COUNT(*) AS num_configs - FROM phpbb_config'; - $result = $db->sql_query_limit($sql, 1); - $this->assertEquals(2, $db->sql_fetchfield('num_configs')); - $db->sql_freeresult($result); + $this->assert_config_count($db, 2); $buffer->insert_all(array( array( @@ -157,20 +138,12 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case if ($db->multi_insert) { - $sql = 'SELECT COUNT(*) AS num_configs - FROM phpbb_config'; - $result = $db->sql_query_limit($sql, 1); - $this->assertEquals(4, $db->sql_fetchfield('num_configs')); - $db->sql_freeresult($result); + $this->assert_config_count($db, 4); // Manually flush $buffer->flush(); } - $sql = 'SELECT COUNT(*) AS num_configs - FROM phpbb_config'; - $result = $db->sql_query_limit($sql, 1); - $this->assertEquals(5, $db->sql_fetchfield('num_configs')); - $db->sql_freeresult($result); + $this->assert_config_count($db, 5); } } From 94a15f85a64db283a9c9402c40b2d35cb484fb37 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 18:06:48 +0100 Subject: [PATCH 06/25] [ticket/11469] Add example code to class documentation. PHPBB3-11469 --- phpBB/includes/db/sql_insert_buffer.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/phpBB/includes/db/sql_insert_buffer.php b/phpBB/includes/db/sql_insert_buffer.php index fe45206893..772f368987 100644 --- a/phpBB/includes/db/sql_insert_buffer.php +++ b/phpBB/includes/db/sql_insert_buffer.php @@ -19,6 +19,21 @@ if (!defined('IN_PHPBB')) * Collects rows for insert into a database until the buffer size is reached. * Then flushes the buffer to the database and starts over again. * +* Usage: +* +* $buffer = new phpbb_db_sql_insert_buffer($db, 'test_table', 1234); +* +* while (do_stuff()) +* { +* $buffer->insert(array( +* 'column1' => 'value1', +* 'column2' => 'value2', +* )); +* } +* +* $buffer->flush(); +* +* * @package dbal */ class phpbb_db_sql_insert_buffer From 8c5fcac2325356bacb72517f6fbd95f9bfdaf16d Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 18:22:59 +0100 Subject: [PATCH 07/25] [ticket/11469] Add benefits over collecting huge insert arrays to class doc. PHPBB3-11469 --- phpBB/includes/db/sql_insert_buffer.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/phpBB/includes/db/sql_insert_buffer.php b/phpBB/includes/db/sql_insert_buffer.php index 6b884dd412..4bf0608227 100644 --- a/phpBB/includes/db/sql_insert_buffer.php +++ b/phpBB/includes/db/sql_insert_buffer.php @@ -19,6 +19,18 @@ if (!defined('IN_PHPBB')) * Collects rows for insert into a database until the buffer size is reached. * Then flushes the buffer to the database and starts over again. * +* Benefits over collecting a (possibly huge) insert array and then using +* $db->sql_multi_insert() include: +* +* - Going over max packet size of the database connection is usually prevented +* because the data is submitted in batches. +* +* - Reaching database connection timeout is usually prevented because +* submission of batches talks to the database every now and then. +* +* - Usage of less PHP memory because data no longer needed is discarded on +* buffer flush. +* * Usage: * * $buffer = new phpbb_db_sql_insert_buffer($db, 'test_table', 1234); From dc766f29b4381e3cecfacbfeb848b4e13c3e48f9 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 19:19:26 +0100 Subject: [PATCH 08/25] [ticket/11469] Have all methods of phpbb_db_sql_insert_buffer provide feedback. PHPBB3-11469 --- phpBB/includes/db/sql_insert_buffer.php | 26 +++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/phpBB/includes/db/sql_insert_buffer.php b/phpBB/includes/db/sql_insert_buffer.php index 4bf0608227..dd4a62a948 100644 --- a/phpBB/includes/db/sql_insert_buffer.php +++ b/phpBB/includes/db/sql_insert_buffer.php @@ -86,7 +86,8 @@ class phpbb_db_sql_insert_buffer * * @param array $row * - * @return null + * @return bool True when some data was flushed to the database. + * False otherwise. */ public function insert(array $row) { @@ -96,15 +97,18 @@ class phpbb_db_sql_insert_buffer // Pass data on to sql_multi_insert right away which will // immediately send an INSERT INTO query to the database. $this->db->sql_multi_insert($this->table_name, array($row)); - return; + + return true; } $this->buffer[] = $row; if (sizeof($this->buffer) >= $this->max_buffered_rows) { - $this->flush(); + return $this->flush(); } + + return false; } /** @@ -116,20 +120,26 @@ class phpbb_db_sql_insert_buffer * * @param array $rows * - * @return null + * @return bool True when some data was flushed to the database. + * False otherwise. */ public function insert_all(array $rows) { + $result = false; + foreach ($rows as $row) { - $this->insert($row); + $result |= $this->insert($row); } + + return $result; } /** * Flushes the buffer content to the DB and clears the buffer. * - * @return null + * @return bool True when some data was flushed to the database. + * False otherwise. */ public function flush() { @@ -137,6 +147,10 @@ class phpbb_db_sql_insert_buffer { $this->db->sql_multi_insert($this->table_name, $this->buffer); $this->buffer = array(); + + return true; } + + return false; } } From 53f9e2131c9b87d35207ea585981fc9b084d0a11 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 19:27:27 +0100 Subject: [PATCH 09/25] [ticket/11469] Add note about calling flush() after batch insert is done. PHPBB3-11469 --- phpBB/includes/db/sql_insert_buffer.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/phpBB/includes/db/sql_insert_buffer.php b/phpBB/includes/db/sql_insert_buffer.php index dd4a62a948..3bd96616b3 100644 --- a/phpBB/includes/db/sql_insert_buffer.php +++ b/phpBB/includes/db/sql_insert_buffer.php @@ -31,6 +31,11 @@ if (!defined('IN_PHPBB')) * - Usage of less PHP memory because data no longer needed is discarded on * buffer flush. * +* Attention: +* Please note that users of this class have to call flush() to flush the +* remaining rows to the database after their batch insert operation is +* finished. +* * Usage: * * $buffer = new phpbb_db_sql_insert_buffer($db, 'test_table', 1234); From 9606ccc2021e7e169473d92306e28680e4b9f966 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 27 Mar 2013 20:54:20 +0100 Subject: [PATCH 10/25] [ticket/11469] Split tests and skip multi_insert if unavailable PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 149 +++++++++++++++++--------- 1 file changed, 100 insertions(+), 49 deletions(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index e3ee767937..bdf3544bbc 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -23,34 +23,10 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $db->sql_freeresult($result); } - public function insert_buffer_data() + public function test_multi_insert_disabled_insert_and_flush() { $db = $this->new_dbal(); - - if ($db->multi_insert) - { - // Test with enabled and disabled multi_insert - return array( - array(true), - array(false), - ); - } - else - { - // Only test with disabled multi_insert, the DB doesn't support it - return array( - array(false), - ); - } - } - - /** - * @dataProvider insert_buffer_data - */ - public function test_insert_and_flush($force_multi_insert) - { - $db = $this->new_dbal(); - $db->multi_insert = $force_multi_insert; + $db->multi_insert = false; $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); @@ -63,14 +39,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case 'is_dynamic' => '0', )); - if ($db->multi_insert) - { - $this->assert_config_count($db, 2); - } - else - { - $this->assert_config_count($db, 3); - } + $this->assert_config_count($db, 3); // Manually flush $buffer->flush(); @@ -78,13 +47,38 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 3); } - /** - * @dataProvider insert_buffer_data - */ - public function test_insert_with_flush($force_multi_insert) + public function test_multi_insert_enabled_insert_and_flush() { $db = $this->new_dbal(); - $db->multi_insert = $force_multi_insert; + + if (!$db->multi_insert) + { + $this->markTestSkipped('Database does not support multi_insert'); + } + + $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); + + $this->assert_config_count($db, 2); + + // This call can be buffered + $buffer->insert(array( + 'config_name' => 'name1', + 'config_value' => 'value1', + 'is_dynamic' => '0', + )); + + $this->assert_config_count($db, 2); + + // Manually flush + $buffer->flush(); + + $this->assert_config_count($db, 3); + } + + public function test_multi_insert_disabled_insert_with_flush() + { + $db = $this->new_dbal(); + $db->multi_insert = false; $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); @@ -106,13 +100,39 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 4); } - /** - * @dataProvider insert_buffer_data - */ - public function test_insert_all_and_flush($force_multi_insert) + public function test_multi_insert_enabled_insert_with_flush() { $db = $this->new_dbal(); - $db->multi_insert = $force_multi_insert; + + if (!$db->multi_insert) + { + $this->markTestSkipped('Database does not support multi_insert'); + } + + $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); + + $this->assert_config_count($db, 2); + + $buffer->insert(array( + 'config_name' => 'name1', + 'config_value' => 'value1', + 'is_dynamic' => '0', + )); + + // This call flushes the values + $buffer->insert(array( + 'config_name' => 'name2', + 'config_value' => 'value2', + 'is_dynamic' => '0', + )); + + $this->assert_config_count($db, 4); + } + + public function test_multi_insert_disabled_insert_all_and_flush() + { + $db = $this->new_dbal(); + $db->multi_insert = false; $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); @@ -136,14 +156,45 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case ), )); - if ($db->multi_insert) - { - $this->assert_config_count($db, 4); + $this->assert_config_count($db, 5); + } - // Manually flush - $buffer->flush(); + public function test_multi_insert_enabled_insert_all_and_flush() + { + $db = $this->new_dbal(); + + if (!$db->multi_insert) + { + $this->markTestSkipped('Database does not support multi_insert'); } + $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); + + $this->assert_config_count($db, 2); + + $buffer->insert_all(array( + array( + 'config_name' => 'name1', + 'config_value' => 'value1', + 'is_dynamic' => '0', + ), + array( + 'config_name' => 'name2', + 'config_value' => 'value2', + 'is_dynamic' => '0', + ), + array( + 'config_name' => 'name3', + 'config_value' => 'value3', + 'is_dynamic' => '0', + ), + )); + + $this->assert_config_count($db, 4); + + // Manually flush + $buffer->flush(); + $this->assert_config_count($db, 5); } } From 69ad4aab787db4c80c431c412a76d731a1abdf63 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 27 Mar 2013 20:55:08 +0100 Subject: [PATCH 11/25] [ticket/11469] Check return values of the functions PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 38 +++++++++++++-------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index bdf3544bbc..fd92ca9bba 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -33,16 +33,16 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); // This call can be buffered - $buffer->insert(array( + $this->assertTrue($buffer->insert(array( 'config_name' => 'name1', 'config_value' => 'value1', 'is_dynamic' => '0', - )); + ))); $this->assert_config_count($db, 3); // Manually flush - $buffer->flush(); + $this->assertFalse($buffer->flush()); $this->assert_config_count($db, 3); } @@ -61,16 +61,16 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); // This call can be buffered - $buffer->insert(array( + $this->assertFalse($buffer->insert(array( 'config_name' => 'name1', 'config_value' => 'value1', 'is_dynamic' => '0', - )); + ))); $this->assert_config_count($db, 2); // Manually flush - $buffer->flush(); + $this->assertTrue($buffer->flush()); $this->assert_config_count($db, 3); } @@ -84,18 +84,18 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); - $buffer->insert(array( + $this->assertTrue($buffer->insert(array( 'config_name' => 'name1', 'config_value' => 'value1', 'is_dynamic' => '0', - )); + ))); // This call flushes the values - $buffer->insert(array( + $this->assertTrue($buffer->insert(array( 'config_name' => 'name2', 'config_value' => 'value2', 'is_dynamic' => '0', - )); + ))); $this->assert_config_count($db, 4); } @@ -113,18 +113,18 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); - $buffer->insert(array( + $this->assertFalse($buffer->insert(array( 'config_name' => 'name1', 'config_value' => 'value1', 'is_dynamic' => '0', - )); + ))); // This call flushes the values - $buffer->insert(array( + $this->assertTrue($buffer->insert(array( 'config_name' => 'name2', 'config_value' => 'value2', 'is_dynamic' => '0', - )); + ))); $this->assert_config_count($db, 4); } @@ -138,7 +138,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); - $buffer->insert_all(array( + $this->assertTrue($buffer->insert_all(array( array( 'config_name' => 'name1', 'config_value' => 'value1', @@ -154,7 +154,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case 'config_value' => 'value3', 'is_dynamic' => '0', ), - )); + ))); $this->assert_config_count($db, 5); } @@ -172,7 +172,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); - $buffer->insert_all(array( + $this->assertTrue($buffer->insert_all(array( array( 'config_name' => 'name1', 'config_value' => 'value1', @@ -188,12 +188,12 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case 'config_value' => 'value3', 'is_dynamic' => '0', ), - )); + ))); $this->assert_config_count($db, 4); // Manually flush - $buffer->flush(); + $this->assertTrue($buffer->flush()); $this->assert_config_count($db, 5); } From c9f059c4f2793e9f98c3e0fbaad06708dd557d31 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Wed, 27 Mar 2013 20:55:48 +0100 Subject: [PATCH 12/25] [ticket/11469] Cast $result to boolean in insert_all() |= returns integer values PHPBB3-11469 --- phpBB/includes/db/sql_insert_buffer.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/includes/db/sql_insert_buffer.php b/phpBB/includes/db/sql_insert_buffer.php index 3bd96616b3..dd7932c7bd 100644 --- a/phpBB/includes/db/sql_insert_buffer.php +++ b/phpBB/includes/db/sql_insert_buffer.php @@ -137,7 +137,7 @@ class phpbb_db_sql_insert_buffer $result |= $this->insert($row); } - return $result; + return (bool) $result; } /** From a534497d82cb9febaa18ee6c858ef68217bc2d14 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 23:21:29 +0100 Subject: [PATCH 13/25] [ticket/11469] Move protected method to end of test file. PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index fd92ca9bba..0cf476b14a 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -14,15 +14,6 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case return $this->createXMLDataSet(dirname(__FILE__).'/fixtures/config.xml'); } - protected function assert_config_count($db, $num_configs) - { - $sql = 'SELECT COUNT(*) AS num_configs - FROM phpbb_config'; - $result = $db->sql_query($sql); - $this->assertEquals($num_configs, $db->sql_fetchfield('num_configs')); - $db->sql_freeresult($result); - } - public function test_multi_insert_disabled_insert_and_flush() { $db = $this->new_dbal(); @@ -197,4 +188,13 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 5); } + + protected function assert_config_count($db, $num_configs) + { + $sql = 'SELECT COUNT(*) AS num_configs + FROM phpbb_config'; + $result = $db->sql_query($sql); + $this->assertEquals($num_configs, $db->sql_fetchfield('num_configs')); + $db->sql_freeresult($result); + } } From 873f098b6ccc108dc293054f2cf7cb7231684caa Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 23:30:23 +0100 Subject: [PATCH 14/25] [ticket/11469] Do not repeat array with three rows. PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 57 +++++++++++---------------- 1 file changed, 23 insertions(+), 34 deletions(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index 0cf476b14a..59bfb73d0f 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -129,23 +129,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); - $this->assertTrue($buffer->insert_all(array( - array( - 'config_name' => 'name1', - 'config_value' => 'value1', - 'is_dynamic' => '0', - ), - array( - 'config_name' => 'name2', - 'config_value' => 'value2', - 'is_dynamic' => '0', - ), - array( - 'config_name' => 'name3', - 'config_value' => 'value3', - 'is_dynamic' => '0', - ), - ))); + $this->assertTrue($buffer->insert_all($this->get_three_rows())); $this->assert_config_count($db, 5); } @@ -163,23 +147,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); - $this->assertTrue($buffer->insert_all(array( - array( - 'config_name' => 'name1', - 'config_value' => 'value1', - 'is_dynamic' => '0', - ), - array( - 'config_name' => 'name2', - 'config_value' => 'value2', - 'is_dynamic' => '0', - ), - array( - 'config_name' => 'name3', - 'config_value' => 'value3', - 'is_dynamic' => '0', - ), - ))); + $this->assertTrue($buffer->insert_all($this->get_three_rows())); $this->assert_config_count($db, 4); @@ -197,4 +165,25 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assertEquals($num_configs, $db->sql_fetchfield('num_configs')); $db->sql_freeresult($result); } + + protected function get_three_rows() + { + return array( + array( + 'config_name' => 'name1', + 'config_value' => 'value1', + 'is_dynamic' => '0', + ), + array( + 'config_name' => 'name2', + 'config_value' => 'value2', + 'is_dynamic' => '0', + ), + array( + 'config_name' => 'name3', + 'config_value' => 'value3', + 'is_dynamic' => '0', + ), + ); + } } From b88eb3c8e099e1a0634ab84e9e1c215c00410264 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 23:33:13 +0100 Subject: [PATCH 15/25] [ticket/11469] Do not repeat row generation. PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 63 ++++++++------------------- 1 file changed, 18 insertions(+), 45 deletions(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index 59bfb73d0f..650a42c36d 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -24,11 +24,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); // This call can be buffered - $this->assertTrue($buffer->insert(array( - 'config_name' => 'name1', - 'config_value' => 'value1', - 'is_dynamic' => '0', - ))); + $this->assertTrue($buffer->insert($this->get_row(1))); $this->assert_config_count($db, 3); @@ -52,11 +48,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); // This call can be buffered - $this->assertFalse($buffer->insert(array( - 'config_name' => 'name1', - 'config_value' => 'value1', - 'is_dynamic' => '0', - ))); + $this->assertFalse($buffer->insert($this->get_row(1))); $this->assert_config_count($db, 2); @@ -75,18 +67,10 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); - $this->assertTrue($buffer->insert(array( - 'config_name' => 'name1', - 'config_value' => 'value1', - 'is_dynamic' => '0', - ))); + $this->assertTrue($buffer->insert($this->get_row(1))); // This call flushes the values - $this->assertTrue($buffer->insert(array( - 'config_name' => 'name2', - 'config_value' => 'value2', - 'is_dynamic' => '0', - ))); + $this->assertTrue($buffer->insert($this->get_row(2))); $this->assert_config_count($db, 4); } @@ -104,18 +88,10 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->assert_config_count($db, 2); - $this->assertFalse($buffer->insert(array( - 'config_name' => 'name1', - 'config_value' => 'value1', - 'is_dynamic' => '0', - ))); + $this->assertFalse($buffer->insert($this->get_row(1))); // This call flushes the values - $this->assertTrue($buffer->insert(array( - 'config_name' => 'name2', - 'config_value' => 'value2', - 'is_dynamic' => '0', - ))); + $this->assertTrue($buffer->insert($this->get_row(2))); $this->assert_config_count($db, 4); } @@ -166,24 +142,21 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $db->sql_freeresult($result); } + protected function get_row($rownum) + { + return array( + 'config_name' => "name$rownum", + 'config_value' => "value$rownum", + 'is_dynamic' => '0', + ); + } + protected function get_three_rows() { return array( - array( - 'config_name' => 'name1', - 'config_value' => 'value1', - 'is_dynamic' => '0', - ), - array( - 'config_name' => 'name2', - 'config_value' => 'value2', - 'is_dynamic' => '0', - ), - array( - 'config_name' => 'name3', - 'config_value' => 'value3', - 'is_dynamic' => '0', - ), + $this->get_row(1), + $this->get_row(2), + $this->get_row(3), ); } } From 4132573088c376fcb44cc588d9341c8d38b6d694 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 23:35:36 +0100 Subject: [PATCH 16/25] [ticket/11469] Use buffer with a single element instead of extra code path. PHPBB3-11469 --- phpBB/includes/db/sql_insert_buffer.php | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/phpBB/includes/db/sql_insert_buffer.php b/phpBB/includes/db/sql_insert_buffer.php index dd7932c7bd..03c8a875b9 100644 --- a/phpBB/includes/db/sql_insert_buffer.php +++ b/phpBB/includes/db/sql_insert_buffer.php @@ -96,19 +96,11 @@ class phpbb_db_sql_insert_buffer */ public function insert(array $row) { - if (!$this->db_supports_multi_insert) - { - // The database does not support multi inserts. - // Pass data on to sql_multi_insert right away which will - // immediately send an INSERT INTO query to the database. - $this->db->sql_multi_insert($this->table_name, array($row)); - - return true; - } - $this->buffer[] = $row; - if (sizeof($this->buffer) >= $this->max_buffered_rows) + // Flush buffer if it is full or when DB does not support multi inserts. + // In the later case, the buffer will always only contain one row. + if (!$this->db_supports_multi_insert || sizeof($this->buffer) >= $this->max_buffered_rows) { return $this->flush(); } From 1bd13acb753553ed5b9ab54144d0ca6507b031a3 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 23:37:08 +0100 Subject: [PATCH 17/25] [ticket/11469] Use multi insert property from DB. Do not copy value to buffer. PHPBB3-11469 --- phpBB/includes/db/sql_insert_buffer.php | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/phpBB/includes/db/sql_insert_buffer.php b/phpBB/includes/db/sql_insert_buffer.php index 03c8a875b9..46d397e7b4 100644 --- a/phpBB/includes/db/sql_insert_buffer.php +++ b/phpBB/includes/db/sql_insert_buffer.php @@ -58,9 +58,6 @@ class phpbb_db_sql_insert_buffer /** @var phpbb_db_driver */ protected $db; - /** @var bool */ - protected $db_supports_multi_insert; - /** @var string */ protected $table_name; @@ -78,7 +75,6 @@ class phpbb_db_sql_insert_buffer public function __construct(phpbb_db_driver $db, $table_name, $max_buffered_rows = 500) { $this->db = $db; - $this->db_supports_multi_insert = $db->multi_insert; $this->table_name = $table_name; $this->max_buffered_rows = $max_buffered_rows; } @@ -100,7 +96,7 @@ class phpbb_db_sql_insert_buffer // Flush buffer if it is full or when DB does not support multi inserts. // In the later case, the buffer will always only contain one row. - if (!$this->db_supports_multi_insert || sizeof($this->buffer) >= $this->max_buffered_rows) + if (!$this->db->multi_insert || sizeof($this->buffer) >= $this->max_buffered_rows) { return $this->flush(); } From b48c4d9549acaff8bbe7846b2390267ccd36d39a Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 23:45:10 +0100 Subject: [PATCH 18/25] [ticket/11469] Use setUp() to setup DB and a buffer with size 2. PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 104 ++++++++++++-------------- 1 file changed, 47 insertions(+), 57 deletions(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index 650a42c36d..bc6508b30a 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -9,6 +9,17 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case { + protected $db; + protected $buffer; + + public function setUp() + { + parent::setUp(); + + $this->db = $this->new_dbal(); + $this->buffer = new phpbb_db_sql_insert_buffer($this->db, 'phpbb_config', 2); + } + public function getDataSet() { return $this->createXMLDataSet(dirname(__FILE__).'/fixtures/config.xml'); @@ -16,130 +27,109 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case public function test_multi_insert_disabled_insert_and_flush() { - $db = $this->new_dbal(); - $db->multi_insert = false; + $this->db->multi_insert = false; - $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); - - $this->assert_config_count($db, 2); + $this->assert_config_count(2); // This call can be buffered - $this->assertTrue($buffer->insert($this->get_row(1))); + $this->assertTrue($this->buffer->insert($this->get_row(1))); - $this->assert_config_count($db, 3); + $this->assert_config_count(3); // Manually flush - $this->assertFalse($buffer->flush()); + $this->assertFalse($this->buffer->flush()); - $this->assert_config_count($db, 3); + $this->assert_config_count(3); } public function test_multi_insert_enabled_insert_and_flush() { - $db = $this->new_dbal(); - - if (!$db->multi_insert) + if (!$this->db->multi_insert) { $this->markTestSkipped('Database does not support multi_insert'); } - $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); - - $this->assert_config_count($db, 2); + $this->assert_config_count(2); // This call can be buffered - $this->assertFalse($buffer->insert($this->get_row(1))); + $this->assertFalse($this->buffer->insert($this->get_row(1))); - $this->assert_config_count($db, 2); + $this->assert_config_count(2); // Manually flush - $this->assertTrue($buffer->flush()); + $this->assertTrue($this->buffer->flush()); - $this->assert_config_count($db, 3); + $this->assert_config_count(3); } public function test_multi_insert_disabled_insert_with_flush() { - $db = $this->new_dbal(); - $db->multi_insert = false; + $this->db->multi_insert = false; - $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); + $this->assert_config_count(2); - $this->assert_config_count($db, 2); - - $this->assertTrue($buffer->insert($this->get_row(1))); + $this->assertTrue($this->buffer->insert($this->get_row(1))); // This call flushes the values - $this->assertTrue($buffer->insert($this->get_row(2))); + $this->assertTrue($this->buffer->insert($this->get_row(2))); - $this->assert_config_count($db, 4); + $this->assert_config_count(4); } public function test_multi_insert_enabled_insert_with_flush() { - $db = $this->new_dbal(); - - if (!$db->multi_insert) + if (!$this->db->multi_insert) { $this->markTestSkipped('Database does not support multi_insert'); } - $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); + $this->assert_config_count(2); - $this->assert_config_count($db, 2); - - $this->assertFalse($buffer->insert($this->get_row(1))); + $this->assertFalse($this->buffer->insert($this->get_row(1))); // This call flushes the values - $this->assertTrue($buffer->insert($this->get_row(2))); + $this->assertTrue($this->buffer->insert($this->get_row(2))); - $this->assert_config_count($db, 4); + $this->assert_config_count(4); } public function test_multi_insert_disabled_insert_all_and_flush() { - $db = $this->new_dbal(); - $db->multi_insert = false; + $this->db->multi_insert = false; - $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); + $this->assert_config_count(2); - $this->assert_config_count($db, 2); + $this->assertTrue($this->buffer->insert_all($this->get_three_rows())); - $this->assertTrue($buffer->insert_all($this->get_three_rows())); - - $this->assert_config_count($db, 5); + $this->assert_config_count(5); } public function test_multi_insert_enabled_insert_all_and_flush() { - $db = $this->new_dbal(); - - if (!$db->multi_insert) + if (!$this->db->multi_insert) { $this->markTestSkipped('Database does not support multi_insert'); } - $buffer = new phpbb_db_sql_insert_buffer($db, 'phpbb_config', 2); + $this->assert_config_count(2); - $this->assert_config_count($db, 2); + $this->assertTrue($this->buffer->insert_all($this->get_three_rows())); - $this->assertTrue($buffer->insert_all($this->get_three_rows())); - - $this->assert_config_count($db, 4); + $this->assert_config_count(4); // Manually flush - $this->assertTrue($buffer->flush()); + $this->assertTrue($this->buffer->flush()); - $this->assert_config_count($db, 5); + $this->assert_config_count(5); } - protected function assert_config_count($db, $num_configs) + protected function assert_config_count($num_configs) { $sql = 'SELECT COUNT(*) AS num_configs FROM phpbb_config'; - $result = $db->sql_query($sql); - $this->assertEquals($num_configs, $db->sql_fetchfield('num_configs')); - $db->sql_freeresult($result); + $result = $this->db->sql_query($sql); + $this->assertEquals($num_configs, $this->db->sql_fetchfield('num_configs')); + $this->db->sql_freeresult($result); } protected function get_row($rownum) From c909d9602b8a8d91d9c6cd72e01c1ddf21dcf593 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 23:46:39 +0100 Subject: [PATCH 19/25] [ticket/11469] Do not repeat assert_config_count(2). Also move to setUp(). PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index bc6508b30a..4aad852149 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -18,6 +18,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->db = $this->new_dbal(); $this->buffer = new phpbb_db_sql_insert_buffer($this->db, 'phpbb_config', 2); + $this->assert_config_count(2); } public function getDataSet() @@ -29,8 +30,6 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case { $this->db->multi_insert = false; - $this->assert_config_count(2); - // This call can be buffered $this->assertTrue($this->buffer->insert($this->get_row(1))); @@ -49,8 +48,6 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->markTestSkipped('Database does not support multi_insert'); } - $this->assert_config_count(2); - // This call can be buffered $this->assertFalse($this->buffer->insert($this->get_row(1))); @@ -66,8 +63,6 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case { $this->db->multi_insert = false; - $this->assert_config_count(2); - $this->assertTrue($this->buffer->insert($this->get_row(1))); // This call flushes the values @@ -83,8 +78,6 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->markTestSkipped('Database does not support multi_insert'); } - $this->assert_config_count(2); - $this->assertFalse($this->buffer->insert($this->get_row(1))); // This call flushes the values @@ -97,8 +90,6 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case { $this->db->multi_insert = false; - $this->assert_config_count(2); - $this->assertTrue($this->buffer->insert_all($this->get_three_rows())); $this->assert_config_count(5); @@ -111,8 +102,6 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->markTestSkipped('Database does not support multi_insert'); } - $this->assert_config_count(2); - $this->assertTrue($this->buffer->insert_all($this->get_three_rows())); $this->assert_config_count(4); From eacd0f3e7d200924a1d98d1ce34a454eda707dd4 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 23:48:48 +0100 Subject: [PATCH 20/25] [ticket/11469] Fix spacing in getDataSet(). PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index 4aad852149..bd03fb9c05 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -23,7 +23,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case public function getDataSet() { - return $this->createXMLDataSet(dirname(__FILE__).'/fixtures/config.xml'); + return $this->createXMLDataSet(dirname(__FILE__) . '/fixtures/config.xml'); } public function test_multi_insert_disabled_insert_and_flush() From 6f946e2188d08bdc939e56143bff9e663c0b6931 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 23:53:03 +0100 Subject: [PATCH 21/25] [ticket/11469] Refactor get_three_rows() into get_rows($n). PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index bd03fb9c05..f06df26b0c 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -90,7 +90,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case { $this->db->multi_insert = false; - $this->assertTrue($this->buffer->insert_all($this->get_three_rows())); + $this->assertTrue($this->buffer->insert_all($this->get_rows(3))); $this->assert_config_count(5); } @@ -102,7 +102,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->markTestSkipped('Database does not support multi_insert'); } - $this->assertTrue($this->buffer->insert_all($this->get_three_rows())); + $this->assertTrue($this->buffer->insert_all($this->get_rows(3))); $this->assert_config_count(4); @@ -130,12 +130,13 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case ); } - protected function get_three_rows() + protected function get_rows($n) { - return array( - $this->get_row(1), - $this->get_row(2), - $this->get_row(3), - ); + $result = array(); + for ($i = 0; $i < $n; ++$i) + { + $result[] = $this->get_row($i); + } + return $result; } } From a04fe625a86c0b1fcc037687afec39c659fbd830 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Wed, 27 Mar 2013 23:55:26 +0100 Subject: [PATCH 22/25] [ticket/11469] Do not repeat markTestSkipped() message. PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index f06df26b0c..9357278a62 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -43,10 +43,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case public function test_multi_insert_enabled_insert_and_flush() { - if (!$this->db->multi_insert) - { - $this->markTestSkipped('Database does not support multi_insert'); - } + $this->check_multi_insert_support(); // This call can be buffered $this->assertFalse($this->buffer->insert($this->get_row(1))); @@ -73,10 +70,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case public function test_multi_insert_enabled_insert_with_flush() { - if (!$this->db->multi_insert) - { - $this->markTestSkipped('Database does not support multi_insert'); - } + $this->check_multi_insert_support(); $this->assertFalse($this->buffer->insert($this->get_row(1))); @@ -97,10 +91,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case public function test_multi_insert_enabled_insert_all_and_flush() { - if (!$this->db->multi_insert) - { - $this->markTestSkipped('Database does not support multi_insert'); - } + $this->check_multi_insert_support(); $this->assertTrue($this->buffer->insert_all($this->get_rows(3))); @@ -121,6 +112,14 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case $this->db->sql_freeresult($result); } + protected function check_multi_insert_support() + { + if (!$this->db->multi_insert) + { + $this->markTestSkipped('Database does not support multi_insert'); + } + } + protected function get_row($rownum) { return array( From e022a7e8f602de67472e4ea5b6ecb6280f4054e4 Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Thu, 28 Mar 2013 00:03:48 +0100 Subject: [PATCH 23/25] [ticket/11469] Remove comments. Method names should be good enough now. PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index 9357278a62..04ef6a3d52 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -29,77 +29,50 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case public function test_multi_insert_disabled_insert_and_flush() { $this->db->multi_insert = false; - - // This call can be buffered $this->assertTrue($this->buffer->insert($this->get_row(1))); - $this->assert_config_count(3); - - // Manually flush $this->assertFalse($this->buffer->flush()); - $this->assert_config_count(3); } public function test_multi_insert_enabled_insert_and_flush() { $this->check_multi_insert_support(); - - // This call can be buffered $this->assertFalse($this->buffer->insert($this->get_row(1))); - $this->assert_config_count(2); - - // Manually flush $this->assertTrue($this->buffer->flush()); - $this->assert_config_count(3); } public function test_multi_insert_disabled_insert_with_flush() { $this->db->multi_insert = false; - $this->assertTrue($this->buffer->insert($this->get_row(1))); - - // This call flushes the values $this->assertTrue($this->buffer->insert($this->get_row(2))); - $this->assert_config_count(4); } public function test_multi_insert_enabled_insert_with_flush() { $this->check_multi_insert_support(); - $this->assertFalse($this->buffer->insert($this->get_row(1))); - - // This call flushes the values $this->assertTrue($this->buffer->insert($this->get_row(2))); - $this->assert_config_count(4); } public function test_multi_insert_disabled_insert_all_and_flush() { $this->db->multi_insert = false; - $this->assertTrue($this->buffer->insert_all($this->get_rows(3))); - $this->assert_config_count(5); } public function test_multi_insert_enabled_insert_all_and_flush() { $this->check_multi_insert_support(); - $this->assertTrue($this->buffer->insert_all($this->get_rows(3))); - $this->assert_config_count(4); - - // Manually flush $this->assertTrue($this->buffer->flush()); - $this->assert_config_count(5); } From e3a6935de6185bde189e53452fca024ef9474c1b Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Thu, 28 Mar 2013 00:20:24 +0100 Subject: [PATCH 24/25] [ticket/11469] Add more table status assertions. PHPBB3-11469 --- tests/dbal/sql_insert_buffer_test.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/dbal/sql_insert_buffer_test.php b/tests/dbal/sql_insert_buffer_test.php index 04ef6a3d52..45339a6b50 100644 --- a/tests/dbal/sql_insert_buffer_test.php +++ b/tests/dbal/sql_insert_buffer_test.php @@ -48,6 +48,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case { $this->db->multi_insert = false; $this->assertTrue($this->buffer->insert($this->get_row(1))); + $this->assert_config_count(3); $this->assertTrue($this->buffer->insert($this->get_row(2))); $this->assert_config_count(4); } @@ -56,6 +57,7 @@ class phpbb_dbal_sql_insert_buffer_test extends phpbb_database_test_case { $this->check_multi_insert_support(); $this->assertFalse($this->buffer->insert($this->get_row(1))); + $this->assert_config_count(2); $this->assertTrue($this->buffer->insert($this->get_row(2))); $this->assert_config_count(4); } From 4bd5f279dc98f036021c04172ecbb30c092de59f Mon Sep 17 00:00:00 2001 From: Andreas Fischer Date: Thu, 28 Mar 2013 00:27:02 +0100 Subject: [PATCH 25/25] [ticket/11469] Add comment about using bitwise operator. PHPBB3-11469 --- phpBB/includes/db/sql_insert_buffer.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/db/sql_insert_buffer.php b/phpBB/includes/db/sql_insert_buffer.php index 46d397e7b4..c18f908429 100644 --- a/phpBB/includes/db/sql_insert_buffer.php +++ b/phpBB/includes/db/sql_insert_buffer.php @@ -118,11 +118,12 @@ class phpbb_db_sql_insert_buffer */ public function insert_all(array $rows) { - $result = false; + // Using bitwise |= because PHP does not have logical ||= + $result = 0; foreach ($rows as $row) { - $result |= $this->insert($row); + $result |= (int) $this->insert($row); } return (bool) $result;