From 9d4d212e0f71789e1f0332046dd852d80ab9c8ba Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Thu, 24 Oct 2013 13:55:23 +0200 Subject: [PATCH 1/6] [ticket/11525] Only remove group or user prefix from given avatar data Until now, the user data had both user_id and group_id keys in the avatar data. As both group_ and user_ prefixes were removed the group_id was collapsed onto the user_id and therefore all users in the same group had the same prefix for their uploaded avatars. This patch will make sure that the correct id is used depending on whether it's a group's or user's avatar data. PHPBB3-11525 --- phpBB/includes/acp/acp_groups.php | 2 +- phpBB/includes/acp/acp_users.php | 2 +- phpBB/includes/functions_display.php | 4 +-- phpBB/includes/ucp/ucp_groups.php | 2 +- phpBB/includes/ucp/ucp_profile.php | 2 +- phpBB/phpbb/avatar/manager.php | 14 +++++---- tests/avatar/manager_test.php | 44 ++++++++++++++++++++++++++-- 7 files changed, 55 insertions(+), 15 deletions(-) diff --git a/phpBB/includes/acp/acp_groups.php b/phpBB/includes/acp/acp_groups.php index ad29a5521b..8f417e753c 100644 --- a/phpBB/includes/acp/acp_groups.php +++ b/phpBB/includes/acp/acp_groups.php @@ -324,7 +324,7 @@ class acp_groups $avatar_drivers = $phpbb_avatar_manager->get_enabled_drivers(); // This is normalised data, without the group_ prefix - $avatar_data = \phpbb\avatar\manager::clean_row($group_row); + $avatar_data = \phpbb\avatar\manager::clean_row($group_row, 'group'); } diff --git a/phpBB/includes/acp/acp_users.php b/phpBB/includes/acp/acp_users.php index 8853200ddc..9feb1074d0 100644 --- a/phpBB/includes/acp/acp_users.php +++ b/phpBB/includes/acp/acp_users.php @@ -1742,7 +1742,7 @@ class acp_users $avatar_drivers = $phpbb_avatar_manager->get_enabled_drivers(); // This is normalised data, without the user_ prefix - $avatar_data = \phpbb\avatar\manager::clean_row($user_row); + $avatar_data = \phpbb\avatar\manager::clean_row($user_row, 'user'); if ($submit) { diff --git a/phpBB/includes/functions_display.php b/phpBB/includes/functions_display.php index c6ab5df90f..f03e4c01d0 100644 --- a/phpBB/includes/functions_display.php +++ b/phpBB/includes/functions_display.php @@ -1352,7 +1352,7 @@ function get_user_rank($user_rank, $user_posts, &$rank_title, &$rank_img, &$rank */ function phpbb_get_user_avatar($user_row, $alt = 'USER_AVATAR', $ignore_config = false) { - $row = \phpbb\avatar\manager::clean_row($user_row); + $row = \phpbb\avatar\manager::clean_row($user_row, 'user'); return phpbb_get_avatar($row, $alt, $ignore_config); } @@ -1367,7 +1367,7 @@ function phpbb_get_user_avatar($user_row, $alt = 'USER_AVATAR', $ignore_config = */ function phpbb_get_group_avatar($user_row, $alt = 'GROUP_AVATAR', $ignore_config = false) { - $row = \phpbb\avatar\manager::clean_row($user_row); + $row = \phpbb\avatar\manager::clean_row($user_row, 'group'); return phpbb_get_avatar($row, $alt, $ignore_config); } diff --git a/phpBB/includes/ucp/ucp_groups.php b/phpBB/includes/ucp/ucp_groups.php index a75d2e9bfc..32b27b55b4 100644 --- a/phpBB/includes/ucp/ucp_groups.php +++ b/phpBB/includes/ucp/ucp_groups.php @@ -465,7 +465,7 @@ class ucp_groups $avatar_drivers = $phpbb_avatar_manager->get_enabled_drivers(); // This is normalised data, without the group_ prefix - $avatar_data = \phpbb\avatar\manager::clean_row($group_row); + $avatar_data = \phpbb\avatar\manager::clean_row($group_row, 'group'); } // Did we submit? diff --git a/phpBB/includes/ucp/ucp_profile.php b/phpBB/includes/ucp/ucp_profile.php index 3f58ce20b4..f7c6aca9e8 100644 --- a/phpBB/includes/ucp/ucp_profile.php +++ b/phpBB/includes/ucp/ucp_profile.php @@ -567,7 +567,7 @@ class ucp_profile $avatar_drivers = $phpbb_avatar_manager->get_enabled_drivers(); // This is normalised data, without the user_ prefix - $avatar_data = \phpbb\avatar\manager::clean_row($user->data); + $avatar_data = \phpbb\avatar\manager::clean_row($user->data, 'user'); if ($submit) { diff --git a/phpBB/phpbb/avatar/manager.php b/phpBB/phpbb/avatar/manager.php index c28380a401..f2bb1a5dbe 100644 --- a/phpBB/phpbb/avatar/manager.php +++ b/phpBB/phpbb/avatar/manager.php @@ -178,14 +178,15 @@ class manager } /** - * Strip out user_ and group_ prefixes from keys + * Strip out user_, group_, or other prefixes from array keys * * @param array $row User data or group data + * @param string $prefix Prefix of data keys * * @return array User data or group data with keys that have been * stripped from the preceding "user_" or "group_" */ - static public function clean_row($row) + static public function clean_row($row, $prefix = '') { // Upon creation of a user/group $row might be empty if (empty($row)) @@ -196,7 +197,7 @@ class manager $keys = array_keys($row); $values = array_values($row); - $keys = array_map(array('\phpbb\avatar\manager', 'strip_prefix'), $keys); + array_walk($keys, array('\phpbb\avatar\manager', 'strip_prefix'), $prefix); return array_combine($keys, $values); } @@ -205,11 +206,12 @@ class manager * Strip prepending user_ or group_ prefix from key * * @param string Array key - * @return string Key that has been stripped from its prefix + * @return void */ - static protected function strip_prefix($key) + static protected function strip_prefix(&$key, $null, $prefix) { - return preg_replace('#^(?:user_|group_)#', '', $key); + $regex = ($prefix !== '') ? "#^(?:{$prefix}_)#" : '#^(?:user_|group_)#'; + $key = preg_replace($regex, '', $key); } /** diff --git a/tests/avatar/manager_test.php b/tests/avatar/manager_test.php index 4afa594beb..f687f7bc86 100644 --- a/tests/avatar/manager_test.php +++ b/tests/avatar/manager_test.php @@ -201,20 +201,58 @@ class phpbb_avatar_manager_test extends PHPUnit_Framework_TestCase 'foobar_avatar_height' => '', ), ), + array( + array( + 'user_avatar' => '', + 'user_id' => 5, + 'group_id' => 4, + ), + array( + 'avatar' => '', + 'id' => 4, + ), + ), + array( + array( + 'user_avatar' => '', + 'user_id' => 5, + 'group_id' => 4, + ), + array( + 'avatar' => '', + 'id' => 5, + 'group_id' => 4, + ), + 'user', + ), + array( + array( + 'group_avatar' => '', + 'user_id' => 5, + 'group_id' => 4, + ), + array( + 'avatar' => '', + 'id' => 4, + 'user_id' => 5, + ), + 'group', + ), ); } /** * @dataProvider database_row_data */ - public function test_clean_row(array $input, array $output) + public function test_clean_row(array $input, array $output, $prefix = '') { $cleaned_row = array(); - $cleaned_row = \phpbb\avatar\manager::clean_row($input); - foreach ($output as $key => $null) + $cleaned_row = \phpbb\avatar\manager::clean_row($input, $prefix); + foreach ($output as $key => $value) { $this->assertArrayHasKey($key, $cleaned_row); + $this->assertEquals($output[$key], $value); } } From 47f2caff6b3f05f6703e359bf4712bd69d23c04c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Sun, 10 Nov 2013 22:19:06 +0100 Subject: [PATCH 2/6] [ticket/11525] Fix doc blocks PHPBB3-11525 --- phpBB/phpbb/avatar/manager.php | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/phpBB/phpbb/avatar/manager.php b/phpBB/phpbb/avatar/manager.php index f2bb1a5dbe..90cd83898f 100644 --- a/phpBB/phpbb/avatar/manager.php +++ b/phpBB/phpbb/avatar/manager.php @@ -180,8 +180,8 @@ class manager /** * Strip out user_, group_, or other prefixes from array keys * - * @param array $row User data or group data - * @param string $prefix Prefix of data keys + * @param array $row User data or group data + * @param string $prefix Prefix of data keys (e.g. user), should not include the trailing underscore * * @return array User data or group data with keys that have been * stripped from the preceding "user_" or "group_" @@ -205,8 +205,11 @@ class manager /** * Strip prepending user_ or group_ prefix from key * - * @param string Array key - * @return void + * @param string $key Array key + * @param string $null Parameter is ignored by the function, just required by the array_walk + * @param string $prefix Prefix that should be stripped off from the keys (e.g. user) + * Should not include the trailing underscore + * @return null */ static protected function strip_prefix(&$key, $null, $prefix) { From aa84f7de04b0efdf871d75694aee60e5ecf37f56 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Sun, 10 Nov 2013 23:07:07 +0100 Subject: [PATCH 3/6] [ticket/11525] Prefix id parameter with 'g' again when its a group avatar PHPBB3-11525 --- phpBB/phpbb/avatar/manager.php | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/phpBB/phpbb/avatar/manager.php b/phpBB/phpbb/avatar/manager.php index 90cd83898f..9f6a5fb089 100644 --- a/phpBB/phpbb/avatar/manager.php +++ b/phpBB/phpbb/avatar/manager.php @@ -183,8 +183,9 @@ class manager * @param array $row User data or group data * @param string $prefix Prefix of data keys (e.g. user), should not include the trailing underscore * - * @return array User data or group data with keys that have been - * stripped from the preceding "user_" or "group_" + * @return array User or group data with keys that have been + * stripped from the preceding "user_" or "group_" + * Also the group id is prefixed with g, when the prefix group is removed. */ static public function clean_row($row, $prefix = '') { @@ -198,8 +199,14 @@ class manager $values = array_values($row); array_walk($keys, array('\phpbb\avatar\manager', 'strip_prefix'), $prefix); + $row = array_combine($keys, $values); - return array_combine($keys, $values); + if ($prefix == 'group') + { + $row['id'] = 'g' . $row['id']; + } + + return $row; } /** From 32ba402c34922fca9bd0d8a5d02bde62e7334d1a Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 11 Nov 2013 00:01:31 +0100 Subject: [PATCH 4/6] [ticket/11525] Compare correct array to expected value PHPBB3-11525 --- tests/avatar/manager_test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/avatar/manager_test.php b/tests/avatar/manager_test.php index f687f7bc86..47d19be66d 100644 --- a/tests/avatar/manager_test.php +++ b/tests/avatar/manager_test.php @@ -252,7 +252,7 @@ class phpbb_avatar_manager_test extends PHPUnit_Framework_TestCase foreach ($output as $key => $value) { $this->assertArrayHasKey($key, $cleaned_row); - $this->assertEquals($output[$key], $value); + $this->assertEquals($cleaned_row[$key], $value); } } From a74c560ecafc0c6e63f1e0fc8191e85b066a05ef Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 11 Nov 2013 00:02:12 +0100 Subject: [PATCH 5/6] [ticket/11525] Fix expected value of group avatar test PHPBB3-11525 --- tests/avatar/manager_test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/avatar/manager_test.php b/tests/avatar/manager_test.php index 47d19be66d..e9f622bfde 100644 --- a/tests/avatar/manager_test.php +++ b/tests/avatar/manager_test.php @@ -233,7 +233,7 @@ class phpbb_avatar_manager_test extends PHPUnit_Framework_TestCase ), array( 'avatar' => '', - 'id' => 4, + 'id' => 'g4', 'user_id' => 5, ), 'group', From 13a4ceedb18ba938d3cd18e2f68707385bc9283a Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 13 Nov 2013 18:27:40 +0100 Subject: [PATCH 6/6] [ticket/11525] Use foreach instead of array_walk in method clean_row() This approach is cleaner and probably even faster the previous ways that included using array_walk() or array_map() and other helper functions and methods. PHPBB3-11525 --- phpBB/phpbb/avatar/manager.php | 32 ++++++---------------- tests/avatar/manager_test.php | 50 +++++++++------------------------- 2 files changed, 22 insertions(+), 60 deletions(-) diff --git a/phpBB/phpbb/avatar/manager.php b/phpBB/phpbb/avatar/manager.php index 9f6a5fb089..12d7861cdf 100644 --- a/phpBB/phpbb/avatar/manager.php +++ b/phpBB/phpbb/avatar/manager.php @@ -195,33 +195,19 @@ class manager return self::$default_row; } - $keys = array_keys($row); - $values = array_values($row); - - array_walk($keys, array('\phpbb\avatar\manager', 'strip_prefix'), $prefix); - $row = array_combine($keys, $values); - - if ($prefix == 'group') + $output = array(); + foreach ($row as $key => $value) { - $row['id'] = 'g' . $row['id']; + $key = preg_replace("#^(?:{$prefix}_)#", '', $key); + $output[$key] = $value; } - return $row; - } + if ($prefix === 'group' && isset($output['id'])) + { + $output['id'] = 'g' . $output['id']; + } - /** - * Strip prepending user_ or group_ prefix from key - * - * @param string $key Array key - * @param string $null Parameter is ignored by the function, just required by the array_walk - * @param string $prefix Prefix that should be stripped off from the keys (e.g. user) - * Should not include the trailing underscore - * @return null - */ - static protected function strip_prefix(&$key, $null, $prefix) - { - $regex = ($prefix !== '') ? "#^(?:{$prefix}_)#" : '#^(?:user_|group_)#'; - $key = preg_replace($regex, '', $key); + return $output; } /** diff --git a/tests/avatar/manager_test.php b/tests/avatar/manager_test.php index e9f622bfde..f29f7aee95 100644 --- a/tests/avatar/manager_test.php +++ b/tests/avatar/manager_test.php @@ -152,31 +152,20 @@ class phpbb_avatar_manager_test extends PHPUnit_Framework_TestCase return array( array( array( - 'user_avatar' => '', - 'user_avatar_type' => '', - 'user_avatar_width' => '', + 'user_avatar' => '', + 'user_avatar_type' => '', + 'user_avatar_width' => '', 'user_avatar_height' => '', + 'group_avatar' => '', ), array( - 'avatar' => '', - 'avatar_type' => '', - 'avatar_width' => '', - 'avatar_height' => '', - ), - ), - array( - array( - 'group_avatar' => '', - 'group_avatar_type' => '', - 'group_avatar_width' => '', - 'group_avatar_height' => '', - ), - array( - 'avatar' => '', - 'avatar_type' => '', - 'avatar_width' => '', - 'avatar_height' => '', + 'user_avatar' => '', + 'user_avatar_type' => '', + 'user_avatar_width' => '', + 'user_avatar_height' => '', + 'group_avatar' => '', ), + 'foobar', ), array( array(), @@ -187,20 +176,6 @@ class phpbb_avatar_manager_test extends PHPUnit_Framework_TestCase 'avatar_height' => '', ), ), - array( - array( - 'foobar_avatar' => '', - 'foobar_avatar_type' => '', - 'foobar_avatar_width' => '', - 'foobar_avatar_height' => '', - ), - array( - 'foobar_avatar' => '', - 'foobar_avatar_type' => '', - 'foobar_avatar_width' => '', - 'foobar_avatar_height' => '', - ), - ), array( array( 'user_avatar' => '', @@ -208,8 +183,9 @@ class phpbb_avatar_manager_test extends PHPUnit_Framework_TestCase 'group_id' => 4, ), array( - 'avatar' => '', - 'id' => 4, + 'user_avatar' => '', + 'user_id' => 5, + 'group_id' => 4, ), ), array(