From 36071ded9d37f38d446d17013b90dff9da94245b Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Sat, 31 May 2014 12:56:44 +0200 Subject: [PATCH 01/11] [ticket/12639] Delete entry in admin-log leads to mysql-error PHPBB3-12639 --- phpBB/includes/acp/acp_logs.php | 2 -- phpBB/includes/mcp/mcp_logs.php | 2 -- phpBB/phpbb/log/log.php | 4 ++-- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/phpBB/includes/acp/acp_logs.php b/phpBB/includes/acp/acp_logs.php index 6b7ed1d269..270cf02be7 100644 --- a/phpBB/includes/acp/acp_logs.php +++ b/phpBB/includes/acp/acp_logs.php @@ -78,8 +78,6 @@ class acp_logs $conditions['keywords'] = $keywords; } - $conditions['log_type'] = $this->log_type; - $phpbb_log = $phpbb_container->get('log'); $phpbb_log->delete($mode, $conditions); } diff --git a/phpBB/includes/mcp/mcp_logs.php b/phpBB/includes/mcp/mcp_logs.php index a0c1bc02ec..bcaf6277a8 100644 --- a/phpBB/includes/mcp/mcp_logs.php +++ b/phpBB/includes/mcp/mcp_logs.php @@ -115,7 +115,6 @@ class mcp_logs if ($deletemark && sizeof($marked)) { $conditions = array( - 'log_type' => LOG_MOD, 'forum_id' => $forum_list, 'log_id' => $marked, ); @@ -127,7 +126,6 @@ class mcp_logs $keywords = utf8_normalize_nfc(request_var('keywords', '', true)); $conditions = array( - 'log_type' => LOG_MOD, 'forum_id' => $forum_list, 'keywords' => $keywords, ); diff --git a/phpBB/phpbb/log/log.php b/phpBB/phpbb/log/log.php index 453cb740bb..6217a7fe46 100644 --- a/phpBB/phpbb/log/log.php +++ b/phpBB/phpbb/log/log.php @@ -393,14 +393,14 @@ class log implements \phpbb\log\log_interface $sql_where = 'WHERE log_type = ' . $log_type; foreach ($conditions as $field => $field_value) { - $sql_where .= ' AND '; - if ($field == 'keywords') { $sql_where .= $this->generate_sql_keyword($field_value, '', ''); } else { + $sql_where .= ' AND '; + if (is_array($field_value) && sizeof($field_value) == 2 && !is_array($field_value[1])) { $sql_where .= $field . ' ' . $field_value[0] . ' ' . $field_value[1]; From 9c497a7b462a24b980912af669a44bfb82f5bdc2 Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Sat, 31 May 2014 13:34:04 +0200 Subject: [PATCH 02/11] [ticket/12639] Add a test case with an empty keywords list PHPBB3-12639 --- phpBB/phpbb/log/log.php | 2 +- tests/log/delete_test.php | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/phpBB/phpbb/log/log.php b/phpBB/phpbb/log/log.php index 6217a7fe46..0dce9306df 100644 --- a/phpBB/phpbb/log/log.php +++ b/phpBB/phpbb/log/log.php @@ -395,7 +395,7 @@ class log implements \phpbb\log\log_interface { if ($field == 'keywords') { - $sql_where .= $this->generate_sql_keyword($field_value, '', ''); + $sql_where .= $this->generate_sql_keyword($field_value, '', ' AND'); } else { diff --git a/tests/log/delete_test.php b/tests/log/delete_test.php index f10e3e582b..14895de059 100644 --- a/tests/log/delete_test.php +++ b/tests/log/delete_test.php @@ -56,5 +56,10 @@ class phpbb_log_delete_test extends phpbb_database_test_case $this->assertCount(3, $log->get_logs('critical', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC')); $log->delete('critical', array('user_id' => array('>', 1))); $this->assertCount(1, $log->get_logs('critical', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC')); + + // Delete with an empty keyword list + $this->assertCount(1, $log->get_logs('critical', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC')); + $log->delete('critical', array('keywords' => '')); + $this->assertEmpty($log->get_logs('critical', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC')); } } From fd9c17ca0486c3000e263dcd53848e3610ec07fc Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Sat, 31 May 2014 17:42:40 +0200 Subject: [PATCH 03/11] [ticket/12639] Update tests to use a dataProvider PHPBB3-12639 --- tests/log/delete_test.php | 159 ++++++++++++++++++++++++++++++-------- 1 file changed, 128 insertions(+), 31 deletions(-) diff --git a/tests/log/delete_test.php b/tests/log/delete_test.php index 14895de059..80fde9db8a 100644 --- a/tests/log/delete_test.php +++ b/tests/log/delete_test.php @@ -1,9 +1,13 @@ +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. * */ @@ -13,53 +17,146 @@ require_once dirname(__FILE__) . '/../../phpBB/includes/utf/utf_tools.php'; class phpbb_log_delete_test extends phpbb_database_test_case { + protected $log; + public function getDataSet() { return $this->createXMLDataSet(dirname(__FILE__) . '/fixtures/delete_log.xml'); } - public function test_log_delete() + protected function setUp() { global $phpbb_root_path, $phpEx, $db, $phpbb_dispatcher, $auth; $db = $this->new_dbal(); - $cache = new phpbb_mock_cache; $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); $user = $this->getMock('\phpbb\user'); $user->data['user_id'] = 1; $auth = $this->getMock('\phpbb\auth\auth'); - $log = new \phpbb\log\log($db, $user, $auth, $phpbb_dispatcher, $phpbb_root_path, 'adm/', $phpEx, LOG_TABLE); + $this->log = new \phpbb\log\log($db, $user, $auth, $phpbb_dispatcher, $phpbb_root_path, 'adm/', $phpEx, LOG_TABLE); - // Delete all admin logs - $this->assertCount(2, $log->get_logs('admin')); - $log->delete('admin'); - // One entry is added to the admin log when the logs are purged - $this->assertCount(1, $log->get_logs('admin')); + parent::setUp(); + } - // Delete with keyword - $this->assertCount(1, $log->get_logs('mod', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC', 'guest')); - $log->delete('mod', array('keywords' => 'guest')); - $this->assertEmpty($log->get_logs('mod', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC', 'guest')); - - // Delete with simples conditions - $this->assertCount(3, $log->get_logs('mod', false, 0, 0, 12, 0, 1, 0, 'l.log_time DESC')); - $log->delete('mod', array('forum_id' => 12, 'user_id' => 1)); - $this->assertEmpty($log->get_logs('mod', false, 0, 0, 12, 0, 1, 0, 'l.log_time DESC')); - - // Delete with IN condition - $this->assertCount(2, $log->get_logs('mod', false, 0, 0, array(13, 14), 0, 0, 0, 'l.log_time DESC')); - $log->delete('mod', array('forum_id' => array('IN' => array(14, 13)))); - $this->assertEmpty($log->get_logs('mod', false, 0, 0, array(13, 14), 0, 0, 0, 'l.log_time DESC')); - - // Delete with a custom condition (ie: WHERE x >= 10) - $this->assertCount(3, $log->get_logs('critical', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC')); - $log->delete('critical', array('user_id' => array('>', 1))); - $this->assertCount(1, $log->get_logs('critical', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC')); + public function log_delete_data() + { + return array( + array( + array(1, 2), + array(16), + array(), + 'admin', + false, + 0, + 0, + 0, + 0, + 0, + 0, + 'l.log_time DESC', + '', + ), + array( + array(11), + array(), + array('keywords' => 'guest'), + 'mod', + false, + 0, + 0, + 0, + 0, + 0, + 0, + 'l.log_time DESC', + 'guest', + ), + array( + array(4, 5, 7), + array(), + array('forum_id' => 12, 'user_id' => 1), + 'mod', + false, + 0, + 0, + 12, + 0, + 1, + 0, + 'l.log_time DESC', + '', + ), + array( + array(12, 13), + array(), + array('forum_id' => array('IN' => array(14, 13))), + 'mod', + false, + 0, + 0, + array(13, 14), + 0, + 0, + 0, + 'l.log_time DESC', + '', + ), + array( + array(3, 14, 15), + array(3), + array('user_id' => array('>', 1)), + 'critical', + false, + 0, + 0, + 0, + 0, + 0, + 0, + 'l.log_time DESC', + '', + ), + array( + array(3, 14, 15), + array(), + array('keywords' => ''), + 'critical', + false, + 0, + 0, + 0, + 0, + 0, + 0, + 'l.log_time DESC', + '', + ), + ); + } + /** + * @dataProvider log_delete_data + */ + public function test_log_delete($expected_before, $expected_after, $delete_conditions, $mode, $count_logs, $limit, $offset, $forum_id, $topic_id, $user_id, $log_time, $sort_by, $keywords) + { + $this->assertEquals($expected_before, $this->get_ids($this->log->get_logs($mode, $count_logs, $limit, $offset, $forum_id, $topic_id, $user_id, $log_time, $sort_by, $keywords)), 'before'); + $this->log->delete($mode, $delete_conditions); + $this->assertEquals($expected_after, $this->get_ids($this->log->get_logs($mode, $count_logs, $limit, $offset, $forum_id, $topic_id, $user_id, $log_time, $sort_by, $keywords)), 'after'); +/* // Delete with an empty keyword list $this->assertCount(1, $log->get_logs('critical', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC')); $log->delete('critical', array('keywords' => '')); - $this->assertEmpty($log->get_logs('critical', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC')); + $this->assertEmpty($log->get_logs('critical', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC'));*/ + } + + public function get_ids($logs) + { + $ids = array(); + foreach ($logs as $log_entry) + { + $ids[] = $log_entry['id']; + } + return $ids; } } From dce7c7e0e07f1a900d88e4db847a464cca826d07 Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Sat, 31 May 2014 18:10:12 +0200 Subject: [PATCH 04/11] [ticket/12639] Fix tests on postgres PHPBB3-12639 --- tests/log/delete_test.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/log/delete_test.php b/tests/log/delete_test.php index 80fde9db8a..e20c8c121f 100644 --- a/tests/log/delete_test.php +++ b/tests/log/delete_test.php @@ -54,7 +54,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 0, 0, - 'l.log_time DESC', + 'l.log_id DESC', '', ), array( @@ -69,7 +69,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 0, 0, - 'l.log_time DESC', + 'l.log_id DESC', 'guest', ), array( @@ -84,7 +84,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 1, 0, - 'l.log_time DESC', + 'l.log_id DESC', '', ), array( @@ -99,7 +99,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 0, 0, - 'l.log_time DESC', + 'l.log_id DESC', '', ), array( @@ -114,7 +114,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 0, 0, - 'l.log_time DESC', + 'l.log_id DESC', '', ), array( @@ -129,7 +129,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 0, 0, - 'l.log_time DESC', + 'l.log_id DESC', '', ), ); From 6d3bc7a60b2da5180bd1ac1f849114b2dadfca78 Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Sat, 31 May 2014 19:00:15 +0200 Subject: [PATCH 05/11] [ticket/12639] Order the results correctly in the test PHPBB3-12639 --- tests/log/delete_test.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/tests/log/delete_test.php b/tests/log/delete_test.php index e20c8c121f..3c97d0c36a 100644 --- a/tests/log/delete_test.php +++ b/tests/log/delete_test.php @@ -54,7 +54,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 0, 0, - 'l.log_id DESC', + 'l.log_id ASC', '', ), array( @@ -69,7 +69,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 0, 0, - 'l.log_id DESC', + 'l.log_id ASC', 'guest', ), array( @@ -84,7 +84,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 1, 0, - 'l.log_id DESC', + 'l.log_id ASC', '', ), array( @@ -99,7 +99,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 0, 0, - 'l.log_id DESC', + 'l.log_id ASC', '', ), array( @@ -114,7 +114,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 0, 0, - 'l.log_id DESC', + 'l.log_id ASC', '', ), array( @@ -129,7 +129,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case 0, 0, 0, - 'l.log_id DESC', + 'l.log_id ASC', '', ), ); From 6794c6b79b751fe90f9c37020276e4510e41150e Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Sun, 1 Jun 2014 01:59:29 +0200 Subject: [PATCH 06/11] [ticket/12639] Remove old commented tests PHPBB3-12639 --- tests/log/delete_test.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/log/delete_test.php b/tests/log/delete_test.php index 3c97d0c36a..188e5138d0 100644 --- a/tests/log/delete_test.php +++ b/tests/log/delete_test.php @@ -143,11 +143,6 @@ class phpbb_log_delete_test extends phpbb_database_test_case $this->assertEquals($expected_before, $this->get_ids($this->log->get_logs($mode, $count_logs, $limit, $offset, $forum_id, $topic_id, $user_id, $log_time, $sort_by, $keywords)), 'before'); $this->log->delete($mode, $delete_conditions); $this->assertEquals($expected_after, $this->get_ids($this->log->get_logs($mode, $count_logs, $limit, $offset, $forum_id, $topic_id, $user_id, $log_time, $sort_by, $keywords)), 'after'); -/* - // Delete with an empty keyword list - $this->assertCount(1, $log->get_logs('critical', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC')); - $log->delete('critical', array('keywords' => '')); - $this->assertEmpty($log->get_logs('critical', false, 0, 0, 0, 0, 0, 0, 'l.log_time DESC'));*/ } public function get_ids($logs) From 8e2a0caf54376b1a6598562e4fc4518f6e255fba Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Sun, 1 Jun 2014 02:57:16 +0200 Subject: [PATCH 07/11] [ticket/12639] Use assertSame PHPBB3-12639 --- tests/log/delete_test.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/log/delete_test.php b/tests/log/delete_test.php index 188e5138d0..b8be15efa5 100644 --- a/tests/log/delete_test.php +++ b/tests/log/delete_test.php @@ -140,9 +140,9 @@ class phpbb_log_delete_test extends phpbb_database_test_case */ public function test_log_delete($expected_before, $expected_after, $delete_conditions, $mode, $count_logs, $limit, $offset, $forum_id, $topic_id, $user_id, $log_time, $sort_by, $keywords) { - $this->assertEquals($expected_before, $this->get_ids($this->log->get_logs($mode, $count_logs, $limit, $offset, $forum_id, $topic_id, $user_id, $log_time, $sort_by, $keywords)), 'before'); + $this->assertSame($expected_before, $this->get_ids($this->log->get_logs($mode, $count_logs, $limit, $offset, $forum_id, $topic_id, $user_id, $log_time, $sort_by, $keywords)), 'before'); $this->log->delete($mode, $delete_conditions); - $this->assertEquals($expected_after, $this->get_ids($this->log->get_logs($mode, $count_logs, $limit, $offset, $forum_id, $topic_id, $user_id, $log_time, $sort_by, $keywords)), 'after'); + $this->assertSame($expected_after, $this->get_ids($this->log->get_logs($mode, $count_logs, $limit, $offset, $forum_id, $topic_id, $user_id, $log_time, $sort_by, $keywords)), 'after'); } public function get_ids($logs) @@ -150,7 +150,7 @@ class phpbb_log_delete_test extends phpbb_database_test_case $ids = array(); foreach ($logs as $log_entry) { - $ids[] = $log_entry['id']; + $ids[] = (int) $log_entry['id']; } return $ids; } From 5b5f3a5d0cf323d6eba6508b8cf2a65e6d8293c0 Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Mon, 2 Jun 2014 23:12:36 +0200 Subject: [PATCH 08/11] [ticket/12639] Send a correct IN entry when deleting marked logs PHPBB3-12639 --- phpBB/includes/acp/acp_logs.php | 2 +- phpBB/includes/mcp/mcp_logs.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/includes/acp/acp_logs.php b/phpBB/includes/acp/acp_logs.php index 270cf02be7..ee9d286f34 100644 --- a/phpBB/includes/acp/acp_logs.php +++ b/phpBB/includes/acp/acp_logs.php @@ -63,7 +63,7 @@ class acp_logs { $sql_in[] = $mark; } - $conditions['log_id'] = $sql_in; + $conditions['log_id'] = array('IN' => $sql_in); unset($sql_in); } diff --git a/phpBB/includes/mcp/mcp_logs.php b/phpBB/includes/mcp/mcp_logs.php index bcaf6277a8..2945e1ec8a 100644 --- a/phpBB/includes/mcp/mcp_logs.php +++ b/phpBB/includes/mcp/mcp_logs.php @@ -116,7 +116,7 @@ class mcp_logs { $conditions = array( 'forum_id' => $forum_list, - 'log_id' => $marked, + 'log_id' => array('IN' => $marked), ); $phpbb_log->delete('mod', $conditions); From 3e31764007dd68ef438bad2a2437ba48707f6831 Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Mon, 2 Jun 2014 23:22:36 +0200 Subject: [PATCH 09/11] [ticket/12639] Don't make a copy of $marked when deleting logs in acp_logs PHPBB3-12639 --- phpBB/includes/acp/acp_logs.php | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/phpBB/includes/acp/acp_logs.php b/phpBB/includes/acp/acp_logs.php index ee9d286f34..80dee1d620 100644 --- a/phpBB/includes/acp/acp_logs.php +++ b/phpBB/includes/acp/acp_logs.php @@ -58,13 +58,7 @@ class acp_logs if ($deletemark && sizeof($marked)) { - $sql_in = array(); - foreach ($marked as $mark) - { - $sql_in[] = $mark; - } - $conditions['log_id'] = array('IN' => $sql_in); - unset($sql_in); + $conditions['log_id'] = array('IN' => $marked); } if ($deleteall) From f980fed5d26e2a7d2e1c139da7e1d8be7c1523e3 Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Mon, 2 Jun 2014 23:35:35 +0200 Subject: [PATCH 10/11] [ticket/12639] Handle $conditions['keywords'] outside of the loop PHPBB3-12639 --- phpBB/phpbb/log/log.php | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/phpBB/phpbb/log/log.php b/phpBB/phpbb/log/log.php index 0dce9306df..d2c946d28b 100644 --- a/phpBB/phpbb/log/log.php +++ b/phpBB/phpbb/log/log.php @@ -391,28 +391,29 @@ class log implements \phpbb\log\log_interface } $sql_where = 'WHERE log_type = ' . $log_type; + + if (isset($conditions['keywords'])) + { + $sql_where .= $this->generate_sql_keyword($conditions['keywords'], '', ' AND'); + + unset($conditions['keywords']); + } + foreach ($conditions as $field => $field_value) { - if ($field == 'keywords') + $sql_where .= ' AND '; + + if (is_array($field_value) && sizeof($field_value) == 2 && !is_array($field_value[1])) { - $sql_where .= $this->generate_sql_keyword($field_value, '', ' AND'); + $sql_where .= $field . ' ' . $field_value[0] . ' ' . $field_value[1]; + } + else if (is_array($field_value) && isset($field_value['IN']) && is_array($field_value['IN'])) + { + $sql_where .= $this->db->sql_in_set($field, $field_value['IN']); } else { - $sql_where .= ' AND '; - - if (is_array($field_value) && sizeof($field_value) == 2 && !is_array($field_value[1])) - { - $sql_where .= $field . ' ' . $field_value[0] . ' ' . $field_value[1]; - } - else if (is_array($field_value) && isset($field_value['IN']) && is_array($field_value['IN'])) - { - $sql_where .= $this->db->sql_in_set($field, $field_value['IN']); - } - else - { - $sql_where .= $field . ' = ' . $field_value; - } + $sql_where .= $field . ' = ' . $field_value; } } From fb46e42ab3393f3899c0aa8b6b4482214ec1d275 Mon Sep 17 00:00:00 2001 From: Tristan Darricau Date: Wed, 4 Jun 2014 20:22:22 +0200 Subject: [PATCH 11/11] [ticket/12639] Add a space in the code generated by generate_sql_keyword() PHPBB3-12639 --- phpBB/phpbb/log/log.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/log/log.php b/phpBB/phpbb/log/log.php index d2c946d28b..10efe5fd1c 100644 --- a/phpBB/phpbb/log/log.php +++ b/phpBB/phpbb/log/log.php @@ -394,7 +394,7 @@ class log implements \phpbb\log\log_interface if (isset($conditions['keywords'])) { - $sql_where .= $this->generate_sql_keyword($conditions['keywords'], '', ' AND'); + $sql_where .= $this->generate_sql_keyword($conditions['keywords'], ''); unset($conditions['keywords']); } @@ -782,7 +782,7 @@ class log implements \phpbb\log\log_interface } } - $sql_keywords = $statement_operator . ' ('; + $sql_keywords = ' ' . $statement_operator . ' ('; if (!empty($operations)) { $sql_keywords .= $this->db->sql_in_set($table_alias . 'log_operation', $operations) . ' OR ';