From e50718008a44953181e133de1e37304ed6a5994f Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 19 Sep 2015 16:39:17 +0200 Subject: [PATCH 01/36] [ticket/14168] Add attachment upload class PHPBB3-14168 --- .../default/container/services_files.yml | 15 + phpBB/includes/functions_posting.php | 176 +----------- phpBB/phpbb/attachment/upload.php | 261 ++++++++++++++++++ 3 files changed, 285 insertions(+), 167 deletions(-) create mode 100644 phpBB/phpbb/attachment/upload.php diff --git a/phpBB/config/default/container/services_files.yml b/phpBB/config/default/container/services_files.yml index cfdade37df..82e2a2e08f 100644 --- a/phpBB/config/default/container/services_files.yml +++ b/phpBB/config/default/container/services_files.yml @@ -1,4 +1,19 @@ services: + attachment.upload: + class: phpbb\attachment\upload + scope: prototype + arguments: + - @auth + - @cache + - @config + - @files.upload + - @language + - @mimetype.guesser + - @dispatcher + - @plupload + - @user + - %core.root_path% + filesystem: class: phpbb\filesystem\filesystem diff --git a/phpBB/includes/functions_posting.php b/phpBB/includes/functions_posting.php index 8b17dba534..d4923de60c 100644 --- a/phpBB/includes/functions_posting.php +++ b/phpBB/includes/functions_posting.php @@ -391,183 +391,25 @@ function posting_gen_topic_types($forum_id, $cur_topic_type = POST_NORMAL) * Upload Attachment - filedata is generated here * Uses upload class * +* @deprecated 3.2.0-a1 (To be removed: 3.4.0) +* * @param string $form_name The form name of the file upload input * @param int $forum_id The id of the forum * @param bool $local Whether the file is local or not * @param string $local_storage The path to the local file * @param bool $is_message Whether it is a PM or not -* @param \filespec $local_filedata A filespec object created for the local file -* @param \phpbb\mimetype\guesser $mimetype_guesser The mimetype guesser object if used -* @param \phpbb\plupload\plupload $plupload The plupload object if one is being used +* @param array $local_filedata A filespec object created for the local file * -* @return object filespec +* @return \phpbb\files\filespec */ -function upload_attachment($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = false, \phpbb\mimetype\guesser $mimetype_guesser = null, \phpbb\plupload\plupload $plupload = null) +function upload_attachment($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = false) { - global $auth, $user, $config, $db, $cache; - global $phpbb_root_path, $phpEx, $phpbb_dispatcher, $phpbb_container; + global $phpbb_container; - $filedata = array( - 'error' => array() - ); + /** @var \phpbb\attachment\upload $attachment_upload */ + $attachment_upload = $phpbb_container->get('attachment.upload'); - $upload = $phpbb_container->get('files.upload'); - - if ($config['check_attachment_content'] && isset($config['mime_triggers'])) - { - $upload->set_disallowed_content(explode('|', $config['mime_triggers'])); - } - else if (!$config['check_attachment_content']) - { - $upload->set_disallowed_content(array()); - } - - $filedata['post_attach'] = $local || $upload->is_valid($form_name); - - if (!$filedata['post_attach']) - { - $filedata['error'][] = $user->lang['NO_UPLOAD_FORM_FOUND']; - return $filedata; - } - - $extensions = $cache->obtain_attach_extensions((($is_message) ? false : (int) $forum_id)); - $upload->set_allowed_extensions(array_keys($extensions['_allowed_'])); - - /** @var \phpbb\files\filespec $file */ - $file = ($local) ? $upload->handle_upload('files.types.local', $local_storage, $local_filedata) : $upload->handle_upload('files.types.form', $form_name); - - if ($file->init_error()) - { - $filedata['post_attach'] = false; - return $filedata; - } - - // Whether the uploaded file is in the image category - $is_image = (isset($extensions[$file->get('extension')]['display_cat'])) ? $extensions[$file->get('extension')]['display_cat'] == ATTACHMENT_CATEGORY_IMAGE : false; - - if (!$auth->acl_get('a_') && !$auth->acl_get('m_', $forum_id)) - { - // Check Image Size, if it is an image - if ($is_image) - { - $file->upload->set_allowed_dimensions(0, 0, $config['img_max_width'], $config['img_max_height']); - } - - // Admins and mods are allowed to exceed the allowed filesize - if (!empty($extensions[$file->get('extension')]['max_filesize'])) - { - $allowed_filesize = $extensions[$file->get('extension')]['max_filesize']; - } - else - { - $allowed_filesize = ($is_message) ? $config['max_filesize_pm'] : $config['max_filesize']; - } - - $file->upload->set_max_filesize($allowed_filesize); - } - - $file->clean_filename('unique', $user->data['user_id'] . '_'); - - // Are we uploading an image *and* this image being within the image category? - // Only then perform additional image checks. - $file->move_file($config['upload_path'], false, !$is_image); - - // Do we have to create a thumbnail? - $filedata['thumbnail'] = ($is_image && $config['img_create_thumbnail']) ? 1 : 0; - - if (sizeof($file->error)) - { - $file->remove(); - $filedata['error'] = array_merge($filedata['error'], $file->error); - $filedata['post_attach'] = false; - - return $filedata; - } - - // Make sure the image category only holds valid images... - if ($is_image && !$file->is_image()) - { - $file->remove(); - - if ($plupload && $plupload->is_active()) - { - $plupload->emit_error(104, 'ATTACHED_IMAGE_NOT_IMAGE'); - } - - // If this error occurs a user tried to exploit an IE Bug by renaming extensions - // Since the image category is displaying content inline we need to catch this. - trigger_error($user->lang['ATTACHED_IMAGE_NOT_IMAGE']); - } - - $filedata['filesize'] = $file->get('filesize'); - $filedata['mimetype'] = $file->get('mimetype'); - $filedata['extension'] = $file->get('extension'); - $filedata['physical_filename'] = $file->get('realname'); - $filedata['real_filename'] = $file->get('uploadname'); - $filedata['filetime'] = time(); - - /** - * Event to modify uploaded file before submit to the post - * - * @event core.modify_uploaded_file - * @var array filedata Array containing uploaded file data - * @var bool is_image Flag indicating if the file is an image - * @since 3.1.0-RC3 - */ - $vars = array( - 'filedata', - 'is_image', - ); - extract($phpbb_dispatcher->trigger_event('core.modify_uploaded_file', compact($vars))); - - // Check our complete quota - if ($config['attachment_quota']) - { - if ($config['upload_dir_size'] + $file->get('filesize') > $config['attachment_quota']) - { - $filedata['error'][] = $user->lang['ATTACH_QUOTA_REACHED']; - $filedata['post_attach'] = false; - - $file->remove(); - - return $filedata; - } - } - - // Check free disk space - if ($free_space = @disk_free_space($phpbb_root_path . $config['upload_path'])) - { - if ($free_space <= $file->get('filesize')) - { - if ($auth->acl_get('a_')) - { - $filedata['error'][] = $user->lang['ATTACH_DISK_FULL']; - } - else - { - $filedata['error'][] = $user->lang['ATTACH_QUOTA_REACHED']; - } - $filedata['post_attach'] = false; - - $file->remove(); - - return $filedata; - } - } - - // Create Thumbnail - if ($filedata['thumbnail']) - { - $source = $file->get('destination_file'); - $destination = $file->get('destination_path') . '/thumb_' . $file->get('realname'); - - if (!create_thumbnail($source, $destination, $file->get('mimetype'))) - { - $filedata['thumbnail'] = 0; - } - } - - return $filedata; + return $attachment_upload->upload($form_name, $forum_id, $local, $local_storage, $is_message, $local_filedata); } /** diff --git a/phpBB/phpbb/attachment/upload.php b/phpBB/phpbb/attachment/upload.php new file mode 100644 index 0000000000..a175dee0b3 --- /dev/null +++ b/phpBB/phpbb/attachment/upload.php @@ -0,0 +1,261 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +namespace phpbb\attachment; + +use phpbb\auth\auth; +use \phpbb\cache\service; +use \phpbb\config\config; +use \phpbb\event\dispatcher; +use \phpbb\language\language; +use \phpbb\mimetype\guesser; +use \phpbb\plupload\plupload; +use \phpbb\user; + +/** + * Attachment upload class + */ + +class upload +{ + /** @var auth */ + protected $auth; + + /** @var service */ + protected $cache; + + /** @var config */ + protected $config; + + /** @var \phpbb\files\upload Upload class */ + protected $files_upload; + + /** @var \phpbb\language\language */ + protected $language; + + /** @var guesser Mimetype guesser */ + protected $mimetype_guesser; + + /** @var dispatcher */ + protected $phpbb_dispatcher; + + /** @var plupload Plupload */ + protected $plupload; + + /** @var user */ + protected $user; + + /** + * Constructor for attachments upload class + * + * @param auth $auth + * @param service $cache + * @param config $config + * @param \phpbb\files\upload $files_upload + * @param language $language + * @param guesser $mimetype_guesser + * @param dispatcher $phpbb_dispatcher + * @param plupload $plupload + * @param user $user + * @param $phpbb_root_path + */ + public function __construct(auth $auth, service $cache, config $config, \phpbb\files\upload $files_upload, language $language, guesser $mimetype_guesser, dispatcher $phpbb_dispatcher, plupload $plupload, user $user, $phpbb_root_path) + { + $this->auth = $auth; + $this->cache = $cache; + $this->config = $config; + $this->files_upload = $files_upload; + $this->language = $language; + $this->mimetype_guesser = $mimetype_guesser; + $this->phpbb_dispatcher = $phpbb_dispatcher; + $this->plupload = $plupload; + $this->user = $user; + $this->phpbb_root_path = $phpbb_root_path; + } + + /** + * Upload Attachment - filedata is generated here + * Uses upload class + * + * @param string $form_name The form name of the file upload input + * @param int $forum_id The id of the forum + * @param bool $local Whether the file is local or not + * @param string $local_storage The path to the local file + * @param bool $is_message Whether it is a PM or not + * @param array $local_filedata An file data object created for the local file + * + * @return object filespec + */ + public function upload($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = false) + { + $filedata = array( + 'error' => array() + ); + + if ($this->config['check_attachment_content'] && isset($this->config['mime_triggers'])) + { + $this->files_upload->set_disallowed_content(explode('|', $this->config['mime_triggers'])); + } + else if (!$this->config['check_attachment_content']) + { + $this->files_upload->set_disallowed_content(array()); + } + + $filedata['post_attach'] = $local || $this->files_upload->is_valid($form_name); + + if (!$filedata['post_attach']) + { + $filedata['error'][] = $this->user->lang['NO_UPLOAD_FORM_FOUND']; + return $filedata; + } + + $extensions = $this->cache->obtain_attach_extensions((($is_message) ? false : (int) $forum_id)); + $this->files_upload->set_allowed_extensions(array_keys($extensions['_allowed_'])); + + /** @var \phpbb\files\filespec $file */ + $file = ($local) ? $this->files_upload->handle_upload('files.types.local', $local_storage, $local_filedata) : $this->files_upload->handle_upload('files.types.form', $form_name); + + if ($file->init_error()) + { + $filedata['post_attach'] = false; + return $filedata; + } + + // Whether the uploaded file is in the image category + $is_image = (isset($extensions[$file->get('extension')]['display_cat'])) ? $extensions[$file->get('extension')]['display_cat'] == ATTACHMENT_CATEGORY_IMAGE : false; + + if (!$this->auth->acl_get('a_') && !$this->auth->acl_get('m_', $forum_id)) + { + // Check Image Size, if it is an image + if ($is_image) + { + $file->upload->set_allowed_dimensions(0, 0, $this->config['img_max_width'], $this->config['img_max_height']); + } + + // Admins and mods are allowed to exceed the allowed filesize + if (!empty($extensions[$file->get('extension')]['max_filesize'])) + { + $allowed_filesize = $extensions[$file->get('extension')]['max_filesize']; + } + else + { + $allowed_filesize = ($is_message) ? $this->config['max_filesize_pm'] : $this->config['max_filesize']; + } + + $file->upload->set_max_filesize($allowed_filesize); + } + + $file->clean_filename('unique', $this->user->data['user_id'] . '_'); + + // Are we uploading an image *and* this image being within the image category? + // Only then perform additional image checks. + $file->move_file($this->config['upload_path'], false, !$is_image); + + // Do we have to create a thumbnail? + $filedata['thumbnail'] = ($is_image && $this->config['img_create_thumbnail']) ? 1 : 0; + + if (sizeof($file->error)) + { + $file->remove(); + $filedata['error'] = array_merge($filedata['error'], $file->error); + $filedata['post_attach'] = false; + + return $filedata; + } + + // Make sure the image category only holds valid images... + if ($is_image && !$file->is_image()) + { + $file->remove(); + + if ($this->plupload && $this->plupload->is_active()) + { + $this->plupload->emit_error(104, 'ATTACHED_IMAGE_NOT_IMAGE'); + } + + // If this error occurs a user tried to exploit an IE Bug by renaming extensions + // Since the image category is displaying content inline we need to catch this. + trigger_error($this->user->lang['ATTACHED_IMAGE_NOT_IMAGE']); + } + + $filedata['filesize'] = $file->get('filesize'); + $filedata['mimetype'] = $file->get('mimetype'); + $filedata['extension'] = $file->get('extension'); + $filedata['physical_filename'] = $file->get('realname'); + $filedata['real_filename'] = $file->get('uploadname'); + $filedata['filetime'] = time(); + + /** + * Event to modify uploaded file before submit to the post + * + * @event core.modify_uploaded_file + * @var array filedata Array containing uploaded file data + * @var bool is_image Flag indicating if the file is an image + * @since 3.1.0-RC3 + */ + $vars = array( + 'filedata', + 'is_image', + ); + extract($this->phpbb_dispatcher->trigger_event('core.modify_uploaded_file', compact($vars))); + + // Check our complete quota + if ($this->config['attachment_quota']) + { + if (intval($this->config['upload_dir_size']) + $file->get('filesize') > $this->config['attachment_quota']) + { + $filedata['error'][] = $this->user->lang['ATTACH_QUOTA_REACHED']; + $filedata['post_attach'] = false; + + $file->remove(); + + return $filedata; + } + } + + // Check free disk space + if ($free_space = @disk_free_space($this->phpbb_root_path . $this->config['upload_path'])) + { + if ($free_space <= $file->get('filesize')) + { + if ($this->auth->acl_get('a_')) + { + $filedata['error'][] = $this->user->lang['ATTACH_DISK_FULL']; + } + else + { + $filedata['error'][] = $this->user->lang['ATTACH_QUOTA_REACHED']; + } + $filedata['post_attach'] = false; + + $file->remove(); + + return $filedata; + } + } + + // Create Thumbnail + if ($filedata['thumbnail']) + { + $source = $file->get('destination_file'); + $destination = $file->get('destination_path') . '/thumb_' . $file->get('realname'); + + if (!create_thumbnail($source, $destination, $file->get('mimetype'))) + { + $filedata['thumbnail'] = 0; + } + } + + return $filedata; + } +} From 07c8e21e8033f35483b5c96653f616b6f094138e Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 19 Sep 2015 16:45:51 +0200 Subject: [PATCH 02/36] [ticket/14168] Use language class and fix incorrect docblock PHPBB3-14168 --- phpBB/phpbb/attachment/upload.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/phpBB/phpbb/attachment/upload.php b/phpBB/phpbb/attachment/upload.php index a175dee0b3..c7de323699 100644 --- a/phpBB/phpbb/attachment/upload.php +++ b/phpBB/phpbb/attachment/upload.php @@ -96,7 +96,7 @@ class upload * * @return object filespec */ - public function upload($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = false) + public function upload($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = array()) { $filedata = array( 'error' => array() @@ -115,7 +115,7 @@ class upload if (!$filedata['post_attach']) { - $filedata['error'][] = $this->user->lang['NO_UPLOAD_FORM_FOUND']; + $filedata['error'][] = $this->language->lang('NO_UPLOAD_FORM_FOUND'); return $filedata; } @@ -185,7 +185,7 @@ class upload // If this error occurs a user tried to exploit an IE Bug by renaming extensions // Since the image category is displaying content inline we need to catch this. - trigger_error($this->user->lang['ATTACHED_IMAGE_NOT_IMAGE']); + trigger_error($this->language->lang('ATTACHED_IMAGE_NOT_IMAGE')); } $filedata['filesize'] = $file->get('filesize'); @@ -214,7 +214,7 @@ class upload { if (intval($this->config['upload_dir_size']) + $file->get('filesize') > $this->config['attachment_quota']) { - $filedata['error'][] = $this->user->lang['ATTACH_QUOTA_REACHED']; + $filedata['error'][] = $this->language->lang('ATTACH_QUOTA_REACHED'); $filedata['post_attach'] = false; $file->remove(); @@ -230,11 +230,11 @@ class upload { if ($this->auth->acl_get('a_')) { - $filedata['error'][] = $this->user->lang['ATTACH_DISK_FULL']; + $filedata['error'][] = $this->language->lang('ATTACH_DISK_FULL'); } else { - $filedata['error'][] = $this->user->lang['ATTACH_QUOTA_REACHED']; + $filedata['error'][] = $this->language->lang('ATTACH_QUOTA_REACHED'); } $filedata['post_attach'] = false; From 6c1cd26b7a3602932f3571eebcd5a21d1ce8ae51 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 19 Sep 2015 16:50:21 +0200 Subject: [PATCH 03/36] [ticket/14168] Split thumbnail creation to separate method PHPBB3-14168 --- phpBB/phpbb/attachment/upload.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/phpBB/phpbb/attachment/upload.php b/phpBB/phpbb/attachment/upload.php index c7de323699..fa6fbad60d 100644 --- a/phpBB/phpbb/attachment/upload.php +++ b/phpBB/phpbb/attachment/upload.php @@ -245,6 +245,21 @@ class upload } // Create Thumbnail + $filedata = $this->create_thumbnail($file, $filedata); + + return $filedata; + } + + /** + * Create thumbnail for file if necessary + * + * @param \phpbb\files\filespec $file + * @param array $filedata File's filedata + * + * @return array Updated $filedata + */ + protected function create_thumbnail(\phpbb\files\filespec $file, $filedata) + { if ($filedata['thumbnail']) { $source = $file->get('destination_file'); From e10aa45470889cba22b51ac4f43582ecfa131310 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 20 Sep 2015 00:51:51 +0200 Subject: [PATCH 04/36] [ticket/14168] Further split up attachment upload class PHPBB3-14168 --- phpBB/phpbb/attachment/upload.php | 130 ++++++++++++++++++------------ 1 file changed, 78 insertions(+), 52 deletions(-) diff --git a/phpBB/phpbb/attachment/upload.php b/phpBB/phpbb/attachment/upload.php index fa6fbad60d..2880e9e42e 100644 --- a/phpBB/phpbb/attachment/upload.php +++ b/phpBB/phpbb/attachment/upload.php @@ -55,6 +55,12 @@ class upload /** @var user */ protected $user; + /** @var \phpbb\files\filespec Current filespec instance */ + private $file; + + /** @var array Extensions array */ + private $extensions; + /** * Constructor for attachments upload class * @@ -102,14 +108,7 @@ class upload 'error' => array() ); - if ($this->config['check_attachment_content'] && isset($this->config['mime_triggers'])) - { - $this->files_upload->set_disallowed_content(explode('|', $this->config['mime_triggers'])); - } - else if (!$this->config['check_attachment_content']) - { - $this->files_upload->set_disallowed_content(array()); - } + $this->init_files_upload($forum_id, $is_message); $filedata['post_attach'] = $local || $this->files_upload->is_valid($form_name); @@ -119,80 +118,64 @@ class upload return $filedata; } - $extensions = $this->cache->obtain_attach_extensions((($is_message) ? false : (int) $forum_id)); - $this->files_upload->set_allowed_extensions(array_keys($extensions['_allowed_'])); + $this->file = ($local) ? $this->files_upload->handle_upload('files.types.local', $local_storage, $local_filedata) : $this->files_upload->handle_upload('files.types.form', $form_name); - /** @var \phpbb\files\filespec $file */ - $file = ($local) ? $this->files_upload->handle_upload('files.types.local', $local_storage, $local_filedata) : $this->files_upload->handle_upload('files.types.form', $form_name); - - if ($file->init_error()) + if ($this->file->init_error()) { $filedata['post_attach'] = false; return $filedata; } // Whether the uploaded file is in the image category - $is_image = (isset($extensions[$file->get('extension')]['display_cat'])) ? $extensions[$file->get('extension')]['display_cat'] == ATTACHMENT_CATEGORY_IMAGE : false; + $is_image = (isset($this->extensions[$this->file->get('extension')]['display_cat'])) ? $this->extensions[$this->file->get('extension')]['display_cat'] == ATTACHMENT_CATEGORY_IMAGE : false; if (!$this->auth->acl_get('a_') && !$this->auth->acl_get('m_', $forum_id)) { // Check Image Size, if it is an image if ($is_image) { - $file->upload->set_allowed_dimensions(0, 0, $this->config['img_max_width'], $this->config['img_max_height']); + $this->file->upload->set_allowed_dimensions(0, 0, $this->config['img_max_width'], $this->config['img_max_height']); } // Admins and mods are allowed to exceed the allowed filesize - if (!empty($extensions[$file->get('extension')]['max_filesize'])) + if (!empty($this->extensions[$this->file->get('extension')]['max_filesize'])) { - $allowed_filesize = $extensions[$file->get('extension')]['max_filesize']; + $allowed_filesize = $this->extensions[$this->file->get('extension')]['max_filesize']; } else { $allowed_filesize = ($is_message) ? $this->config['max_filesize_pm'] : $this->config['max_filesize']; } - $file->upload->set_max_filesize($allowed_filesize); + $this->file->upload->set_max_filesize($allowed_filesize); } - $file->clean_filename('unique', $this->user->data['user_id'] . '_'); + $this->file->clean_filename('unique', $this->user->data['user_id'] . '_'); // Are we uploading an image *and* this image being within the image category? // Only then perform additional image checks. - $file->move_file($this->config['upload_path'], false, !$is_image); + $this->file->move_file($this->config['upload_path'], false, !$is_image); // Do we have to create a thumbnail? $filedata['thumbnail'] = ($is_image && $this->config['img_create_thumbnail']) ? 1 : 0; - if (sizeof($file->error)) + if (sizeof($this->file->error)) { - $file->remove(); - $filedata['error'] = array_merge($filedata['error'], $file->error); + $this->file->remove(); + $filedata['error'] = array_merge($filedata['error'], $this->file->error); $filedata['post_attach'] = false; return $filedata; } // Make sure the image category only holds valid images... - if ($is_image && !$file->is_image()) - { - $file->remove(); + $this->check_image($is_image); - if ($this->plupload && $this->plupload->is_active()) - { - $this->plupload->emit_error(104, 'ATTACHED_IMAGE_NOT_IMAGE'); - } - - // If this error occurs a user tried to exploit an IE Bug by renaming extensions - // Since the image category is displaying content inline we need to catch this. - trigger_error($this->language->lang('ATTACHED_IMAGE_NOT_IMAGE')); - } - - $filedata['filesize'] = $file->get('filesize'); - $filedata['mimetype'] = $file->get('mimetype'); - $filedata['extension'] = $file->get('extension'); - $filedata['physical_filename'] = $file->get('realname'); - $filedata['real_filename'] = $file->get('uploadname'); + $filedata['filesize'] = $this->file->get('filesize'); + $filedata['mimetype'] = $this->file->get('mimetype'); + $filedata['extension'] = $this->file->get('extension'); + $filedata['physical_filename'] = $this->file->get('realname'); + $filedata['real_filename'] = $this->file->get('uploadname'); $filedata['filetime'] = time(); /** @@ -212,12 +195,12 @@ class upload // Check our complete quota if ($this->config['attachment_quota']) { - if (intval($this->config['upload_dir_size']) + $file->get('filesize') > $this->config['attachment_quota']) + if (intval($this->config['upload_dir_size']) + $this->file->get('filesize') > $this->config['attachment_quota']) { $filedata['error'][] = $this->language->lang('ATTACH_QUOTA_REACHED'); $filedata['post_attach'] = false; - $file->remove(); + $this->file->remove(); return $filedata; } @@ -226,7 +209,7 @@ class upload // Check free disk space if ($free_space = @disk_free_space($this->phpbb_root_path . $this->config['upload_path'])) { - if ($free_space <= $file->get('filesize')) + if ($free_space <= $this->file->get('filesize')) { if ($this->auth->acl_get('a_')) { @@ -238,14 +221,14 @@ class upload } $filedata['post_attach'] = false; - $file->remove(); + $this->file->remove(); return $filedata; } } // Create Thumbnail - $filedata = $this->create_thumbnail($file, $filedata); + $filedata = $this->create_thumbnail($filedata); return $filedata; } @@ -253,19 +236,18 @@ class upload /** * Create thumbnail for file if necessary * - * @param \phpbb\files\filespec $file * @param array $filedata File's filedata * * @return array Updated $filedata */ - protected function create_thumbnail(\phpbb\files\filespec $file, $filedata) + protected function create_thumbnail($filedata) { if ($filedata['thumbnail']) { - $source = $file->get('destination_file'); - $destination = $file->get('destination_path') . '/thumb_' . $file->get('realname'); + $source = $this->file->get('destination_file'); + $destination = $this->file->get('destination_path') . '/thumb_' . $this->file->get('realname'); - if (!create_thumbnail($source, $destination, $file->get('mimetype'))) + if (!create_thumbnail($source, $destination, $this->file->get('mimetype'))) { $filedata['thumbnail'] = 0; } @@ -273,4 +255,48 @@ class upload return $filedata; } + + /** + * Init files upload class + * + * @param int $forum_id Forum ID + * @param bool $is_message Whether attachment is inside PM or not + */ + protected function init_files_upload($forum_id, $is_message) + { + if ($this->config['check_attachment_content'] && isset($this->config['mime_triggers'])) + { + $this->files_upload->set_disallowed_content(explode('|', $this->config['mime_triggers'])); + } + else if (!$this->config['check_attachment_content']) + { + $this->files_upload->set_disallowed_content(array()); + } + + $this->extensions = $this->cache->obtain_attach_extensions((($is_message) ? false : (int) $forum_id)); + $this->files_upload->set_allowed_extensions(array_keys($this->extensions['_allowed_'])); + } + + /** + * Check if uploaded file is really an image + * + * @param bool $is_image Whether file is image + */ + protected function check_image($is_image) + { + // Make sure the image category only holds valid images... + if ($is_image && !$this->file->is_image()) + { + $this->file->remove(); + + if ($this->plupload && $this->plupload->is_active()) + { + $this->plupload->emit_error(104, 'ATTACHED_IMAGE_NOT_IMAGE'); + } + + // If this error occurs a user tried to exploit an IE Bug by renaming extensions + // Since the image category is displaying content inline we need to catch this. + trigger_error($this->language->lang('ATTACHED_IMAGE_NOT_IMAGE')); + } + } } From a60beb6f2f9d75e34c67a53c9b2332e15fe21b4a Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 20 Sep 2015 12:18:58 +0200 Subject: [PATCH 05/36] [ticket/14168] Further split up attachment upload class PHPBB3-14168 --- phpBB/phpbb/attachment/upload.php | 155 ++++++++++++++++++------------ 1 file changed, 94 insertions(+), 61 deletions(-) diff --git a/phpBB/phpbb/attachment/upload.php b/phpBB/phpbb/attachment/upload.php index 2880e9e42e..d57b33b137 100644 --- a/phpBB/phpbb/attachment/upload.php +++ b/phpBB/phpbb/attachment/upload.php @@ -58,6 +58,11 @@ class upload /** @var \phpbb\files\filespec Current filespec instance */ private $file; + /** @var array File data */ + private $file_data = array( + 'error' => array() + ); + /** @var array Extensions array */ private $extensions; @@ -104,26 +109,22 @@ class upload */ public function upload($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = array()) { - $filedata = array( - 'error' => array() - ); - $this->init_files_upload($forum_id, $is_message); - $filedata['post_attach'] = $local || $this->files_upload->is_valid($form_name); + $this->file_data['post_attach'] = $local || $this->files_upload->is_valid($form_name); - if (!$filedata['post_attach']) + if (!$this->file_data['post_attach']) { - $filedata['error'][] = $this->language->lang('NO_UPLOAD_FORM_FOUND'); - return $filedata; + $this->file_data['error'][] = $this->language->lang('NO_UPLOAD_FORM_FOUND'); + return $this->file_data; } $this->file = ($local) ? $this->files_upload->handle_upload('files.types.local', $local_storage, $local_filedata) : $this->files_upload->handle_upload('files.types.form', $form_name); if ($this->file->init_error()) { - $filedata['post_attach'] = false; - return $filedata; + $this->file_data['post_attach'] = false; + return $this->file_data; } // Whether the uploaded file is in the image category @@ -157,26 +158,23 @@ class upload $this->file->move_file($this->config['upload_path'], false, !$is_image); // Do we have to create a thumbnail? - $filedata['thumbnail'] = ($is_image && $this->config['img_create_thumbnail']) ? 1 : 0; + $this->file_data['thumbnail'] = ($is_image && $this->config['img_create_thumbnail']) ? 1 : 0; if (sizeof($this->file->error)) { $this->file->remove(); - $filedata['error'] = array_merge($filedata['error'], $this->file->error); - $filedata['post_attach'] = false; + $this->file_data['error'] = array_merge($this->file_data['error'], $this->file->error); + $this->file_data['post_attach'] = false; - return $filedata; + return $this->file_data; } // Make sure the image category only holds valid images... $this->check_image($is_image); - $filedata['filesize'] = $this->file->get('filesize'); - $filedata['mimetype'] = $this->file->get('mimetype'); - $filedata['extension'] = $this->file->get('extension'); - $filedata['physical_filename'] = $this->file->get('realname'); - $filedata['real_filename'] = $this->file->get('uploadname'); - $filedata['filetime'] = time(); + $this->fill_file_data(); + + $filedata = $this->file_data; /** * Event to modify uploaded file before submit to the post @@ -191,69 +189,38 @@ class upload 'is_image', ); extract($this->phpbb_dispatcher->trigger_event('core.modify_uploaded_file', compact($vars))); + $this->file_data = $filedata; + unset($filedata); - // Check our complete quota - if ($this->config['attachment_quota']) + // Check for attachment quota and free space + if (!$this->check_attach_quota() || !$this->check_disk_space()) { - if (intval($this->config['upload_dir_size']) + $this->file->get('filesize') > $this->config['attachment_quota']) - { - $filedata['error'][] = $this->language->lang('ATTACH_QUOTA_REACHED'); - $filedata['post_attach'] = false; - - $this->file->remove(); - - return $filedata; - } - } - - // Check free disk space - if ($free_space = @disk_free_space($this->phpbb_root_path . $this->config['upload_path'])) - { - if ($free_space <= $this->file->get('filesize')) - { - if ($this->auth->acl_get('a_')) - { - $filedata['error'][] = $this->language->lang('ATTACH_DISK_FULL'); - } - else - { - $filedata['error'][] = $this->language->lang('ATTACH_QUOTA_REACHED'); - } - $filedata['post_attach'] = false; - - $this->file->remove(); - - return $filedata; - } + return $this->file_data; } // Create Thumbnail - $filedata = $this->create_thumbnail($filedata); + $this->create_thumbnail(); - return $filedata; + return $this->file_data; } /** * Create thumbnail for file if necessary * - * @param array $filedata File's filedata - * * @return array Updated $filedata */ - protected function create_thumbnail($filedata) + protected function create_thumbnail() { - if ($filedata['thumbnail']) + if ($this->file_data['thumbnail']) { $source = $this->file->get('destination_file'); $destination = $this->file->get('destination_path') . '/thumb_' . $this->file->get('realname'); if (!create_thumbnail($source, $destination, $this->file->get('mimetype'))) { - $filedata['thumbnail'] = 0; + $this->file_data['thumbnail'] = 0; } } - - return $filedata; } /** @@ -299,4 +266,70 @@ class upload trigger_error($this->language->lang('ATTACHED_IMAGE_NOT_IMAGE')); } } + + /** + * Check if attachment quota was reached + * + * @return bool False if attachment quota was reached, true if not + */ + protected function check_attach_quota() + { + if ($this->config['attachment_quota']) + { + if (intval($this->config['upload_dir_size']) + $this->file->get('filesize') > $this->config['attachment_quota']) + { + $this->file_data['error'][] = $this->language->lang('ATTACH_QUOTA_REACHED'); + $this->file_data['post_attach'] = false; + + $this->file->remove(); + + return false; + } + } + + return true; + } + + /** + * Check if there is enough free space available on disk + * + * @return bool True if disk space is available, false if not + */ + protected function check_disk_space() + { + if ($free_space = @disk_free_space($this->phpbb_root_path . $this->config['upload_path'])) + { + if ($free_space <= $this->file->get('filesize')) + { + if ($this->auth->acl_get('a_')) + { + $this->file_data['error'][] = $this->language->lang('ATTACH_DISK_FULL'); + } + else + { + $this->file_data['error'][] = $this->language->lang('ATTACH_QUOTA_REACHED'); + } + $this->file_data['post_attach'] = false; + + $this->file->remove(); + + return false; + } + } + + return true; + } + + /** + * Fills file data with file information and current time as filetime + */ + protected function fill_file_data() + { + $this->file_data['filesize'] = $this->file->get('filesize'); + $this->file_data['mimetype'] = $this->file->get('mimetype'); + $this->file_data['extension'] = $this->file->get('extension'); + $this->file_data['physical_filename'] = $this->file->get('realname'); + $this->file_data['real_filename'] = $this->file->get('uploadname'); + $this->file_data['filetime'] = time(); + } } From 16d5208d9aec0678b3295b0e14042991094197ba Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 20 Sep 2015 12:36:52 +0200 Subject: [PATCH 06/36] [ticket/14168] Use attachment upload class in message_parser PHPBB3-14168 --- phpBB/includes/message_parser.php | 27 +++++++-------------------- phpBB/posting.php | 1 - 2 files changed, 7 insertions(+), 21 deletions(-) diff --git a/phpBB/includes/message_parser.php b/phpBB/includes/message_parser.php index 31fc1577a2..e0a0d69fba 100644 --- a/phpBB/includes/message_parser.php +++ b/phpBB/includes/message_parser.php @@ -1140,12 +1140,6 @@ class parse_message extends bbcode_firstpass */ protected $plupload; - /** - * The mimetype guesser object used for attachment mimetypes - * @var \phpbb\mimetype\guesser - */ - protected $mimetype_guesser; - /** * Init - give message here or manually */ @@ -1541,6 +1535,7 @@ class parse_message extends bbcode_firstpass function parse_attachments($form_name, $mode, $forum_id, $submit, $preview, $refresh, $is_message = false) { global $config, $auth, $user, $phpbb_root_path, $phpEx, $db, $request; + global $phpbb_container; $error = array(); @@ -1576,7 +1571,9 @@ class parse_message extends bbcode_firstpass { if ($num_attachments < $cfg['max_attachments'] || $auth->acl_get('a_') || $auth->acl_get('m_', $forum_id)) { - $filedata = upload_attachment($form_name, $forum_id, false, '', $is_message); + /** @var \phpbb\attachment\upload $attachment_upload */ + $attachment_upload = $phpbb_container->get('attachment.upload'); + $filedata = $attachment_upload->upload($form_name, $forum_id, false, '', $is_message); $error = $filedata['error']; if ($filedata['post_attach'] && !sizeof($error)) @@ -1692,7 +1689,9 @@ class parse_message extends bbcode_firstpass { if ($num_attachments < $cfg['max_attachments'] || $auth->acl_gets('m_', 'a_', $forum_id)) { - $filedata = upload_attachment($form_name, $forum_id, false, '', $is_message, false, $this->mimetype_guesser, $this->plupload); + /** @var \phpbb\attachment\upload $attachment_upload */ + $attachment_upload = $phpbb_container->get('attachment.upload'); + $filedata = $attachment_upload->upload($form_name, $forum_id, false, '', $is_message);; $error = array_merge($error, $filedata['error']); if (!sizeof($error)) @@ -1980,18 +1979,6 @@ class parse_message extends bbcode_firstpass $this->plupload = $plupload; } - /** - * Setter function for passing the mimetype_guesser object - * - * @param \phpbb\mimetype\guesser $mimetype_guesser The mimetype_guesser object - * - * @return null - */ - public function set_mimetype_guesser(\phpbb\mimetype\guesser $mimetype_guesser) - { - $this->mimetype_guesser = $mimetype_guesser; - } - /** * Function to perform custom bbcode validation by extensions * can be used in bbcode_init() to assign regexp replacement diff --git a/phpBB/posting.php b/phpBB/posting.php index 47fdb2d378..05a078ea39 100644 --- a/phpBB/posting.php +++ b/phpBB/posting.php @@ -571,7 +571,6 @@ $plupload = $phpbb_container->get('plupload'); /* @var $mimetype_guesser \phpbb\mimetype\guesser */ $mimetype_guesser = $phpbb_container->get('mimetype.guesser'); $message_parser->set_plupload($plupload); -$message_parser->set_mimetype_guesser($mimetype_guesser); if (isset($post_data['post_text'])) { From d1030450f5f2ddca88f1ef17cc7b949541e64bd8 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 21 Sep 2015 08:54:57 +0200 Subject: [PATCH 07/36] [ticket/14168] Move function for attachment deletion into class PHPBB3-14168 --- .../default/container/services_files.yml | 8 + phpBB/includes/functions_admin.php | 321 +------------- phpBB/phpbb/attachment/delete.php | 398 ++++++++++++++++++ 3 files changed, 412 insertions(+), 315 deletions(-) create mode 100644 phpBB/phpbb/attachment/delete.php diff --git a/phpBB/config/default/container/services_files.yml b/phpBB/config/default/container/services_files.yml index 82e2a2e08f..12ae3f79c6 100644 --- a/phpBB/config/default/container/services_files.yml +++ b/phpBB/config/default/container/services_files.yml @@ -14,6 +14,14 @@ services: - @user - %core.root_path% + attachment.delete: + class: phpbb\attachment\delete + scope: prototype + arguments: + - @config + - @dbal.conn + - @dispatcher + filesystem: class: phpbb\filesystem\filesystem diff --git a/phpBB/includes/functions_admin.php b/phpBB/includes/functions_admin.php index 3515150f93..aafac876a5 100644 --- a/phpBB/includes/functions_admin.php +++ b/phpBB/includes/functions_admin.php @@ -1111,329 +1111,20 @@ function delete_posts($where_type, $where_ids, $auto_sync = true, $posted_sync = /** * Delete Attachments * +* @deprecated 3.2.0-a1 (To be removed: 3.4.0) +* * @param string $mode can be: post|message|topic|attach|user * @param mixed $ids can be: post_ids, message_ids, topic_ids, attach_ids, user_ids * @param bool $resync set this to false if you are deleting posts or topics */ function delete_attachments($mode, $ids, $resync = true) { - global $db, $config, $phpbb_dispatcher; + global $phpbb_container; - // 0 is as bad as an empty array - if (empty($ids)) - { - return false; - } + /** @var \phpbb\attachment\delete $attachment_delete */ + $attachment_delete = $phpbb_container->get('attachment.delete'); - if (is_array($ids)) - { - $ids = array_unique($ids); - $ids = array_map('intval', $ids); - } - else - { - $ids = array((int) $ids); - } - - $sql_where = ''; - - switch ($mode) - { - case 'post': - case 'message': - $sql_id = 'post_msg_id'; - $sql_where = ' AND in_message = ' . ($mode == 'message' ? 1 : 0); - break; - - case 'topic': - $sql_id = 'topic_id'; - break; - - case 'user': - $sql_id = 'poster_id'; - break; - - case 'attach': - default: - $sql_id = 'attach_id'; - $mode = 'attach'; - break; - } - - $post_ids = $message_ids = $topic_ids = $physical = array(); - - /** - * Perform additional actions before collecting data for attachment(s) deletion - * - * @event core.delete_attachments_collect_data_before - * @var string mode Variable containing attachments deletion mode, can be: post|message|topic|attach|user - * @var mixed ids Array or comma separated list of ids corresponding to the mode - * @var bool resync Flag indicating if posts/messages/topics should be synchronized - * @var string sql_id The field name to collect/delete data for depending on the mode - * @since 3.1.7-RC1 - */ - $vars = array( - 'mode', - 'ids', - 'resync', - 'sql_id', - ); - extract($phpbb_dispatcher->trigger_event('core.delete_attachments_collect_data_before', compact($vars))); - - // Collect post and topic ids for later use if we need to touch remaining entries (if resync is enabled) - $sql = 'SELECT post_msg_id, topic_id, in_message, physical_filename, thumbnail, filesize, is_orphan - FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $db->sql_in_set($sql_id, $ids); - - $sql .= $sql_where; - - $result = $db->sql_query($sql); - - while ($row = $db->sql_fetchrow($result)) - { - // We only need to store post/message/topic ids if resync is enabled and the file is not orphaned - if ($resync && !$row['is_orphan']) - { - if (!$row['in_message']) - { - $post_ids[] = $row['post_msg_id']; - $topic_ids[] = $row['topic_id']; - } - else - { - $message_ids[] = $row['post_msg_id']; - } - } - - $physical[] = array('filename' => $row['physical_filename'], 'thumbnail' => $row['thumbnail'], 'filesize' => $row['filesize'], 'is_orphan' => $row['is_orphan']); - } - $db->sql_freeresult($result); - - /** - * Perform additional actions before attachment(s) deletion - * - * @event core.delete_attachments_before - * @var string mode Variable containing attachments deletion mode, can be: post|message|topic|attach|user - * @var mixed ids Array or comma separated list of ids corresponding to the mode - * @var bool resync Flag indicating if posts/messages/topics should be synchronized - * @var string sql_id The field name to collect/delete data for depending on the mode - * @var array post_ids Array with post ids for deleted attachment(s) - * @var array topic_ids Array with topic ids for deleted attachment(s) - * @var array message_ids Array with private message ids for deleted attachment(s) - * @var array physical Array with deleted attachment(s) physical file(s) data - * @since 3.1.7-RC1 - */ - $vars = array( - 'mode', - 'ids', - 'resync', - 'sql_id', - 'post_ids', - 'topic_ids', - 'message_ids', - 'physical', - ); - extract($phpbb_dispatcher->trigger_event('core.delete_attachments_before', compact($vars))); - - // Delete attachments - $sql = 'DELETE FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $db->sql_in_set($sql_id, $ids); - - $sql .= $sql_where; - - $db->sql_query($sql); - $num_deleted = $db->sql_affectedrows(); - - /** - * Perform additional actions after attachment(s) deletion from the database - * - * @event core.delete_attachments_from_database_after - * @var string mode Variable containing attachments deletion mode, can be: post|message|topic|attach|user - * @var mixed ids Array or comma separated list of ids corresponding to the mode - * @var bool resync Flag indicating if posts/messages/topics should be synchronized - * @var string sql_id The field name to collect/delete data for depending on the mode - * @var array post_ids Array with post ids for deleted attachment(s) - * @var array topic_ids Array with topic ids for deleted attachment(s) - * @var array message_ids Array with private message ids for deleted attachment(s) - * @var array physical Array with deleted attachment(s) physical file(s) data - * @var int num_deleted The number of deleted attachment(s) from the database - * @since 3.1.7-RC1 - */ - $vars = array( - 'mode', - 'ids', - 'resync', - 'sql_id', - 'post_ids', - 'topic_ids', - 'message_ids', - 'physical', - 'num_deleted', - ); - extract($phpbb_dispatcher->trigger_event('core.delete_attachments_from_database_after', compact($vars))); - - if (!$num_deleted) - { - return 0; - } - - // Delete attachments from filesystem - $space_removed = $files_removed = 0; - foreach ($physical as $file_ary) - { - if (phpbb_unlink($file_ary['filename'], 'file', true) && !$file_ary['is_orphan']) - { - // Only non-orphaned files count to the file size - $space_removed += $file_ary['filesize']; - $files_removed++; - } - - if ($file_ary['thumbnail']) - { - phpbb_unlink($file_ary['filename'], 'thumbnail', true); - } - } - - /** - * Perform additional actions after attachment(s) deletion from the filesystem - * - * @event core.delete_attachments_from_filesystem_after - * @var string mode Variable containing attachments deletion mode, can be: post|message|topic|attach|user - * @var mixed ids Array or comma separated list of ids corresponding to the mode - * @var bool resync Flag indicating if posts/messages/topics should be synchronized - * @var string sql_id The field name to collect/delete data for depending on the mode - * @var array post_ids Array with post ids for deleted attachment(s) - * @var array topic_ids Array with topic ids for deleted attachment(s) - * @var array message_ids Array with private message ids for deleted attachment(s) - * @var array physical Array with deleted attachment(s) physical file(s) data - * @var int num_deleted The number of deleted attachment(s) from the database - * @var int space_removed The size of deleted files(s) from the filesystem - * @var int files_removed The number of deleted file(s) from the filesystem - * @since 3.1.7-RC1 - */ - $vars = array( - 'mode', - 'ids', - 'resync', - 'sql_id', - 'post_ids', - 'topic_ids', - 'message_ids', - 'physical', - 'num_deleted', - 'space_removed', - 'files_removed', - ); - extract($phpbb_dispatcher->trigger_event('core.delete_attachments_from_filesystem_after', compact($vars))); - - if ($space_removed || $files_removed) - { - $config->increment('upload_dir_size', $space_removed * (-1), false); - $config->increment('num_files', $files_removed * (-1), false); - } - - // If we do not resync, we do not need to adjust any message, post, topic or user entries - if (!$resync) - { - return $num_deleted; - } - - // No more use for the original ids - unset($ids); - - // Now, we need to resync posts, messages, topics. We go through every one of them - $post_ids = array_unique($post_ids); - $message_ids = array_unique($message_ids); - $topic_ids = array_unique($topic_ids); - - // Update post indicators for posts now no longer having attachments - if (sizeof($post_ids)) - { - // Just check which posts are still having an assigned attachment not orphaned by querying the attachments table - $sql = 'SELECT post_msg_id - FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $db->sql_in_set('post_msg_id', $post_ids) . ' - AND in_message = 0 - AND is_orphan = 0'; - $result = $db->sql_query($sql); - - $remaining_ids = array(); - while ($row = $db->sql_fetchrow($result)) - { - $remaining_ids[] = $row['post_msg_id']; - } - $db->sql_freeresult($result); - - // Now only unset those ids remaining - $post_ids = array_diff($post_ids, $remaining_ids); - - if (sizeof($post_ids)) - { - $sql = 'UPDATE ' . POSTS_TABLE . ' - SET post_attachment = 0 - WHERE ' . $db->sql_in_set('post_id', $post_ids); - $db->sql_query($sql); - } - } - - // Update message table if messages are affected - if (sizeof($message_ids)) - { - // Just check which messages are still having an assigned attachment not orphaned by querying the attachments table - $sql = 'SELECT post_msg_id - FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $db->sql_in_set('post_msg_id', $message_ids) . ' - AND in_message = 1 - AND is_orphan = 0'; - $result = $db->sql_query($sql); - - $remaining_ids = array(); - while ($row = $db->sql_fetchrow($result)) - { - $remaining_ids[] = $row['post_msg_id']; - } - $db->sql_freeresult($result); - - // Now only unset those ids remaining - $message_ids = array_diff($message_ids, $remaining_ids); - - if (sizeof($message_ids)) - { - $sql = 'UPDATE ' . PRIVMSGS_TABLE . ' - SET message_attachment = 0 - WHERE ' . $db->sql_in_set('msg_id', $message_ids); - $db->sql_query($sql); - } - } - - // Now update the topics. This is a bit trickier, because there could be posts still having attachments within the topic - if (sizeof($topic_ids)) - { - // Just check which topics are still having an assigned attachment not orphaned by querying the attachments table (much less entries expected) - $sql = 'SELECT topic_id - FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $db->sql_in_set('topic_id', $topic_ids) . ' - AND is_orphan = 0'; - $result = $db->sql_query($sql); - - $remaining_ids = array(); - while ($row = $db->sql_fetchrow($result)) - { - $remaining_ids[] = $row['topic_id']; - } - $db->sql_freeresult($result); - - // Now only unset those ids remaining - $topic_ids = array_diff($topic_ids, $remaining_ids); - - if (sizeof($topic_ids)) - { - $sql = 'UPDATE ' . TOPICS_TABLE . ' - SET topic_attachment = 0 - WHERE ' . $db->sql_in_set('topic_id', $topic_ids); - $db->sql_query($sql); - } - } + $num_deleted = $attachment_delete->delete($mode, $ids, $resync); return $num_deleted; } diff --git a/phpBB/phpbb/attachment/delete.php b/phpBB/phpbb/attachment/delete.php new file mode 100644 index 0000000000..a2fe1a8cdf --- /dev/null +++ b/phpBB/phpbb/attachment/delete.php @@ -0,0 +1,398 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +namespace phpbb\attachment; + +use \phpbb\config\config; +use \phpbb\db\driver\driver_interface; +use \phpbb\event\dispatcher; + +/** + * Attachment delete class + */ + +class delete +{ + /** @var \phpbb\config\config */ + protected $config; + + /** @var \phpbb\db\driver\driver_interface */ + protected $db; + + /** @var \phpbb\event\dispatcher */ + protected $dispatcher; + + /** @var array Attachement IDs */ + protected $ids; + + /** + * Attachment delete class constructor + * + * @param config $config + * @param driver_interface $db + * @param dispatcher>>>>>>> 85b6020... [ticket/14168] Move function for attachment deletion into class + */ + public function __construct(config $config, driver_interface $db, dispatcher $dispatcher) + { + $this->config = $config; + $this->db = $db; + $this->dispatcher = $dispatcher; + } + + /** + * Delete Attachments + * + * @param string $mode can be: post|message|topic|attach|user + * @param mixed $ids can be: post_ids, message_ids, topic_ids, attach_ids, user_ids + * @param bool $resync set this to false if you are deleting posts or topics + * + * @return int|bool Number of deleted attachments or false if something + * went wrong during attachment deletion + */ + function delete($mode, $ids, $resync = true) + { + if (!$this->set_attachment_ids($ids)) + { + return false; + } + + $sql_where = ''; + + switch ($mode) + { + case 'post': + case 'message': + $sql_id = 'post_msg_id'; + $sql_where = ' AND in_message = ' . ($mode == 'message' ? 1 : 0); + break; + + case 'topic': + $sql_id = 'topic_id'; + break; + + case 'user': + $sql_id = 'poster_id'; + break; + + case 'attach': + default: + $sql_id = 'attach_id'; + break; + } + + $post_ids = $message_ids = $topic_ids = $physical = array(); + + /** + * Perform additional actions before collecting data for attachment(s) deletion + * + * @event core.delete_attachments_collect_data_before + * @var string mode Variable containing attachments deletion mode, can be: post|message|topic|attach|user + * @var mixed ids Array or comma separated list of ids corresponding to the mode + * @var bool resync Flag indicating if posts/messages/topics should be synchronized + * @var string sql_id The field name to collect/delete data for depending on the mode + * @since 3.1.7-RC1 + */ + $vars = array( + 'mode', + 'ids', + 'resync', + 'sql_id', + ); + extract($this->dispatcher->trigger_event('core.delete_attachments_collect_data_before', compact($vars))); + + // Collect post and topic ids for later use if we need to touch remaining entries (if resync is enabled) + $sql = 'SELECT post_msg_id, topic_id, in_message, physical_filename, thumbnail, filesize, is_orphan + FROM ' . ATTACHMENTS_TABLE . ' + WHERE ' . $this->db->sql_in_set($sql_id, $ids); + + $sql .= $sql_where; + + $result = $this->db->sql_query($sql); + + while ($row = $this->db->sql_fetchrow($result)) + { + // We only need to store post/message/topic ids if resync is enabled and the file is not orphaned + if ($resync && !$row['is_orphan']) + { + if (!$row['in_message']) + { + $post_ids[] = $row['post_msg_id']; + $topic_ids[] = $row['topic_id']; + } + else + { + $message_ids[] = $row['post_msg_id']; + } + } + + $physical[] = array('filename' => $row['physical_filename'], 'thumbnail' => $row['thumbnail'], 'filesize' => $row['filesize'], 'is_orphan' => $row['is_orphan']); + } + $this->db->sql_freeresult($result); + + /** + * Perform additional actions before attachment(s) deletion + * + * @event core.delete_attachments_before + * @var string mode Variable containing attachments deletion mode, can be: post|message|topic|attach|user + * @var mixed ids Array or comma separated list of ids corresponding to the mode + * @var bool resync Flag indicating if posts/messages/topics should be synchronized + * @var string sql_id The field name to collect/delete data for depending on the mode + * @var array post_ids Array with post ids for deleted attachment(s) + * @var array topic_ids Array with topic ids for deleted attachment(s) + * @var array message_ids Array with private message ids for deleted attachment(s) + * @var array physical Array with deleted attachment(s) physical file(s) data + * @since 3.1.7-RC1 + */ + $vars = array( + 'mode', + 'ids', + 'resync', + 'sql_id', + 'post_ids', + 'topic_ids', + 'message_ids', + 'physical', + ); + extract($this->dispatcher->trigger_event('core.delete_attachments_before', compact($vars))); + + // Delete attachments + $sql = 'DELETE FROM ' . ATTACHMENTS_TABLE . ' + WHERE ' . $this->db->sql_in_set($sql_id, $ids); + + $sql .= $sql_where; + + $this->db->sql_query($sql); + $num_deleted = $this->db->sql_affectedrows(); + + /** + * Perform additional actions after attachment(s) deletion from the database + * + * @event core.delete_attachments_from_database_after + * @var string mode Variable containing attachments deletion mode, can be: post|message|topic|attach|user + * @var mixed ids Array or comma separated list of ids corresponding to the mode + * @var bool resync Flag indicating if posts/messages/topics should be synchronized + * @var string sql_id The field name to collect/delete data for depending on the mode + * @var array post_ids Array with post ids for deleted attachment(s) + * @var array topic_ids Array with topic ids for deleted attachment(s) + * @var array message_ids Array with private message ids for deleted attachment(s) + * @var array physical Array with deleted attachment(s) physical file(s) data + * @var int num_deleted The number of deleted attachment(s) from the database + * @since 3.1.7-RC1 + */ + $vars = array( + 'mode', + 'ids', + 'resync', + 'sql_id', + 'post_ids', + 'topic_ids', + 'message_ids', + 'physical', + 'num_deleted', + ); + extract($this->dispatcher->trigger_event('core.delete_attachments_from_database_after', compact($vars))); + + if (!$num_deleted) + { + return 0; + } + + // Delete attachments from filesystem + $space_removed = $files_removed = 0; + foreach ($physical as $file_ary) + { + if (phpbb_unlink($file_ary['filename'], 'file', true) && !$file_ary['is_orphan']) + { + // Only non-orphaned files count to the file size + $space_removed += $file_ary['filesize']; + $files_removed++; + } + + if ($file_ary['thumbnail']) + { + phpbb_unlink($file_ary['filename'], 'thumbnail', true); + } + } + + /** + * Perform additional actions after attachment(s) deletion from the filesystem + * + * @event core.delete_attachments_from_filesystem_after + * @var string mode Variable containing attachments deletion mode, can be: post|message|topic|attach|user + * @var mixed ids Array or comma separated list of ids corresponding to the mode + * @var bool resync Flag indicating if posts/messages/topics should be synchronized + * @var string sql_id The field name to collect/delete data for depending on the mode + * @var array post_ids Array with post ids for deleted attachment(s) + * @var array topic_ids Array with topic ids for deleted attachment(s) + * @var array message_ids Array with private message ids for deleted attachment(s) + * @var array physical Array with deleted attachment(s) physical file(s) data + * @var int num_deleted The number of deleted attachment(s) from the database + * @var int space_removed The size of deleted files(s) from the filesystem + * @var int files_removed The number of deleted file(s) from the filesystem + * @since 3.1.7-RC1 + */ + $vars = array( + 'mode', + 'ids', + 'resync', + 'sql_id', + 'post_ids', + 'topic_ids', + 'message_ids', + 'physical', + 'num_deleted', + 'space_removed', + 'files_removed', + ); + extract($this->dispatcher->trigger_event('core.delete_attachments_from_filesystem_after', compact($vars))); + + if ($space_removed || $files_removed) + { + $this->config->increment('upload_dir_size', $space_removed * (-1), false); + $this->config->increment('num_files', $files_removed * (-1), false); + } + + // If we do not resync, we do not need to adjust any message, post, topic or user entries + if (!$resync) + { + return $num_deleted; + } + + // No more use for the original ids + unset($ids); + + // Now, we need to resync posts, messages, topics. We go through every one of them + $post_ids = array_unique($post_ids); + $message_ids = array_unique($message_ids); + $topic_ids = array_unique($topic_ids); + + // Update post indicators for posts now no longer having attachments + if (sizeof($post_ids)) + { + // Just check which posts are still having an assigned attachment not orphaned by querying the attachments table + $sql = 'SELECT post_msg_id + FROM ' . ATTACHMENTS_TABLE . ' + WHERE ' . $this->db->sql_in_set('post_msg_id', $post_ids) . ' + AND in_message = 0 + AND is_orphan = 0'; + $result = $this->db->sql_query($sql); + + $remaining_ids = array(); + while ($row = $this->db->sql_fetchrow($result)) + { + $remaining_ids[] = $row['post_msg_id']; + } + $this->db->sql_freeresult($result); + + // Now only unset those ids remaining + $post_ids = array_diff($post_ids, $remaining_ids); + + if (sizeof($post_ids)) + { + $sql = 'UPDATE ' . POSTS_TABLE . ' + SET post_attachment = 0 + WHERE ' . $this->db->sql_in_set('post_id', $post_ids); + $this->db->sql_query($sql); + } + } + + // Update message table if messages are affected + if (sizeof($message_ids)) + { + // Just check which messages are still having an assigned attachment not orphaned by querying the attachments table + $sql = 'SELECT post_msg_id + FROM ' . ATTACHMENTS_TABLE . ' + WHERE ' . $this->db->sql_in_set('post_msg_id', $message_ids) . ' + AND in_message = 1 + AND is_orphan = 0'; + $result = $this->db->sql_query($sql); + + $remaining_ids = array(); + while ($row = $this->db->sql_fetchrow($result)) + { + $remaining_ids[] = $row['post_msg_id']; + } + $this->db->sql_freeresult($result); + + // Now only unset those ids remaining + $message_ids = array_diff($message_ids, $remaining_ids); + + if (sizeof($message_ids)) + { + $sql = 'UPDATE ' . PRIVMSGS_TABLE . ' + SET message_attachment = 0 + WHERE ' . $this->db->sql_in_set('msg_id', $message_ids); + $this->db->sql_query($sql); + } + } + + // Now update the topics. This is a bit trickier, because there could be posts still having attachments within the topic + if (sizeof($topic_ids)) + { + // Just check which topics are still having an assigned attachment not orphaned by querying the attachments table (much less entries expected) + $sql = 'SELECT topic_id + FROM ' . ATTACHMENTS_TABLE . ' + WHERE ' . $this->db->sql_in_set('topic_id', $topic_ids) . ' + AND is_orphan = 0'; + $result = $this->db->sql_query($sql); + + $remaining_ids = array(); + while ($row = $this->db->sql_fetchrow($result)) + { + $remaining_ids[] = $row['topic_id']; + } + $this->db->sql_freeresult($result); + + // Now only unset those ids remaining + $topic_ids = array_diff($topic_ids, $remaining_ids); + + if (sizeof($topic_ids)) + { + $sql = 'UPDATE ' . TOPICS_TABLE . ' + SET topic_attachment = 0 + WHERE ' . $this->db->sql_in_set('topic_id', $topic_ids); + $this->db->sql_query($sql); + } + } + + return $num_deleted; + } + + /** + * Set attachment IDs + * + * @param array $ids + * + * @return bool True if attachment IDs were set, false if not + */ + protected function set_attachment_ids($ids) + { + // 0 is as bad as an empty array + if (empty($ids)) + { + return false; + } + + if (is_array($ids)) + { + $ids = array_unique($ids); + $this->ids = array_map('intval', $ids); + } + else + { + $this->ids = array((int) $ids); + } + + return true; + } +} \ No newline at end of file From 2ce73a22e76ed021ec06cb29cd84af8b10f77c48 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 21 Sep 2015 10:06:58 +0200 Subject: [PATCH 08/36] [ticket/14168] Fix tests after adding attachment delete class PHPBB3-14168 --- tests/content_visibility/delete_post_test.php | 2 ++ tests/functions_user/delete_user_test.php | 1 + tests/privmsgs/delete_user_pms_test.php | 1 + 3 files changed, 4 insertions(+) diff --git a/tests/content_visibility/delete_post_test.php b/tests/content_visibility/delete_post_test.php index b59b6493d4..d6534456d0 100644 --- a/tests/content_visibility/delete_post_test.php +++ b/tests/content_visibility/delete_post_test.php @@ -312,12 +312,14 @@ class phpbb_content_visibility_delete_post_test extends phpbb_database_test_case $lang_loader = new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx); $lang = new \phpbb\language\language($lang_loader); $user = new \phpbb\user($lang, '\phpbb\datetime'); + $attachment_delete = new \phpbb\attachment\delete($config, $db); $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); $phpbb_container->set('content.visibility', new \phpbb\content_visibility($auth, $config, $phpbb_dispatcher, $db, $user, $phpbb_root_path, $phpEx, FORUMS_TABLE, POSTS_TABLE, TOPICS_TABLE, USERS_TABLE)); + $phpbb_container->set('attachment.delete', $attachment_delete); delete_post($forum_id, $topic_id, $post_id, $data, $is_soft, $reason); diff --git a/tests/functions_user/delete_user_test.php b/tests/functions_user/delete_user_test.php index 9e2caf9d21..163e5d77e2 100644 --- a/tests/functions_user/delete_user_test.php +++ b/tests/functions_user/delete_user_test.php @@ -36,6 +36,7 @@ class phpbb_functions_user_delete_user_test extends phpbb_database_test_case $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); + $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete($config, $db)); $phpbb_container->set( 'auth.provider.db', new phpbb_mock_auth_provider() diff --git a/tests/privmsgs/delete_user_pms_test.php b/tests/privmsgs/delete_user_pms_test.php index 0f061e9784..f8c40f588a 100644 --- a/tests/privmsgs/delete_user_pms_test.php +++ b/tests/privmsgs/delete_user_pms_test.php @@ -91,6 +91,7 @@ class phpbb_privmsgs_delete_user_pms_test extends phpbb_database_test_case $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); + $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db)); phpbb_delete_user_pms($delete_user); From 4709c8a12884563170e0c1d3aaa65b893c98ed53 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 21 Sep 2015 10:13:26 +0200 Subject: [PATCH 09/36] [ticket/14168] Unset prototype classes after use PHPBB3-14168 --- phpBB/includes/functions_admin.php | 1 + phpBB/includes/functions_posting.php | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/phpBB/includes/functions_admin.php b/phpBB/includes/functions_admin.php index aafac876a5..228de95996 100644 --- a/phpBB/includes/functions_admin.php +++ b/phpBB/includes/functions_admin.php @@ -1125,6 +1125,7 @@ function delete_attachments($mode, $ids, $resync = true) $attachment_delete = $phpbb_container->get('attachment.delete'); $num_deleted = $attachment_delete->delete($mode, $ids, $resync); + unset($attachment_delete); return $num_deleted; } diff --git a/phpBB/includes/functions_posting.php b/phpBB/includes/functions_posting.php index d4923de60c..c39b9470a3 100644 --- a/phpBB/includes/functions_posting.php +++ b/phpBB/includes/functions_posting.php @@ -408,8 +408,10 @@ function upload_attachment($form_name, $forum_id, $local = false, $local_storage /** @var \phpbb\attachment\upload $attachment_upload */ $attachment_upload = $phpbb_container->get('attachment.upload'); + $file = $attachment_upload->upload($form_name, $forum_id, $local, $local_storage, $is_message, $local_filedata); + unset($attachment_upload); - return $attachment_upload->upload($form_name, $forum_id, $local, $local_storage, $is_message, $local_filedata); + return $file; } /** From ebfdd69525cc10446fba6c6e8600b44e1124a64d Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 21 Sep 2015 10:28:09 +0200 Subject: [PATCH 10/36] [ticket/14168] Further split up attachment delete class PHPBB3-14168 --- phpBB/phpbb/attachment/delete.php | 499 +++++++++++++++++------------- 1 file changed, 278 insertions(+), 221 deletions(-) diff --git a/phpBB/phpbb/attachment/delete.php b/phpBB/phpbb/attachment/delete.php index a2fe1a8cdf..7a168c2243 100644 --- a/phpBB/phpbb/attachment/delete.php +++ b/phpBB/phpbb/attachment/delete.php @@ -35,6 +35,27 @@ class delete /** @var array Attachement IDs */ protected $ids; + /** @var string SQL ID string */ + private $sql_id; + + /** @var string SQL where string */ + private $sql_where = ''; + + /** @var int Number of deleted items */ + private $num_deleted; + + /** @var array Post IDs */ + private $post_ids = array(); + + /** @var array Message IDs */ + private $message_ids = array(); + + /** @var array Topic IDs */ + private $topic_ids = array(); + + /** @var array Info of physical file */ + private $physical = array(); + /** * Attachment delete class constructor * @@ -66,31 +87,7 @@ class delete return false; } - $sql_where = ''; - - switch ($mode) - { - case 'post': - case 'message': - $sql_id = 'post_msg_id'; - $sql_where = ' AND in_message = ' . ($mode == 'message' ? 1 : 0); - break; - - case 'topic': - $sql_id = 'topic_id'; - break; - - case 'user': - $sql_id = 'poster_id'; - break; - - case 'attach': - default: - $sql_id = 'attach_id'; - break; - } - - $post_ids = $message_ids = $topic_ids = $physical = array(); + $this->set_sql_constraints($mode); /** * Perform additional actions before collecting data for attachment(s) deletion @@ -111,68 +108,10 @@ class delete extract($this->dispatcher->trigger_event('core.delete_attachments_collect_data_before', compact($vars))); // Collect post and topic ids for later use if we need to touch remaining entries (if resync is enabled) - $sql = 'SELECT post_msg_id, topic_id, in_message, physical_filename, thumbnail, filesize, is_orphan - FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $this->db->sql_in_set($sql_id, $ids); + $this->collect_attachment_info($resync); - $sql .= $sql_where; - - $result = $this->db->sql_query($sql); - - while ($row = $this->db->sql_fetchrow($result)) - { - // We only need to store post/message/topic ids if resync is enabled and the file is not orphaned - if ($resync && !$row['is_orphan']) - { - if (!$row['in_message']) - { - $post_ids[] = $row['post_msg_id']; - $topic_ids[] = $row['topic_id']; - } - else - { - $message_ids[] = $row['post_msg_id']; - } - } - - $physical[] = array('filename' => $row['physical_filename'], 'thumbnail' => $row['thumbnail'], 'filesize' => $row['filesize'], 'is_orphan' => $row['is_orphan']); - } - $this->db->sql_freeresult($result); - - /** - * Perform additional actions before attachment(s) deletion - * - * @event core.delete_attachments_before - * @var string mode Variable containing attachments deletion mode, can be: post|message|topic|attach|user - * @var mixed ids Array or comma separated list of ids corresponding to the mode - * @var bool resync Flag indicating if posts/messages/topics should be synchronized - * @var string sql_id The field name to collect/delete data for depending on the mode - * @var array post_ids Array with post ids for deleted attachment(s) - * @var array topic_ids Array with topic ids for deleted attachment(s) - * @var array message_ids Array with private message ids for deleted attachment(s) - * @var array physical Array with deleted attachment(s) physical file(s) data - * @since 3.1.7-RC1 - */ - $vars = array( - 'mode', - 'ids', - 'resync', - 'sql_id', - 'post_ids', - 'topic_ids', - 'message_ids', - 'physical', - ); - extract($this->dispatcher->trigger_event('core.delete_attachments_before', compact($vars))); - - // Delete attachments - $sql = 'DELETE FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $this->db->sql_in_set($sql_id, $ids); - - $sql .= $sql_where; - - $this->db->sql_query($sql); - $num_deleted = $this->db->sql_affectedrows(); + // Delete attachments from database + $this->delete_attachments_from_db(); /** * Perform additional actions after attachment(s) deletion from the database @@ -202,14 +141,265 @@ class delete ); extract($this->dispatcher->trigger_event('core.delete_attachments_from_database_after', compact($vars))); - if (!$num_deleted) + if (!$this->num_deleted) { return 0; } // Delete attachments from filesystem + $this->delete_attachments_from_filesystem(); + + // If we do not resync, we do not need to adjust any message, post, topic or user entries + if (!$resync) + { + return $this->num_deleted; + } + + // No more use for the original ids + unset($ids); + + // Now, we need to resync posts, messages, topics. We go through every one of them + // Update post indicators for posts now no longer having attachments + if (sizeof($this->post_ids)) + { + // Just check which posts are still having an assigned attachment not orphaned by querying the attachments table + $sql = 'SELECT post_msg_id + FROM ' . ATTACHMENTS_TABLE . ' + WHERE ' . $this->db->sql_in_set('post_msg_id', $this->post_ids) . ' + AND in_message = 0 + AND is_orphan = 0'; + $result = $this->db->sql_query($sql); + + $remaining_ids = array(); + while ($row = $this->db->sql_fetchrow($result)) + { + $remaining_ids[] = $row['post_msg_id']; + } + $this->db->sql_freeresult($result); + + // Now only unset those ids remaining + $this->post_ids = array_diff($this->post_ids, $remaining_ids); + + if (sizeof($this->post_ids)) + { + $sql = 'UPDATE ' . POSTS_TABLE . ' + SET post_attachment = 0 + WHERE ' . $this->db->sql_in_set('post_id', $this->post_ids); + $this->db->sql_query($sql); + } + } + + // Update message table if messages are affected + if (sizeof($this->message_ids)) + { + // Just check which messages are still having an assigned attachment not orphaned by querying the attachments table + $sql = 'SELECT post_msg_id + FROM ' . ATTACHMENTS_TABLE . ' + WHERE ' . $this->db->sql_in_set('post_msg_id', $this->message_ids) . ' + AND in_message = 1 + AND is_orphan = 0'; + $result = $this->db->sql_query($sql); + + $remaining_ids = array(); + while ($row = $this->db->sql_fetchrow($result)) + { + $remaining_ids[] = $row['post_msg_id']; + } + $this->db->sql_freeresult($result); + + // Now only unset those ids remaining + $this->message_ids = array_diff($this->message_ids, $remaining_ids); + + if (sizeof($this->message_ids)) + { + $sql = 'UPDATE ' . PRIVMSGS_TABLE . ' + SET message_attachment = 0 + WHERE ' . $this->db->sql_in_set('msg_id', $this->message_ids); + $this->db->sql_query($sql); + } + } + + // Now update the topics. This is a bit trickier, because there could be posts still having attachments within the topic + if (sizeof($this->topic_ids)) + { + // Just check which topics are still having an assigned attachment not orphaned by querying the attachments table (much less entries expected) + $sql = 'SELECT topic_id + FROM ' . ATTACHMENTS_TABLE . ' + WHERE ' . $this->db->sql_in_set('topic_id', $this->topic_ids) . ' + AND is_orphan = 0'; + $result = $this->db->sql_query($sql); + + $remaining_ids = array(); + while ($row = $this->db->sql_fetchrow($result)) + { + $remaining_ids[] = $row['topic_id']; + } + $this->db->sql_freeresult($result); + + // Now only unset those ids remaining + $this->topic_ids = array_diff($this->topic_ids, $remaining_ids); + + if (sizeof($this->topic_ids)) + { + $sql = 'UPDATE ' . TOPICS_TABLE . ' + SET topic_attachment = 0 + WHERE ' . $this->db->sql_in_set('topic_id', $this->topic_ids); + $this->db->sql_query($sql); + } + } + + return $this->num_deleted; + } + + /** + * Set attachment IDs + * + * @param array $ids + * + * @return bool True if attachment IDs were set, false if not + */ + protected function set_attachment_ids($ids) + { + // 0 is as bad as an empty array + if (empty($ids)) + { + return false; + } + + if (is_array($ids)) + { + $ids = array_unique($ids); + $this->ids = array_map('intval', $ids); + } + else + { + $this->ids = array((int) $ids); + } + + return true; + } + + /** + * Set SQL constraints based on mode + * + * @param string $mode Delete mode; can be: post|message|topic|attach|user + */ + private function set_sql_constraints($mode) + { + switch ($mode) + { + case 'post': + case 'message': + $this->sql_id = 'post_msg_id'; + $this->sql_where = ' AND in_message = ' . ($mode == 'message' ? 1 : 0); + break; + + case 'topic': + $this->sql_id = 'topic_id'; + break; + + case 'user': + $this->sql_id = 'poster_id'; + break; + + case 'attach': + default: + $this->sql_id = 'attach_id'; + break; + } + } + + /** + * Collect info about attachment IDs + * + * @param bool $resync Whether topics/posts should be resynced after delete + */ + protected function collect_attachment_info($resync) + { + // Collect post and topic ids for later use if we need to touch remaining entries (if resync is enabled) + $sql = 'SELECT post_msg_id, topic_id, in_message, physical_filename, thumbnail, filesize, is_orphan + FROM ' . ATTACHMENTS_TABLE . ' + WHERE ' . $this->db->sql_in_set($this->sql_id, $this->ids); + + $sql .= $this->sql_where; + + $result = $this->db->sql_query($sql); + + while ($row = $this->db->sql_fetchrow($result)) + { + // We only need to store post/message/topic ids if resync is enabled and the file is not orphaned + if ($resync && !$row['is_orphan']) + { + if (!$row['in_message']) + { + $this->post_ids[] = $row['post_msg_id']; + $this->topic_ids[] = $row['topic_id']; + } + else + { + $this->message_ids[] = $row['post_msg_id']; + } + } + + $this->physical[] = array('filename' => $row['physical_filename'], 'thumbnail' => $row['thumbnail'], 'filesize' => $row['filesize'], 'is_orphan' => $row['is_orphan']); + } + $this->db->sql_freeresult($result); + + // IDs should be unique + $this->post_ids = array_unique($this->post_ids); + $this->message_ids = array_unique($this->message_ids); + $this->topic_ids = array_unique($this->topic_ids); + } + + /** + * Delete attachments from database table + */ + protected function delete_attachments_from_db() + { + /** + * Perform additional actions before attachment(s) deletion + * + * @event core.delete_attachments_before + * @var string mode Variable containing attachments deletion mode, can be: post|message|topic|attach|user + * @var mixed ids Array or comma separated list of ids corresponding to the mode + * @var bool resync Flag indicating if posts/messages/topics should be synchronized + * @var string sql_id The field name to collect/delete data for depending on the mode + * @var array post_ids Array with post ids for deleted attachment(s) + * @var array topic_ids Array with topic ids for deleted attachment(s) + * @var array message_ids Array with private message ids for deleted attachment(s) + * @var array physical Array with deleted attachment(s) physical file(s) data + * @since 3.1.7-RC1 + */ + $vars = array( + 'mode', + 'ids', + 'resync', + 'sql_id', + 'post_ids', + 'topic_ids', + 'message_ids', + 'physical', + ); + extract($this->dispatcher->trigger_event('core.delete_attachments_before', compact($vars))); + + // Delete attachments + $sql = 'DELETE FROM ' . ATTACHMENTS_TABLE . ' + WHERE ' . $this->db->sql_in_set($this->sql_id, $this->ids); + + $sql .= $this->sql_where; + + $this->db->sql_query($sql); + $this->num_deleted = $this->db->sql_affectedrows(); + } + + /** + * Delete attachments from filesystem + */ + protected function delete_attachments_from_filesystem() + { $space_removed = $files_removed = 0; - foreach ($physical as $file_ary) + + foreach ($this->physical as $file_ary) { if (phpbb_unlink($file_ary['filename'], 'file', true) && !$file_ary['is_orphan']) { @@ -261,138 +451,5 @@ class delete $this->config->increment('upload_dir_size', $space_removed * (-1), false); $this->config->increment('num_files', $files_removed * (-1), false); } - - // If we do not resync, we do not need to adjust any message, post, topic or user entries - if (!$resync) - { - return $num_deleted; - } - - // No more use for the original ids - unset($ids); - - // Now, we need to resync posts, messages, topics. We go through every one of them - $post_ids = array_unique($post_ids); - $message_ids = array_unique($message_ids); - $topic_ids = array_unique($topic_ids); - - // Update post indicators for posts now no longer having attachments - if (sizeof($post_ids)) - { - // Just check which posts are still having an assigned attachment not orphaned by querying the attachments table - $sql = 'SELECT post_msg_id - FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $this->db->sql_in_set('post_msg_id', $post_ids) . ' - AND in_message = 0 - AND is_orphan = 0'; - $result = $this->db->sql_query($sql); - - $remaining_ids = array(); - while ($row = $this->db->sql_fetchrow($result)) - { - $remaining_ids[] = $row['post_msg_id']; - } - $this->db->sql_freeresult($result); - - // Now only unset those ids remaining - $post_ids = array_diff($post_ids, $remaining_ids); - - if (sizeof($post_ids)) - { - $sql = 'UPDATE ' . POSTS_TABLE . ' - SET post_attachment = 0 - WHERE ' . $this->db->sql_in_set('post_id', $post_ids); - $this->db->sql_query($sql); - } - } - - // Update message table if messages are affected - if (sizeof($message_ids)) - { - // Just check which messages are still having an assigned attachment not orphaned by querying the attachments table - $sql = 'SELECT post_msg_id - FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $this->db->sql_in_set('post_msg_id', $message_ids) . ' - AND in_message = 1 - AND is_orphan = 0'; - $result = $this->db->sql_query($sql); - - $remaining_ids = array(); - while ($row = $this->db->sql_fetchrow($result)) - { - $remaining_ids[] = $row['post_msg_id']; - } - $this->db->sql_freeresult($result); - - // Now only unset those ids remaining - $message_ids = array_diff($message_ids, $remaining_ids); - - if (sizeof($message_ids)) - { - $sql = 'UPDATE ' . PRIVMSGS_TABLE . ' - SET message_attachment = 0 - WHERE ' . $this->db->sql_in_set('msg_id', $message_ids); - $this->db->sql_query($sql); - } - } - - // Now update the topics. This is a bit trickier, because there could be posts still having attachments within the topic - if (sizeof($topic_ids)) - { - // Just check which topics are still having an assigned attachment not orphaned by querying the attachments table (much less entries expected) - $sql = 'SELECT topic_id - FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $this->db->sql_in_set('topic_id', $topic_ids) . ' - AND is_orphan = 0'; - $result = $this->db->sql_query($sql); - - $remaining_ids = array(); - while ($row = $this->db->sql_fetchrow($result)) - { - $remaining_ids[] = $row['topic_id']; - } - $this->db->sql_freeresult($result); - - // Now only unset those ids remaining - $topic_ids = array_diff($topic_ids, $remaining_ids); - - if (sizeof($topic_ids)) - { - $sql = 'UPDATE ' . TOPICS_TABLE . ' - SET topic_attachment = 0 - WHERE ' . $this->db->sql_in_set('topic_id', $topic_ids); - $this->db->sql_query($sql); - } - } - - return $num_deleted; - } - - /** - * Set attachment IDs - * - * @param array $ids - * - * @return bool True if attachment IDs were set, false if not - */ - protected function set_attachment_ids($ids) - { - // 0 is as bad as an empty array - if (empty($ids)) - { - return false; - } - - if (is_array($ids)) - { - $ids = array_unique($ids); - $this->ids = array_map('intval', $ids); - } - else - { - $this->ids = array((int) $ids); - } - - return true; } } \ No newline at end of file From 40117c77304468120245ae05b6b99f1d2a68c9f6 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 21 Sep 2015 10:56:32 +0200 Subject: [PATCH 11/36] [ticket/14168] Add attachment resync class PHPBB3-14168 --- .../default/container/services_files.yml | 23 ++-- phpBB/phpbb/attachment/delete.php | 98 ++------------ phpBB/phpbb/attachment/resync.php | 123 ++++++++++++++++++ tests/content_visibility/delete_post_test.php | 2 +- tests/functions_user/delete_user_test.php | 2 +- tests/privmsgs/delete_user_pms_test.php | 2 +- 6 files changed, 152 insertions(+), 98 deletions(-) create mode 100644 phpBB/phpbb/attachment/resync.php diff --git a/phpBB/config/default/container/services_files.yml b/phpBB/config/default/container/services_files.yml index 12ae3f79c6..d46e3d0401 100644 --- a/phpBB/config/default/container/services_files.yml +++ b/phpBB/config/default/container/services_files.yml @@ -1,4 +1,19 @@ services: + attachment.delete: + class: phpbb\attachment\delete + scope: prototype + arguments: + - @config + - @dbal.conn + - @dispatcher + - @attachment.resync + + attachment.resync: + class: phpbb\attachment\resync + scope: prototype + arguments: + - @dbal.conn + attachment.upload: class: phpbb\attachment\upload scope: prototype @@ -14,14 +29,6 @@ services: - @user - %core.root_path% - attachment.delete: - class: phpbb\attachment\delete - scope: prototype - arguments: - - @config - - @dbal.conn - - @dispatcher - filesystem: class: phpbb\filesystem\filesystem diff --git a/phpBB/phpbb/attachment/delete.php b/phpBB/phpbb/attachment/delete.php index 7a168c2243..4ef320eab4 100644 --- a/phpBB/phpbb/attachment/delete.php +++ b/phpBB/phpbb/attachment/delete.php @@ -32,6 +32,9 @@ class delete /** @var \phpbb\event\dispatcher */ protected $dispatcher; + /** @var \phpbb\attachment\resync */ + protected $resync; + /** @var array Attachement IDs */ protected $ids; @@ -61,13 +64,15 @@ class delete * * @param config $config * @param driver_interface $db - * @param dispatcher>>>>>>> 85b6020... [ticket/14168] Move function for attachment deletion into class + * @param dispatcher $dispatcher + * @param resync $resync */ - public function __construct(config $config, driver_interface $db, dispatcher $dispatcher) + public function __construct(config $config, driver_interface $db, dispatcher $dispatcher, resync $resync) { $this->config = $config; $this->db = $db; $this->dispatcher = $dispatcher; + $this->resync = $resync; } /** @@ -158,95 +163,14 @@ class delete // No more use for the original ids unset($ids); - // Now, we need to resync posts, messages, topics. We go through every one of them // Update post indicators for posts now no longer having attachments - if (sizeof($this->post_ids)) - { - // Just check which posts are still having an assigned attachment not orphaned by querying the attachments table - $sql = 'SELECT post_msg_id - FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $this->db->sql_in_set('post_msg_id', $this->post_ids) . ' - AND in_message = 0 - AND is_orphan = 0'; - $result = $this->db->sql_query($sql); - - $remaining_ids = array(); - while ($row = $this->db->sql_fetchrow($result)) - { - $remaining_ids[] = $row['post_msg_id']; - } - $this->db->sql_freeresult($result); - - // Now only unset those ids remaining - $this->post_ids = array_diff($this->post_ids, $remaining_ids); - - if (sizeof($this->post_ids)) - { - $sql = 'UPDATE ' . POSTS_TABLE . ' - SET post_attachment = 0 - WHERE ' . $this->db->sql_in_set('post_id', $this->post_ids); - $this->db->sql_query($sql); - } - } + $this->resync->resync('post', $this->post_ids); // Update message table if messages are affected - if (sizeof($this->message_ids)) - { - // Just check which messages are still having an assigned attachment not orphaned by querying the attachments table - $sql = 'SELECT post_msg_id - FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $this->db->sql_in_set('post_msg_id', $this->message_ids) . ' - AND in_message = 1 - AND is_orphan = 0'; - $result = $this->db->sql_query($sql); - - $remaining_ids = array(); - while ($row = $this->db->sql_fetchrow($result)) - { - $remaining_ids[] = $row['post_msg_id']; - } - $this->db->sql_freeresult($result); - - // Now only unset those ids remaining - $this->message_ids = array_diff($this->message_ids, $remaining_ids); - - if (sizeof($this->message_ids)) - { - $sql = 'UPDATE ' . PRIVMSGS_TABLE . ' - SET message_attachment = 0 - WHERE ' . $this->db->sql_in_set('msg_id', $this->message_ids); - $this->db->sql_query($sql); - } - } + $this->resync->resync('message', $this->message_ids); // Now update the topics. This is a bit trickier, because there could be posts still having attachments within the topic - if (sizeof($this->topic_ids)) - { - // Just check which topics are still having an assigned attachment not orphaned by querying the attachments table (much less entries expected) - $sql = 'SELECT topic_id - FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $this->db->sql_in_set('topic_id', $this->topic_ids) . ' - AND is_orphan = 0'; - $result = $this->db->sql_query($sql); - - $remaining_ids = array(); - while ($row = $this->db->sql_fetchrow($result)) - { - $remaining_ids[] = $row['topic_id']; - } - $this->db->sql_freeresult($result); - - // Now only unset those ids remaining - $this->topic_ids = array_diff($this->topic_ids, $remaining_ids); - - if (sizeof($this->topic_ids)) - { - $sql = 'UPDATE ' . TOPICS_TABLE . ' - SET topic_attachment = 0 - WHERE ' . $this->db->sql_in_set('topic_id', $this->topic_ids); - $this->db->sql_query($sql); - } - } + $this->resync->resync('topic', $this->topic_ids); return $this->num_deleted; } @@ -384,7 +308,7 @@ class delete // Delete attachments $sql = 'DELETE FROM ' . ATTACHMENTS_TABLE . ' - WHERE ' . $this->db->sql_in_set($this->sql_id, $this->ids); + WHERE ' . $this->db->sql_in_set($this->sql_id, $this->ids); $sql .= $this->sql_where; diff --git a/phpBB/phpbb/attachment/resync.php b/phpBB/phpbb/attachment/resync.php new file mode 100644 index 0000000000..c247fcce7a --- /dev/null +++ b/phpBB/phpbb/attachment/resync.php @@ -0,0 +1,123 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +namespace phpbb\attachment; + +use \phpbb\db\driver\driver_interface; + +/** + * Attachment delete class + */ + +class resync +{ + /** @var driver_interface */ + protected $db; + + /** @var string Attachment table SQL ID */ + private $attach_sql_id; + + /** @var string Resync table SQL ID */ + private $resync_sql_id; + + /** @var string Resync SQL table */ + private $resync_table; + + /** @var string SQL where statement */ + private $sql_where; + + /** + * Constructor for attachment resync class + * + * @param driver_interface $db Database driver + */ + public function __construct(driver_interface $db) + { + $this->db = $db; + } + + /** + * Set type constraints for attachment resync + * + * @param string $type Type of resync; can be: message|post|topic + */ + protected function set_type_constraints($type) + { + switch ($type) + { + case 'message': + $this->attach_sql_id = 'post_msg_id'; + $this->sql_where = ' AND in_message = 1 + AND is_orphan = 0'; + $this->resync_table = PRIVMSGS_TABLE; + $this->resync_sql_id = 'msg_id'; + break; + + case 'post': + $this->attach_sql_id = 'post_msg_id'; + $this->sql_where = ' AND in_message = 0 + AND is_orphan = 0'; + $this->resync_table = POSTS_TABLE; + $this->resync_sql_id = 'post_id'; + break; + + case 'topic': + $this->attach_sql_id = 'topic_id'; + $this->sql_where = ' AND is_orphan = 0'; + $this->resync_table = TOPICS_TABLE; + $this->resync_sql_id = 'topic_id'; + break; + } + } + + /** + * Resync specified type + * + * @param string $type Type of resync + * @param array $ids IDs to resync + */ + public function resync($type, $ids) + { + if (empty($type) || !is_array($ids) || !sizeof($ids) || !in_array($type, array('post', 'topic', 'message'))) + { + return; + } + + // Just check which elements are still having an assigned attachment + // not orphaned by querying the attachments table + $sql = 'SELECT ' . $this->attach_sql_id . ' + FROM ' . ATTACHMENTS_TABLE . ' + WHERE ' . $this->db->sql_in_set($this->attach_sql_id, $ids) + . $this->sql_where; + $result = $this->db->sql_query($sql); + + $remaining_ids = array(); + while ($row = $this->db->sql_fetchrow($result)) + { + $remaining_ids[] = $row[$this->attach_sql_id]; + } + $this->db->sql_freeresult($result); + + // Now only unset those ids remaining + $ids = array_diff($ids, $remaining_ids); + + if (sizeof($ids)) + { + $sql = 'UPDATE ' . POSTS_TABLE . ' + SET ' . $type . '_attachment = 0 + WHERE ' . $this->db->sql_in_set($this->resync_sql_id, $ids); + $this->db->sql_query($sql); + } + } + +} \ No newline at end of file diff --git a/tests/content_visibility/delete_post_test.php b/tests/content_visibility/delete_post_test.php index d6534456d0..bb609b6ab7 100644 --- a/tests/content_visibility/delete_post_test.php +++ b/tests/content_visibility/delete_post_test.php @@ -312,7 +312,7 @@ class phpbb_content_visibility_delete_post_test extends phpbb_database_test_case $lang_loader = new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx); $lang = new \phpbb\language\language($lang_loader); $user = new \phpbb\user($lang, '\phpbb\datetime'); - $attachment_delete = new \phpbb\attachment\delete($config, $db); + $attachment_delete = new \phpbb\attachment\delete($config, $db, new \phpbb\attachment\resync($db)); $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); diff --git a/tests/functions_user/delete_user_test.php b/tests/functions_user/delete_user_test.php index 163e5d77e2..b877164629 100644 --- a/tests/functions_user/delete_user_test.php +++ b/tests/functions_user/delete_user_test.php @@ -36,7 +36,7 @@ class phpbb_functions_user_delete_user_test extends phpbb_database_test_case $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); - $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete($config, $db)); + $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete($config, $db, new \phpbb\attachment\resync($db))); $phpbb_container->set( 'auth.provider.db', new phpbb_mock_auth_provider() diff --git a/tests/privmsgs/delete_user_pms_test.php b/tests/privmsgs/delete_user_pms_test.php index f8c40f588a..7345721d8b 100644 --- a/tests/privmsgs/delete_user_pms_test.php +++ b/tests/privmsgs/delete_user_pms_test.php @@ -91,7 +91,7 @@ class phpbb_privmsgs_delete_user_pms_test extends phpbb_database_test_case $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); - $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db)); + $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db, new \phpbb\attachment\resync($db))); phpbb_delete_user_pms($delete_user); From e158db5daa8b9eedfea9d17b5d678320ac7f8f61 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 21 Sep 2015 11:01:01 +0200 Subject: [PATCH 12/36] [ticket/14168] Minor coding style fixes PHPBB3-14168 --- phpBB/includes/message_parser.php | 2 +- phpBB/phpbb/attachment/delete.php | 2 +- phpBB/phpbb/attachment/resync.php | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/phpBB/includes/message_parser.php b/phpBB/includes/message_parser.php index e0a0d69fba..a56ef0d4a9 100644 --- a/phpBB/includes/message_parser.php +++ b/phpBB/includes/message_parser.php @@ -1691,7 +1691,7 @@ class parse_message extends bbcode_firstpass { /** @var \phpbb\attachment\upload $attachment_upload */ $attachment_upload = $phpbb_container->get('attachment.upload'); - $filedata = $attachment_upload->upload($form_name, $forum_id, false, '', $is_message);; + $filedata = $attachment_upload->upload($form_name, $forum_id, false, '', $is_message); $error = array_merge($error, $filedata['error']); if (!sizeof($error)) diff --git a/phpBB/phpbb/attachment/delete.php b/phpBB/phpbb/attachment/delete.php index 4ef320eab4..32469c124c 100644 --- a/phpBB/phpbb/attachment/delete.php +++ b/phpBB/phpbb/attachment/delete.php @@ -376,4 +376,4 @@ class delete $this->config->increment('num_files', $files_removed * (-1), false); } } -} \ No newline at end of file +} diff --git a/phpBB/phpbb/attachment/resync.php b/phpBB/phpbb/attachment/resync.php index c247fcce7a..9abf961c6b 100644 --- a/phpBB/phpbb/attachment/resync.php +++ b/phpBB/phpbb/attachment/resync.php @@ -120,4 +120,4 @@ class resync } } -} \ No newline at end of file +} From cebc064f4c15b9c53210cc3729756b4d7888041b Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 21 Sep 2015 14:51:39 +0200 Subject: [PATCH 13/36] [ticket/14168] Coding guidelines fixes PHPBB3-14168 --- phpBB/phpbb/attachment/delete.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/phpBB/phpbb/attachment/delete.php b/phpBB/phpbb/attachment/delete.php index 32469c124c..4c863a2205 100644 --- a/phpBB/phpbb/attachment/delete.php +++ b/phpBB/phpbb/attachment/delete.php @@ -216,20 +216,20 @@ class delete case 'message': $this->sql_id = 'post_msg_id'; $this->sql_where = ' AND in_message = ' . ($mode == 'message' ? 1 : 0); - break; + break; case 'topic': $this->sql_id = 'topic_id'; - break; + break; case 'user': $this->sql_id = 'poster_id'; - break; + break; case 'attach': default: $this->sql_id = 'attach_id'; - break; + break; } } From 583f42a2aa5ba8132e9e516b363ba0180fe46289 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 21 Sep 2015 16:28:53 +0200 Subject: [PATCH 14/36] [ticket/14168] Add tests for attachment resync class PHPBB3-14168 --- phpBB/phpbb/attachment/resync.php | 4 +- tests/attachment/fixtures/resync.xml | 81 ++++++++++++++++++++++++++++ tests/attachment/resync_test.php | 74 +++++++++++++++++++++++++ 3 files changed, 158 insertions(+), 1 deletion(-) create mode 100644 tests/attachment/fixtures/resync.xml create mode 100644 tests/attachment/resync_test.php diff --git a/phpBB/phpbb/attachment/resync.php b/phpBB/phpbb/attachment/resync.php index 9abf961c6b..adb642fe97 100644 --- a/phpBB/phpbb/attachment/resync.php +++ b/phpBB/phpbb/attachment/resync.php @@ -93,6 +93,8 @@ class resync return; } + $this->set_type_constraints($type); + // Just check which elements are still having an assigned attachment // not orphaned by querying the attachments table $sql = 'SELECT ' . $this->attach_sql_id . ' @@ -113,7 +115,7 @@ class resync if (sizeof($ids)) { - $sql = 'UPDATE ' . POSTS_TABLE . ' + $sql = 'UPDATE ' . $this->resync_table . ' SET ' . $type . '_attachment = 0 WHERE ' . $this->db->sql_in_set($this->resync_sql_id, $ids); $this->db->sql_query($sql); diff --git a/tests/attachment/fixtures/resync.xml b/tests/attachment/fixtures/resync.xml new file mode 100644 index 0000000000..132f62382c --- /dev/null +++ b/tests/attachment/fixtures/resync.xml @@ -0,0 +1,81 @@ + + + + post_msg_id + topic_id + in_message + is_orphan + attach_comment + + 1 + 1 + 0 + 0 + foo + + + 1 + 1 + 1 + 0 + foo2 + +
+ + post_id + post_text + poster_id + post_attachment + + 1 + foo + 1 + 1 + + + 2 + foo + 1 + 1 + +
+ + msg_id + message_text + message_attachment + to_address + bcc_address + + 1 + foo + 1 + 2 + 2 + + + 2 + foo + 1 + 2 + 2 + +
+ + topic_id + forum_id + topic_title + topic_attachment + + 1 + 1 + foo + 1 + + + 2 + 1 + bar + 1 + +
+ diff --git a/tests/attachment/resync_test.php b/tests/attachment/resync_test.php new file mode 100644 index 0000000000..de8a42637e --- /dev/null +++ b/tests/attachment/resync_test.php @@ -0,0 +1,74 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +class phpbb_attachment_resync_test extends \phpbb_database_test_case +{ + /** @var \phpbb\db\driver\driver_interface */ + protected $db; + + /** @var \phpbb\attachment\resync */ + protected $resync; + + public function getDataSet() + { + return $this->createXMLDataSet(dirname(__FILE__) . '/fixtures/resync.xml'); + } + + public function setUp() + { + parent::setUp(); + + $this->db = $this->new_dbal(); + $this->resync = new \phpbb\attachment\resync($this->db); + } + + public function data_resync() + { + return array( + array('', array(1), 'post_id', POSTS_TABLE, array('post_attachment' => '1'), array('post_attachment' => '1')), + array('post', array(1), 'post_id', POSTS_TABLE, array('post_attachment' => '1'), array('post_attachment' => '1')), + array('post', array(2), 'post_id', POSTS_TABLE, array('post_attachment' => '1'), array('post_attachment' => '0')), + array('topic', array(1), 'topic_id', TOPICS_TABLE, array('topic_attachment' => '1'), array('topic_attachment' => '1')), + array('topic', array(2), 'topic_id', TOPICS_TABLE, array('topic_attachment' => '1'), array('topic_attachment' => '0')), + array('message', array(1), 'msg_id', PRIVMSGS_TABLE, array('message_attachment' => '1'), array('message_attachment' => '1')), + array('message', array(2), 'msg_id', PRIVMSGS_TABLE, array('message_attachment' => '1'), array('message_attachment' => '0')), + ); + } + + /** + * @dataProvider data_resync + */ + public function test_resync($type, $ids, $sql_id, $exist_table, $exist_data, $resync_data) + { + $sql_prefix = ($type) ?: 'post'; + $sql = 'SELECT ' . $sql_prefix . '_attachment + FROM ' . $exist_table . ' + WHERE ' . $sql_id . ' = ' . $ids[0]; + $result = $this->db->sql_query($sql); + $data = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + $this->assertSame($exist_data, $data); + + $this->resync->resync($type, $ids); + + $sql = 'SELECT ' . $sql_prefix . '_attachment + FROM ' . $exist_table . ' + WHERE ' . $sql_id . ' = ' . $ids[0]; + $result = $this->db->sql_query($sql); + $data = $this->db->sql_fetchrow($result); + $this->db->sql_freeresult($result); + + $this->assertSame($resync_data, $data); + } +} From b00c7511f022dfafb32f570db1f5fe762a62ef21 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 21 Sep 2015 22:16:23 +0200 Subject: [PATCH 15/36] [ticket/14168] Add tests for attachment delete class PHPBB3-14168 --- phpBB/phpbb/attachment/delete.php | 2 +- tests/attachment/delete_test.php | 70 +++++++++++++++++++++++++++++++ 2 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 tests/attachment/delete_test.php diff --git a/phpBB/phpbb/attachment/delete.php b/phpBB/phpbb/attachment/delete.php index 4c863a2205..372495aef0 100644 --- a/phpBB/phpbb/attachment/delete.php +++ b/phpBB/phpbb/attachment/delete.php @@ -85,7 +85,7 @@ class delete * @return int|bool Number of deleted attachments or false if something * went wrong during attachment deletion */ - function delete($mode, $ids, $resync = true) + public function delete($mode, $ids, $resync = true) { if (!$this->set_attachment_ids($ids)) { diff --git a/tests/attachment/delete_test.php b/tests/attachment/delete_test.php new file mode 100644 index 0000000000..19a5e0bcca --- /dev/null +++ b/tests/attachment/delete_test.php @@ -0,0 +1,70 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +require_once(dirname(__FILE__) . '/../../phpBB/includes/functions_admin.php'); + +class phpbb_attachment_delete_test extends \phpbb_database_test_case +{ + /** @var \phpbb\config\config */ + protected $config; + + /** @var \phpbb\db\driver\driver_interface */ + protected $db; + + /** @var \phpbb\attachment\resync */ + protected $resync; + + /** @var \phpbb\attachment\delete */ + protected $attachment_delete; + + public function getDataSet() + { + return $this->createXMLDataSet(dirname(__FILE__) . '/fixtures/resync.xml'); + } + + public function setUp() + { + global $db; + + parent::setUp(); + + $this->config = new \phpbb\config\config(array()); + $this->db = $this->new_dbal(); + $db = $this->db; + $this->resync = new \phpbb\attachment\resync($this->db); + $this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->resync); + } + + public function data_attachment_delete() + { + return array( + array('attach', '', false, false), + array('meh', 5, false, 0), + array('attach', array(5), false, 0), + array('attach', array(1,2), false, 2), + array('attach', array(1,2), true, 2), + array('post', 5, false, 0), + array('topic', 5, false, 0), + array('topic', 1, true, 2), + array('user', 1, false, 0), + ); + } + + /** + * @dataProvider data_attachment_delete + */ + public function test_attachment_delete($mode, $ids, $resync, $expected) + { + $this->assertSame($expected, $this->attachment_delete->delete($mode, $ids, $resync)); + } +} From c6f9e3a966cffdc09c64f73a25ac98d6bbe559cd Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 21 Sep 2015 22:17:28 +0200 Subject: [PATCH 16/36] [ticket/14168] Fix resync tests on sqlite3 PHPBB3-14168 --- tests/attachment/resync_test.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/attachment/resync_test.php b/tests/attachment/resync_test.php index de8a42637e..f882af9ae5 100644 --- a/tests/attachment/resync_test.php +++ b/tests/attachment/resync_test.php @@ -58,7 +58,7 @@ class phpbb_attachment_resync_test extends \phpbb_database_test_case $data = $this->db->sql_fetchrow($result); $this->db->sql_freeresult($result); - $this->assertSame($exist_data, $data); + $this->assertEquals($exist_data, $data); $this->resync->resync($type, $ids); @@ -69,6 +69,6 @@ class phpbb_attachment_resync_test extends \phpbb_database_test_case $data = $this->db->sql_fetchrow($result); $this->db->sql_freeresult($result); - $this->assertSame($resync_data, $data); + $this->assertEquals($resync_data, $data); } } From 8d03b9e001526fb237cdb744dd832c1dd4182d01 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 23 Sep 2015 09:09:09 +0200 Subject: [PATCH 17/36] [ticket/14168] Reset sequence before tests in delete tests PHPBB3-14168 --- tests/attachment/delete_test.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/attachment/delete_test.php b/tests/attachment/delete_test.php index 19a5e0bcca..df7e305dc5 100644 --- a/tests/attachment/delete_test.php +++ b/tests/attachment/delete_test.php @@ -65,6 +65,13 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case */ public function test_attachment_delete($mode, $ids, $resync, $expected) { + // We need to reset the attachment ID sequence to properly test this + if ($this->db->get_sql_layer() === 'postgres') + { + $sql = 'ALTER SEQUENCE phpbb_attachments_seq RESTART WITH 1'; + $this->db->sql_query($sql); + } + $this->assertSame($expected, $this->attachment_delete->delete($mode, $ids, $resync)); } } From 04786313892df25f5adce555ebae6b2e5ad4d222 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 23 Sep 2015 10:17:38 +0200 Subject: [PATCH 18/36] [ticket/14168] Move phpbb_unlink() into attachment delete class PHPBB3-14168 --- .../default/container/services_files.yml | 2 + phpBB/includes/functions_admin.php | 24 +++---- phpBB/phpbb/attachment/delete.php | 70 ++++++++++++++++--- tests/attachment/delete_test.php | 55 ++++++++++++++- tests/attachment/fixtures/resync.xml | 15 ++++ tests/content_visibility/delete_post_test.php | 2 +- tests/functions_user/delete_user_test.php | 4 +- tests/privmsgs/delete_user_pms_test.php | 4 +- 8 files changed, 144 insertions(+), 32 deletions(-) diff --git a/phpBB/config/default/container/services_files.yml b/phpBB/config/default/container/services_files.yml index d46e3d0401..b94b48606d 100644 --- a/phpBB/config/default/container/services_files.yml +++ b/phpBB/config/default/container/services_files.yml @@ -6,7 +6,9 @@ services: - @config - @dbal.conn - @dispatcher + - @filesystem - @attachment.resync + - %core.root_path% attachment.resync: class: phpbb\attachment\resync diff --git a/phpBB/includes/functions_admin.php b/phpBB/includes/functions_admin.php index 228de95996..8928aeb2d6 100644 --- a/phpBB/includes/functions_admin.php +++ b/phpBB/includes/functions_admin.php @@ -1243,27 +1243,19 @@ function update_posted_info(&$topic_ids) /** * Delete attached file +* +* @deprecated 3.2.0-a1 (To be removed: 3.4.0) */ function phpbb_unlink($filename, $mode = 'file', $entry_removed = false) { - global $db, $phpbb_root_path, $config; + global $phpbb_container;; - // Because of copying topics or modifications a physical filename could be assigned more than once. If so, do not remove the file itself. - $sql = 'SELECT COUNT(attach_id) AS num_entries - FROM ' . ATTACHMENTS_TABLE . " - WHERE physical_filename = '" . $db->sql_escape(utf8_basename($filename)) . "'"; - $result = $db->sql_query($sql); - $num_entries = (int) $db->sql_fetchfield('num_entries'); - $db->sql_freeresult($result); + /** @var \phpbb\attachment\delete $attachment_delete */ + $attachment_delete = $phpbb_container->get('attachment.delete'); + $unlink = $attachment_delete->unlink_attachment($filename, $mode, $entry_removed); + unset($attachment_delete); - // Do not remove file if at least one additional entry with the same name exist. - if (($entry_removed && $num_entries > 0) || (!$entry_removed && $num_entries > 1)) - { - return false; - } - - $filename = ($mode == 'thumbnail') ? 'thumb_' . utf8_basename($filename) : utf8_basename($filename); - return @unlink($phpbb_root_path . $config['upload_path'] . '/' . $filename); + return $unlink; } /** diff --git a/phpBB/phpbb/attachment/delete.php b/phpBB/phpbb/attachment/delete.php index 372495aef0..ad385861a1 100644 --- a/phpBB/phpbb/attachment/delete.php +++ b/phpBB/phpbb/attachment/delete.php @@ -16,6 +16,7 @@ namespace phpbb\attachment; use \phpbb\config\config; use \phpbb\db\driver\driver_interface; use \phpbb\event\dispatcher; +use \phpbb\filesystem\filesystem; /** * Attachment delete class @@ -23,18 +24,24 @@ use \phpbb\event\dispatcher; class delete { - /** @var \phpbb\config\config */ + /** @var config */ protected $config; - /** @var \phpbb\db\driver\driver_interface */ + /** @var driver_interface */ protected $db; /** @var \phpbb\event\dispatcher */ protected $dispatcher; - /** @var \phpbb\attachment\resync */ + /** @var filesystem */ + protected $filesystem; + + /** @var resync */ protected $resync; + /** @var string phpBB root path */ + protected $phpbb_root_path; + /** @var array Attachement IDs */ protected $ids; @@ -65,14 +72,18 @@ class delete * @param config $config * @param driver_interface $db * @param dispatcher $dispatcher + * @param filesystem $filesystem * @param resync $resync + * @param string $phpbb_root_path */ - public function __construct(config $config, driver_interface $db, dispatcher $dispatcher, resync $resync) + public function __construct(config $config, driver_interface $db, dispatcher $dispatcher, filesystem $filesystem, resync $resync, $phpbb_root_path) { $this->config = $config; $this->db = $db; $this->dispatcher = $dispatcher; + $this->filesystem = $filesystem; $this->resync = $resync; + $this->phpbb_root_path = $phpbb_root_path; } /** @@ -152,7 +163,7 @@ class delete } // Delete attachments from filesystem - $this->delete_attachments_from_filesystem(); + $this->remove_from_filesystem(); // If we do not resync, we do not need to adjust any message, post, topic or user entries if (!$resync) @@ -319,13 +330,13 @@ class delete /** * Delete attachments from filesystem */ - protected function delete_attachments_from_filesystem() + protected function remove_from_filesystem() { $space_removed = $files_removed = 0; foreach ($this->physical as $file_ary) { - if (phpbb_unlink($file_ary['filename'], 'file', true) && !$file_ary['is_orphan']) + if ($this->unlink_attachment($file_ary['filename'], 'file', true) && !$file_ary['is_orphan']) { // Only non-orphaned files count to the file size $space_removed += $file_ary['filesize']; @@ -334,7 +345,7 @@ class delete if ($file_ary['thumbnail']) { - phpbb_unlink($file_ary['filename'], 'thumbnail', true); + $this->unlink_attachment($file_ary['filename'], 'thumbnail', true); } } @@ -376,4 +387,47 @@ class delete $this->config->increment('num_files', $files_removed * (-1), false); } } + + /** + * Delete attachment from filesystem + * + * @param string $filename Filename of attachment + * @param string $mode Delete mode + * @param bool $entry_removed Whether entry was removed. Defaults to false + * @return bool True if file was removed, false if not + */ + public function unlink_attachment($filename, $mode = 'file', $entry_removed = false) + { + // Because of copying topics or modifications a physical filename could be assigned more than once. If so, do not remove the file itself. + $sql = 'SELECT COUNT(attach_id) AS num_entries + FROM ' . ATTACHMENTS_TABLE . " + WHERE physical_filename = '" . $this->db->sql_escape(utf8_basename($filename)) . "'"; + $result = $this->db->sql_query($sql); + $num_entries = (int) $this->db->sql_fetchfield('num_entries'); + $this->db->sql_freeresult($result); + + // Do not remove file if at least one additional entry with the same name exist. + if (($entry_removed && $num_entries > 0) || (!$entry_removed && $num_entries > 1)) + { + return false; + } + + $filename = ($mode == 'thumbnail') ? 'thumb_' . utf8_basename($filename) : utf8_basename($filename); + $filepath = $this->phpbb_root_path . $this->config['upload_path'] . '/' . $filename; + + try + { + if ($this->filesystem->exists($filepath)) + { + $this->filesystem->remove($this->phpbb_root_path . $this->config['upload_path'] . '/' . $filename); + return true; + } + } + catch (\phpbb\filesystem\exception\filesystem_exception $exception) + { + // Fail is covered by return statement below + } + + return false; + } } diff --git a/tests/attachment/delete_test.php b/tests/attachment/delete_test.php index df7e305dc5..8db6487542 100644 --- a/tests/attachment/delete_test.php +++ b/tests/attachment/delete_test.php @@ -21,12 +21,17 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case /** @var \phpbb\db\driver\driver_interface */ protected $db; + /** @var \phpbb\filesystem\filesystem */ + protected $filesystem; + /** @var \phpbb\attachment\resync */ protected $resync; /** @var \phpbb\attachment\delete */ protected $attachment_delete; + protected $phpbb_root_path; + public function getDataSet() { return $this->createXMLDataSet(dirname(__FILE__) . '/fixtures/resync.xml'); @@ -34,7 +39,7 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case public function setUp() { - global $db; + global $db, $phpbb_root_path; parent::setUp(); @@ -42,7 +47,15 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case $this->db = $this->new_dbal(); $db = $this->db; $this->resync = new \phpbb\attachment\resync($this->db); - $this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->resync); + $this->filesystem = $this->getMock('\phpbb\filesystem\filesystem', array('remove', 'exists')); + $this->filesystem->expects($this->any()) + ->method('remove') + ->willReturn(false); + $this->filesystem->expects($this->any()) + ->method('exists') + ->willReturn(true); + $this->phpbb_root_path = $phpbb_root_path; + $this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->filesystem, $this->resync, $phpbb_root_path); } public function data_attachment_delete() @@ -55,7 +68,7 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case array('attach', array(1,2), true, 2), array('post', 5, false, 0), array('topic', 5, false, 0), - array('topic', 1, true, 2), + array('topic', 1, true, 3), array('user', 1, false, 0), ); } @@ -74,4 +87,40 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case $this->assertSame($expected, $this->attachment_delete->delete($mode, $ids, $resync)); } + + public function data_attachment_unlink() + { + return array( + array(true, true, true), + array(true, false, false), + array(true, true, false, true), + ); + } + + /** + * @dataProvider data_attachment_unlink + */ + public function test_attachment_delete_success($remove_success, $exists_success, $expected, $throw_exception = false) + { + $this->filesystem = $this->getMock('\phpbb\filesystem\filesystem', array('remove', 'exists')); + if ($throw_exception) + { + $this->filesystem->expects($this->any()) + ->method('remove') + ->willThrowException(new \phpbb\filesystem\exception\filesystem_exception);; + } + else + { + $this->filesystem->expects($this->any()) + ->method('remove') + ->willReturn($remove_success); + } + + $this->filesystem->expects($this->any()) + ->method('exists') + ->willReturn($exists_success); + + $this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->filesystem, $this->resync, $this->phpbb_root_path); + $this->assertSame($expected, $this->attachment_delete->unlink_attachment('foobar')); + } } diff --git a/tests/attachment/fixtures/resync.xml b/tests/attachment/fixtures/resync.xml index 132f62382c..d15f6d17f9 100644 --- a/tests/attachment/fixtures/resync.xml +++ b/tests/attachment/fixtures/resync.xml @@ -6,12 +6,16 @@ in_message is_orphan attach_comment + physical_filename + thumbnail 1 1 0 0 foo + foo + 0 1 @@ -19,6 +23,17 @@ 1 0 foo2 + foo2 + 0 + + + 1 + 1 + 1 + 0 + foo2 + foo2 + 1 diff --git a/tests/content_visibility/delete_post_test.php b/tests/content_visibility/delete_post_test.php index bb609b6ab7..c383ebfa2f 100644 --- a/tests/content_visibility/delete_post_test.php +++ b/tests/content_visibility/delete_post_test.php @@ -312,7 +312,7 @@ class phpbb_content_visibility_delete_post_test extends phpbb_database_test_case $lang_loader = new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx); $lang = new \phpbb\language\language($lang_loader); $user = new \phpbb\user($lang, '\phpbb\datetime'); - $attachment_delete = new \phpbb\attachment\delete($config, $db, new \phpbb\attachment\resync($db)); + $attachment_delete = new \phpbb\attachment\delete($config, $db, new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path); $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); diff --git a/tests/functions_user/delete_user_test.php b/tests/functions_user/delete_user_test.php index b877164629..f28c7286e3 100644 --- a/tests/functions_user/delete_user_test.php +++ b/tests/functions_user/delete_user_test.php @@ -25,7 +25,7 @@ class phpbb_functions_user_delete_user_test extends phpbb_database_test_case { parent::setUp(); - global $cache, $config, $db, $phpbb_dispatcher, $phpbb_container; + global $cache, $config, $db, $phpbb_dispatcher, $phpbb_container, $phpbb_root_path; $db = $this->db = $this->new_dbal(); $config = new \phpbb\config\config(array( @@ -36,7 +36,7 @@ class phpbb_functions_user_delete_user_test extends phpbb_database_test_case $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); - $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete($config, $db, new \phpbb\attachment\resync($db))); + $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete($config, $db, new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path)); $phpbb_container->set( 'auth.provider.db', new phpbb_mock_auth_provider() diff --git a/tests/privmsgs/delete_user_pms_test.php b/tests/privmsgs/delete_user_pms_test.php index 7345721d8b..1da4b76f9f 100644 --- a/tests/privmsgs/delete_user_pms_test.php +++ b/tests/privmsgs/delete_user_pms_test.php @@ -85,13 +85,13 @@ class phpbb_privmsgs_delete_user_pms_test extends phpbb_database_test_case */ public function test_delete_user_pms($delete_user, $remaining_privmsgs, $remaining_privmsgs_to) { - global $db, $phpbb_container; + global $db, $phpbb_container, $phpbb_root_path; $db = $this->new_dbal(); $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); - $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db, new \phpbb\attachment\resync($db))); + $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db, new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path)); phpbb_delete_user_pms($delete_user); From 98ebbbdca2b7a57136745354b204d9aef181df4a Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 23 Sep 2015 10:43:33 +0200 Subject: [PATCH 19/36] [ticket/14168] No longer use deprecated functions in core files PHPBB3-14168 --- phpBB/includes/acp/acp_attachments.php | 10 +++++++--- phpBB/includes/acp/acp_forums.php | 7 +++++-- phpBB/includes/acp/acp_users.php | 10 ++++++++-- phpBB/includes/functions_admin.php | 5 ++++- phpBB/includes/functions_privmsgs.php | 20 ++++++++------------ phpBB/includes/message_parser.php | 9 ++++++--- phpBB/includes/ucp/ucp_attachments.php | 10 ++++------ 7 files changed, 42 insertions(+), 29 deletions(-) diff --git a/phpBB/includes/acp/acp_attachments.php b/phpBB/includes/acp/acp_attachments.php index 7ff9846a75..49b8ea462c 100644 --- a/phpBB/includes/acp/acp_attachments.php +++ b/phpBB/includes/acp/acp_attachments.php @@ -39,6 +39,9 @@ class acp_attachments /** @var \phpbb\filesystem\filesystem_interface */ protected $filesystem; + /** @var \phpbb\attachment\delete */ + protected $attachment_delete; + public $id; public $u_action; protected $new_config; @@ -55,6 +58,7 @@ class acp_attachments $this->user = $user; $this->phpbb_container = $phpbb_container; $this->filesystem = $phpbb_filesystem; + $this->attachment_delete = $phpbb_container->get('attachment.delete'); $user->add_lang(array('posting', 'viewtopic', 'acp/attachments')); @@ -922,11 +926,11 @@ class acp_attachments $delete_files = array(); while ($row = $db->sql_fetchrow($result)) { - phpbb_unlink($row['physical_filename'], 'file'); + $this->attachment_delete->unlink_attachment($row['physical_filename'], 'file'); if ($row['thumbnail']) { - phpbb_unlink($row['physical_filename'], 'thumbnail'); + $this->attachment_delete->unlink_attachment($row['physical_filename'], 'thumbnail'); } $delete_files[$row['attach_id']] = $row['real_filename']; @@ -1091,7 +1095,7 @@ class acp_attachments } $db->sql_freeresult($result); - if ($num_deleted = delete_attachments('attach', $delete_files)) + if ($num_deleted = $this->attachment_delete->delete('attach', $delete_files)) { if (sizeof($delete_files) != $num_deleted) { diff --git a/phpBB/includes/acp/acp_forums.php b/phpBB/includes/acp/acp_forums.php index f252f2a594..905a885a56 100644 --- a/phpBB/includes/acp/acp_forums.php +++ b/phpBB/includes/acp/acp_forums.php @@ -1788,7 +1788,7 @@ class acp_forums */ function delete_forum_content($forum_id) { - global $db, $config, $phpbb_root_path, $phpEx, $phpbb_dispatcher; + global $db, $config, $phpbb_root_path, $phpEx, $phpbb_container, $phpbb_dispatcher; include_once($phpbb_root_path . 'includes/functions_posting.' . $phpEx); @@ -1809,7 +1809,10 @@ class acp_forums } $db->sql_freeresult($result); - delete_attachments('topic', $topic_ids, false); + /** @var \phpbb\attachment\delete $attachment_delete */ + $attachment_delete = $phpbb_container->get('attachment.delete'); + $attachment_delete->delete('topic', $topic_ids, false); + unset($attachment_delete); // Delete shadow topics pointing to topics in this forum delete_topic_shadows($forum_id); diff --git a/phpBB/includes/acp/acp_users.php b/phpBB/includes/acp/acp_users.php index 857c625867..ab0eda507e 100644 --- a/phpBB/includes/acp/acp_users.php +++ b/phpBB/includes/acp/acp_users.php @@ -543,7 +543,10 @@ class acp_users if (confirm_box(true)) { - delete_attachments('user', $user_id); + /** @var \phpbb\attachment\delete $attachment_delete */ + $attachment_delete = $phpbb_container->get('attachment.delete'); + $attachment_delete->delete('user', $user_id); + unset($attachment_delete); $phpbb_log->add('admin', $user->data['user_id'], $user->ip, 'LOG_USER_DEL_ATTACH', false, array($user_row['username'])); trigger_error($user->lang['USER_ATTACHMENTS_REMOVED'] . adm_back_link($this->u_action . '&u=' . $user_id)); @@ -2126,7 +2129,10 @@ class acp_users } $db->sql_freeresult($result); - delete_attachments('attach', $marked); + /** @var \phpbb\attachment\delete $attachment_delete */ + $attachment_delete = $phpbb_container->get('attachment.delete'); + $attachment_delete->delete('attach', $marked); + unset($attachment_delete); $message = (sizeof($log_attachments) == 1) ? $user->lang['ATTACHMENT_DELETED'] : $user->lang['ATTACHMENTS_DELETED']; diff --git a/phpBB/includes/functions_admin.php b/phpBB/includes/functions_admin.php index 8928aeb2d6..c267577117 100644 --- a/phpBB/includes/functions_admin.php +++ b/phpBB/includes/functions_admin.php @@ -1024,7 +1024,10 @@ function delete_posts($where_type, $where_ids, $auto_sync = true, $posted_sync = $search->index_remove($post_ids, $poster_ids, $forum_ids); - delete_attachments('post', $post_ids, false); + /** @var \phpbb\attachment\delete $attachment_delete */ + $attachment_delete = $phpbb_container->get('attachment.delete'); + $attachment_delete->delete('post', $post_ids, false); + unset($attachment_delete); /** * Perform additional actions during post(s) deletion diff --git a/phpBB/includes/functions_privmsgs.php b/phpBB/includes/functions_privmsgs.php index 7b6540cda7..fa84a5ed3c 100644 --- a/phpBB/includes/functions_privmsgs.php +++ b/phpBB/includes/functions_privmsgs.php @@ -1153,12 +1153,10 @@ function delete_pm($user_id, $msg_ids, $folder_id) if (sizeof($delete_ids)) { // Check if there are any attachments we need to remove - if (!function_exists('delete_attachments')) - { - include($phpbb_root_path . 'includes/functions_admin.' . $phpEx); - } - - delete_attachments('message', $delete_ids, false); + /** @var \phpbb\attachment\delete $attachment_delete */ + $attachment_delete = $phpbb_container->get('attachment.delete'); + $attachment_delete->delete('message', $delete_ids, false); + unset($attachment_delete); $sql = 'DELETE FROM ' . PRIVMSGS_TABLE . ' WHERE ' . $db->sql_in_set('msg_id', $delete_ids); @@ -1363,12 +1361,10 @@ function phpbb_delete_users_pms($user_ids) if (!empty($delete_ids)) { // Check if there are any attachments we need to remove - if (!function_exists('delete_attachments')) - { - include($phpbb_root_path . 'includes/functions_admin.' . $phpEx); - } - - delete_attachments('message', $delete_ids, false); + /** @var \phpbb\attachment\delete $attachment_delete */ + $attachment_delete = $phpbb_container->get('attachment.delete'); + $attachment_delete->delete('message', $delete_ids, false); + unset($attachment_delete); $sql = 'DELETE FROM ' . PRIVMSGS_TABLE . ' WHERE ' . $db->sql_in_set('msg_id', $delete_ids); diff --git a/phpBB/includes/message_parser.php b/phpBB/includes/message_parser.php index a56ef0d4a9..05ab115141 100644 --- a/phpBB/includes/message_parser.php +++ b/phpBB/includes/message_parser.php @@ -1643,6 +1643,9 @@ class parse_message extends bbcode_firstpass if ($index !== false && !empty($this->attachment_data[$index])) { + /** @var \phpbb\attachment\delete $attachment_delete */ + $attachment_delete = $phpbb_container->get('attachment.delete'); + // delete selected attachment if ($this->attachment_data[$index]['is_orphan']) { @@ -1657,11 +1660,11 @@ class parse_message extends bbcode_firstpass if ($row) { - phpbb_unlink($row['physical_filename'], 'file'); + $attachment_delete->unlink_attachment($row['physical_filename'], 'file'); if ($row['thumbnail']) { - phpbb_unlink($row['physical_filename'], 'thumbnail'); + $attachment_delete->unlink_attachment($row['physical_filename'], 'thumbnail'); } $db->sql_query('DELETE FROM ' . ATTACHMENTS_TABLE . ' WHERE attach_id = ' . (int) $this->attachment_data[$index]['attach_id']); @@ -1669,7 +1672,7 @@ class parse_message extends bbcode_firstpass } else { - delete_attachments('attach', array(intval($this->attachment_data[$index]['attach_id']))); + $attachment_delete->delete('attach', $this->attachment_data[$index]['attach_id']); } unset($this->attachment_data[$index]); diff --git a/phpBB/includes/ucp/ucp_attachments.php b/phpBB/includes/ucp/ucp_attachments.php index 639f308091..64cad1aa90 100644 --- a/phpBB/includes/ucp/ucp_attachments.php +++ b/phpBB/includes/ucp/ucp_attachments.php @@ -70,12 +70,10 @@ class ucp_attachments if (confirm_box(true)) { - if (!function_exists('delete_attachments')) - { - include_once($phpbb_root_path . 'includes/functions_admin.' . $phpEx); - } - - delete_attachments('attach', $delete_ids); + /** @var \phpbb\attachment\delete $attachment_delete */ + $attachment_delete = $phpbb_container->get('attachment.delete'); + $attachment_delete->delete('attach', $delete_ids); + unset($attachment_delete); meta_refresh(3, $this->u_action); $message = ((sizeof($delete_ids) == 1) ? $user->lang['ATTACHMENT_DELETED'] : $user->lang['ATTACHMENTS_DELETED']) . '

' . sprintf($user->lang['RETURN_UCP'], '', ''); From b7238bca0e4381e739a04ff9d257d358f0d31d7a Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 23 Sep 2015 11:37:27 +0200 Subject: [PATCH 20/36] [ticket/14168] Fix CS issue PHPBB3-14168 --- phpBB/includes/functions_admin.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/includes/functions_admin.php b/phpBB/includes/functions_admin.php index c267577117..d44c60e5ec 100644 --- a/phpBB/includes/functions_admin.php +++ b/phpBB/includes/functions_admin.php @@ -1251,7 +1251,7 @@ function update_posted_info(&$topic_ids) */ function phpbb_unlink($filename, $mode = 'file', $entry_removed = false) { - global $phpbb_container;; + global $phpbb_container; /** @var \phpbb\attachment\delete $attachment_delete */ $attachment_delete = $phpbb_container->get('attachment.delete'); From 8a6224bf8ad37ff5c739137dbbe4264e4328910e Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 23 Sep 2015 11:39:27 +0200 Subject: [PATCH 21/36] [ticket/14168] Add basic test file for attachments upload PHPBB3-14168 --- tests/attachment/upload_test.php | 183 +++++++++++++++++++++++++++++++ 1 file changed, 183 insertions(+) create mode 100644 tests/attachment/upload_test.php diff --git a/tests/attachment/upload_test.php b/tests/attachment/upload_test.php new file mode 100644 index 0000000000..2c08b5b72a --- /dev/null +++ b/tests/attachment/upload_test.php @@ -0,0 +1,183 @@ + +* @license GNU General Public License, version 2 (GPL-2.0) +* +* For full copyright and license information, please see +* the docs/CREDITS.txt file. +* +*/ + +require_once(dirname(__FILE__) . '/../../phpBB/includes/functions.php'); + +class phpbb_attachment_upload_test extends \phpbb_database_test_case +{ + /** @var \phpbb\auth\auth */ + protected $auth; + + /** @var \phpbb\cache\service */ + protected $cache; + + /** @var \phpbb\config\config */ + protected $config; + + /** @var \phpbb\files\upload */ + protected $files_upload; + + /** @var \phpbb\language\language */ + protected $language; + + /** @var \phpbb\mimetype\guesser */ + protected $mimetype_guesser; + + /** @var \phpbb\event\dispatcher */ + protected $phpbb_dispatcher; + + /** @var \phpbb\plupload\plupload */ + protected $plupload; + + /** @var \phpbb\user */ + protected $user; + + /** @var string phpBB root path */ + protected $phpbb_root_path; + + /** @var \phpbb\db\driver\driver_interface */ + protected $db; + + /** @var \phpbb\attachment\upload */ + protected $upload; + + private $filesystem; + + /** @var \Symfony\Component\DependencyInjection\ContainerInterface */ + protected $container; + + /** @var \phpbb\files\factory */ + protected $factory; + + /** @var \bantu\IniGetWrapper\IniGetWrapper */ + protected $php_ini; + + public function getDataSet() + { + return $this->createXMLDataSet(dirname(__FILE__) . '/fixtures/resync.xml'); + } + + public function setUp() + { + global $config, $phpbb_root_path, $phpEx; + + parent::setUp(); + + $this->auth = new \phpbb\auth\auth(); + $this->config = new \phpbb\config\config(array()); + $config = $this->config; + $this->db = $this->new_dbal(); + $this->cache = new \phpbb\cache\service(new \phpbb\cache\driver\dummy(), $this->config, $this->db, $phpbb_root_path, $phpEx); + $this->request = $this->getMock('\phpbb\request\request'); + + $this->filesystem = new \phpbb\filesystem\filesystem(); + $this->language = new \phpbb\language\language(new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx)); + $this->php_ini = new \bantu\IniGetWrapper\IniGetWrapper; + $guessers = array( + new \Symfony\Component\HttpFoundation\File\MimeType\FileinfoMimeTypeGuesser(), + new \Symfony\Component\HttpFoundation\File\MimeType\FileBinaryMimeTypeGuesser(), + new \phpbb\mimetype\content_guesser(), + new \phpbb\mimetype\extension_guesser(), + ); + $guessers[2]->set_priority(-2); + $guessers[3]->set_priority(-2); + $this->mimetype_guesser = new \phpbb\mimetype\guesser($guessers); + $this->plupload = new \phpbb\plupload\plupload($phpbb_root_path, $this->config, $this->request, new \phpbb\user($this->language, '\phpbb\datetime'), $this->php_ini, $this->mimetype_guesser); + $factory_mock = $this->getMockBuilder('\phpbb\files\factory') + ->disableOriginalConstructor() + ->getMock(); + $factory_mock->expects($this->any()) + ->method('get') + ->willReturn(new \phpbb\files\filespec( + $this->filesystem, + $this->language, + $this->php_ini, + new \FastImageSize\FastImageSize(), + $this->phpbb_root_path, + $this->mimetype_guesser + )); + + $this->container = new phpbb_mock_container_builder($phpbb_root_path, $phpEx); + $this->container->set('files.filespec', new \phpbb\files\filespec( + $this->filesystem, + $this->language, + $this->php_ini, + new \FastImageSize\FastImageSize(), + $phpbb_root_path, + new \phpbb\mimetype\guesser(array( + 'mimetype.extension_guesser' => new \phpbb\mimetype\extension_guesser(), + )))); + $this->container->set('files.types.form', new \phpbb\files\types\form( + $factory_mock, + $this->language, + $this->php_ini, + $this->plupload, + $this->request + )); + $this->container->set('files.types.local', new \phpbb\files\types\local( + $factory_mock, + $this->language, + $this->php_ini, + $this->request + )); + $this->factory = new \phpbb\files\factory($this->container); + $this->files_upload = new \phpbb\files\upload($this->filesystem, $this->factory, $this->language, $this->php_ini, $this->request, $this->phpbb_root_path); + $this->phpbb_dispatcher = new phpbb_mock_event_dispatcher(); + $this->user = new \phpbb\user($this->language, '\phpbb\datetime'); + + + $this->upload = new \phpbb\attachment\upload( + $this->auth, + $this->cache, + $this->config, + $this->files_upload, + $this->language, + $this->mimetype_guesser, + $this->phpbb_dispatcher, + $this->plupload, + $this->user, + $this->phpbb_root_path + ); + } + + public function data_upload() + { + return array( + array('foobar', 1, false, array( + 'error' => array( + 'Upload initiated but no valid file upload form found.', + ), + 'post_attach' => false, + ) + ), + array('foobar', 1, true, array( + 'error' => array( + 'NOT_UPLOADED', + ), + 'post_attach' => false, + 'thumbnail' => 0, + ) + ), + ); + } + + /** + * @dataProvider data_upload + */ + public function test_upload($form_name, $forum_id, $local, $expected) + { + $filedata = $this->upload->upload($form_name, $forum_id, $local); + + $this->assertSame($expected, $filedata); + } +} From 6c80fd92c6ab4aebb5701e6b1a46c46208d793c9 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sat, 3 Oct 2015 20:10:13 +0200 Subject: [PATCH 22/36] [ticket/14168] Add tests for init_error() during upload PHPBB3-14168 --- tests/attachment/upload_test.php | 49 ++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/tests/attachment/upload_test.php b/tests/attachment/upload_test.php index 2c08b5b72a..03d688cc1f 100644 --- a/tests/attachment/upload_test.php +++ b/tests/attachment/upload_test.php @@ -180,4 +180,53 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case $this->assertSame($expected, $filedata); } + + public function test_init_error() + { + $filespec = $this->getMockBuilder('\phpbb\files\filespec') + ->disableOriginalConstructor() + ->getMock(); + $filespec->expects($this->any()) + ->method('init_error') + ->willReturn(true); + $filespec->expects($this->any()) + ->method('set_upload_namespace') + ->willReturnSelf(); + $filespec->expects($this->any()) + ->method('set_upload_ary') + ->willReturnSelf(); + $this->container->set('files.filespec', $filespec); + $factory_mock = $this->getMockBuilder('\phpbb\files\factory') + ->disableOriginalConstructor() + ->getMock(); + $factory_mock->expects($this->any()) + ->method('get') + ->willReturn($filespec); + $this->container->set('files.types.local', new \phpbb\files\types\local( + $factory_mock, + $this->language, + $this->php_ini, + $this->request + )); + + $this->upload = new \phpbb\attachment\upload( + $this->auth, + $this->cache, + $this->config, + $this->files_upload, + $this->language, + $this->mimetype_guesser, + $this->phpbb_dispatcher, + $this->plupload, + $this->user, + $this->phpbb_root_path + ); + + $filedata = $this->upload->upload('foobar', 1, true); + + $this->assertSame(array( + 'error' => array(), + 'post_attach' => false, + ), $filedata); + } } From 36ea105236c87e84ca2b9f0a193b5b8721e8cf97 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 4 Oct 2015 11:10:07 +0200 Subject: [PATCH 23/36] [ticket/14168] Move image check and don't use trigger_error() PHPBB3-14168 --- phpBB/phpbb/attachment/upload.php | 8 +- tests/attachment/fixtures/resync.xml | 34 ++++++++ tests/attachment/upload_test.php | 125 +++++++++++++++++++++++++-- 3 files changed, 158 insertions(+), 9 deletions(-) diff --git a/phpBB/phpbb/attachment/upload.php b/phpBB/phpbb/attachment/upload.php index d57b33b137..ae8d70c18e 100644 --- a/phpBB/phpbb/attachment/upload.php +++ b/phpBB/phpbb/attachment/upload.php @@ -160,6 +160,9 @@ class upload // Do we have to create a thumbnail? $this->file_data['thumbnail'] = ($is_image && $this->config['img_create_thumbnail']) ? 1 : 0; + // Make sure the image category only holds valid images... + $this->check_image($is_image); + if (sizeof($this->file->error)) { $this->file->remove(); @@ -169,9 +172,6 @@ class upload return $this->file_data; } - // Make sure the image category only holds valid images... - $this->check_image($is_image); - $this->fill_file_data(); $filedata = $this->file_data; @@ -263,7 +263,7 @@ class upload // If this error occurs a user tried to exploit an IE Bug by renaming extensions // Since the image category is displaying content inline we need to catch this. - trigger_error($this->language->lang('ATTACHED_IMAGE_NOT_IMAGE')); + $this->file->set_error($this->language->lang('ATTACHED_IMAGE_NOT_IMAGE')); } } diff --git a/tests/attachment/fixtures/resync.xml b/tests/attachment/fixtures/resync.xml index d15f6d17f9..6e2cc62f68 100644 --- a/tests/attachment/fixtures/resync.xml +++ b/tests/attachment/fixtures/resync.xml @@ -36,6 +36,40 @@ 1
+ + extension + group_id + + jpg + 1 + + + png + 1 + +
+ + cat_id + group_id + download_mode + upload_icon + max_filesize + allow_group + allow_in_pm + allowed_forums + group_name + + 1 + 1 + 1 + + 1000 + 1 + 1 + a:1:{i:0;i:1;} + Images + +
post_idpost_text diff --git a/tests/attachment/upload_test.php b/tests/attachment/upload_test.php index 03d688cc1f..d1007dbd77 100644 --- a/tests/attachment/upload_test.php +++ b/tests/attachment/upload_test.php @@ -74,7 +74,9 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case parent::setUp(); $this->auth = new \phpbb\auth\auth(); - $this->config = new \phpbb\config\config(array()); + $this->config = new \phpbb\config\config(array( + 'upload_path' => '../attachment/fixtures/', + )); $config = $this->config; $this->db = $this->new_dbal(); $this->cache = new \phpbb\cache\service(new \phpbb\cache\driver\dummy(), $this->config, $this->db, $phpbb_root_path, $phpEx); @@ -153,14 +155,33 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case public function data_upload() { return array( - array('foobar', 1, false, array( + array('foobar', 1, false, + array(), + array( 'error' => array( 'Upload initiated but no valid file upload form found.', ), 'post_attach' => false, ) ), - array('foobar', 1, true, array( + array('foobar', 1, true, + array( + 'realname' => 'foobar.jpg', + 'type' => 'jpg', + 'size' => 100, + ), + array( + 'error' => array( + 'NOT_UPLOADED', + 'The image file you tried to attach is invalid.', + ), + 'post_attach' => false, + 'thumbnail' => 0, + ) + ), + array('foobar', 1, true, + array(), + array( 'error' => array( 'NOT_UPLOADED', ), @@ -174,9 +195,9 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case /** * @dataProvider data_upload */ - public function test_upload($form_name, $forum_id, $local, $expected) + public function test_upload($form_name, $forum_id, $local, $filedata, $expected) { - $filedata = $this->upload->upload($form_name, $forum_id, $local); + $filedata = $this->upload->upload($form_name, $forum_id, $local, '', false, $filedata); $this->assertSame($expected, $filedata); } @@ -229,4 +250,98 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case 'post_attach' => false, ), $filedata); } + + public function data_image_not_image() + { + return array( + array(false), + array(true), + ); + } + + /** + * @dataProvider data_image_not_image + */ + public function test_image_not_image($plupload_active) + { + $filespec = $this->getMock('\phpbb\files\filespec', + array( + 'init_error', + 'is_image', + 'move_file', + 'is_uploaded', + ), + array( + $this->filesystem, + $this->language, + $this->php_ini, + new \FastImageSize\FastImageSize(), + $this->phpbb_root_path, + $this->mimetype_guesser, + $this->plupload + )); + $filespec->set_upload_namespace($this->files_upload); + $filespec->expects($this->any()) + ->method('init_error') + ->willReturn(false); + $filespec->expects($this->any()) + ->method('is_image') + ->willReturn(false); + $filespec->expects($this->any()) + ->method('is_uploaded') + ->willReturn(true); + $filespec->expects($this->any()) + ->method('move_file') + ->willReturn(false); + $this->container->set('files.filespec', $filespec); + $factory_mock = $this->getMockBuilder('\phpbb\files\factory') + ->disableOriginalConstructor() + ->getMock(); + $factory_mock->expects($this->any()) + ->method('get') + ->willReturn($filespec); + $this->container->set('files.types.local', new \phpbb\files\types\local( + $factory_mock, + $this->language, + $this->php_ini, + $this->request + )); + + $plupload = $this->getMockBuilder('\phpbb\plupload\plupload') + ->disableOriginalConstructor() + ->getMock(); + $plupload->expects($this->any()) + ->method('is_active') + ->willReturn($plupload_active); + if ($plupload_active) + { + $plupload->expects($this->once()) + ->method('emit_error') + ->with(104, 'ATTACHED_IMAGE_NOT_IMAGE') + ->willReturn(false); + } + $this->upload = new \phpbb\attachment\upload( + $this->auth, + $this->cache, + $this->config, + $this->files_upload, + $this->language, + $this->mimetype_guesser, + $this->phpbb_dispatcher, + $plupload, + $this->user, + $this->phpbb_root_path + ); + + $filedata = $this->upload->upload('foobar', 1, true, '', false, array( + 'realname' => 'foobar.jpg', + 'type' => 'jpg', + 'size' => 100, + )); + $this->assertEquals(array( + 'error' => array('The image file you tried to attach is invalid.'), + 'post_attach' => false, + 'thumbnail' => 0, + ), $filedata); + } } From 1f1e708815369cc25f1c59c5ed69a49f80b64318 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Sun, 4 Oct 2015 12:28:12 +0200 Subject: [PATCH 24/36] [ticket/14168] Improve code coverage in upload class PHPBB3-14168 --- tests/attachment/upload_test.php | 109 +++++++++++++++++++++++++++---- 1 file changed, 96 insertions(+), 13 deletions(-) diff --git a/tests/attachment/upload_test.php b/tests/attachment/upload_test.php index d1007dbd77..295b6b15c9 100644 --- a/tests/attachment/upload_test.php +++ b/tests/attachment/upload_test.php @@ -12,6 +12,7 @@ */ require_once(dirname(__FILE__) . '/../../phpBB/includes/functions.php'); +require_once(dirname(__FILE__) . '/../../phpBB/includes/functions_posting.php'); class phpbb_attachment_upload_test extends \phpbb_database_test_case { @@ -75,7 +76,8 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case $this->auth = new \phpbb\auth\auth(); $this->config = new \phpbb\config\config(array( - 'upload_path' => '../attachment/fixtures/', + 'upload_path' => '', + 'img_create_thumbnail' => true, )); $config = $this->config; $this->db = $this->new_dbal(); @@ -176,7 +178,7 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case 'The image file you tried to attach is invalid.', ), 'post_attach' => false, - 'thumbnail' => 0, + 'thumbnail' => 1, ) ), array('foobar', 1, true, @@ -251,18 +253,89 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case ), $filedata); } - public function data_image_not_image() + public function data_image_upload() { return array( - array(false), - array(true), + array(false, false, array(), + array( + 'error' => array('The image file you tried to attach is invalid.'), + 'post_attach' => false, + 'thumbnail' => 1, + ) + ), + array(false, true, array(), + array( + 'error' => array('The image file you tried to attach is invalid.'), + 'post_attach' => false, + 'thumbnail' => 1, + ) + ), + array(true, false, array(), + array( + 'error' => array(), + 'post_attach' => true, + // thumbnail gets reset to 0 as creation was not possible + 'thumbnail' => 0, + 'filesize' => 100, + 'mimetype' => 'jpg', + 'extension' => 'jpg', + 'real_filename' => 'foobar.jpg', + ) + ), + array(true, false, + array( + 'check_attachment_content' => true, + 'mime_triggers' => '', + ), + array( + 'error' => array(), + 'post_attach' => true, + // thumbnail gets reset to 0 as creation was not possible + 'thumbnail' => 0, + 'filesize' => 100, + 'mimetype' => 'jpg', + 'extension' => 'jpg', + 'real_filename' => 'foobar.jpg', + ) + ), + array(true, false, + array( + 'attachment_quota' => 150, + ), + array( + 'error' => array(), + 'post_attach' => true, + // thumbnail gets reset to 0 as creation was not possible + 'thumbnail' => 0, + 'filesize' => 100, + 'mimetype' => 'jpg', + 'extension' => 'jpg', + 'real_filename' => 'foobar.jpg', + ) + ), + array(true, false, + array( + 'attachment_quota' => 50, + ), + array( + 'error' => array( + 'ATTACH_QUOTA_REACHED', + ), + 'post_attach' => false, + 'thumbnail' => 1, + 'filesize' => 100, + 'mimetype' => 'jpg', + 'extension' => 'jpg', + 'real_filename' => 'foobar.jpg', + ) + ), ); } /** - * @dataProvider data_image_not_image + * @dataProvider data_image_upload */ - public function test_image_not_image($plupload_active) + public function test_image_upload($is_image, $plupload_active, $config_data, $expected) { $filespec = $this->getMock('\phpbb\files\filespec', array( @@ -280,13 +353,17 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case $this->mimetype_guesser, $this->plupload )); + foreach ($config_data as $key => $value) + { + $this->config[$key] = $value; + } $filespec->set_upload_namespace($this->files_upload); $filespec->expects($this->any()) ->method('init_error') ->willReturn(false); $filespec->expects($this->any()) ->method('is_image') - ->willReturn(false); + ->willReturn($is_image); $filespec->expects($this->any()) ->method('is_uploaded') ->willReturn(true); @@ -338,10 +415,16 @@ class phpbb_attachment_upload_test extends \phpbb_database_test_case 'type' => 'jpg', 'size' => 100, )); - $this->assertEquals(array( - 'error' => array('The image file you tried to attach is invalid.'), - 'post_attach' => false, - 'thumbnail' => 0, - ), $filedata); + + foreach ($expected as $key => $entry) + { + $this->assertEquals($entry, $filedata[$key]); + } + + // Reset config data + foreach ($config_data as $key => $value) + { + $this->config->delete($key); + } } } From 11475283eb85d2dd029a57d0917b8cd7194d0886 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 5 Oct 2015 14:09:13 +0200 Subject: [PATCH 25/36] [ticket/14168] Move attachment service definitions to services_attachment PHPBB3-14168 --- .../default/container/services_attachment.yml | 32 +++++++++++++++++++ .../default/container/services_files.yml | 32 ------------------- 2 files changed, 32 insertions(+), 32 deletions(-) create mode 100644 phpBB/config/default/container/services_attachment.yml diff --git a/phpBB/config/default/container/services_attachment.yml b/phpBB/config/default/container/services_attachment.yml new file mode 100644 index 0000000000..8e3fbe8f35 --- /dev/null +++ b/phpBB/config/default/container/services_attachment.yml @@ -0,0 +1,32 @@ +services: + attachment.delete: + class: phpbb\attachment\delete + scope: prototype + arguments: + - @config + - @dbal.conn + - @dispatcher + - @filesystem + - @attachment.resync + - %core.root_path% + + attachment.resync: + class: phpbb\attachment\resync + scope: prototype + arguments: + - @dbal.conn + + attachment.upload: + class: phpbb\attachment\upload + scope: prototype + arguments: + - @auth + - @cache + - @config + - @files.upload + - @language + - @mimetype.guesser + - @dispatcher + - @plupload + - @user + - %core.root_path% diff --git a/phpBB/config/default/container/services_files.yml b/phpBB/config/default/container/services_files.yml index b94b48606d..cfdade37df 100644 --- a/phpBB/config/default/container/services_files.yml +++ b/phpBB/config/default/container/services_files.yml @@ -1,36 +1,4 @@ services: - attachment.delete: - class: phpbb\attachment\delete - scope: prototype - arguments: - - @config - - @dbal.conn - - @dispatcher - - @filesystem - - @attachment.resync - - %core.root_path% - - attachment.resync: - class: phpbb\attachment\resync - scope: prototype - arguments: - - @dbal.conn - - attachment.upload: - class: phpbb\attachment\upload - scope: prototype - arguments: - - @auth - - @cache - - @config - - @files.upload - - @language - - @mimetype.guesser - - @dispatcher - - @plupload - - @user - - %core.root_path% - filesystem: class: phpbb\filesystem\filesystem From 80e3c79f383366915b799675c6bcd3e15715780e Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Wed, 7 Oct 2015 11:04:15 +0200 Subject: [PATCH 26/36] [ticket/14168] Minor coding style fixes PHPBB3-14168 --- phpBB/phpbb/attachment/delete.php | 3 +-- phpBB/phpbb/attachment/resync.php | 1 - phpBB/phpbb/attachment/upload.php | 1 - 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/phpBB/phpbb/attachment/delete.php b/phpBB/phpbb/attachment/delete.php index ad385861a1..e093da8865 100644 --- a/phpBB/phpbb/attachment/delete.php +++ b/phpBB/phpbb/attachment/delete.php @@ -21,7 +21,6 @@ use \phpbb\filesystem\filesystem; /** * Attachment delete class */ - class delete { /** @var config */ @@ -189,7 +188,7 @@ class delete /** * Set attachment IDs * - * @param array $ids + * @param mixed $ids ID or array of IDs * * @return bool True if attachment IDs were set, false if not */ diff --git a/phpBB/phpbb/attachment/resync.php b/phpBB/phpbb/attachment/resync.php index adb642fe97..699d37340d 100644 --- a/phpBB/phpbb/attachment/resync.php +++ b/phpBB/phpbb/attachment/resync.php @@ -18,7 +18,6 @@ use \phpbb\db\driver\driver_interface; /** * Attachment delete class */ - class resync { /** @var driver_interface */ diff --git a/phpBB/phpbb/attachment/upload.php b/phpBB/phpbb/attachment/upload.php index ae8d70c18e..2d49e05b71 100644 --- a/phpBB/phpbb/attachment/upload.php +++ b/phpBB/phpbb/attachment/upload.php @@ -25,7 +25,6 @@ use \phpbb\user; /** * Attachment upload class */ - class upload { /** @var auth */ From ca1308ba5f3be0529bf536fcffb62fb61ee1807f Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Thu, 8 Oct 2015 15:05:08 +0200 Subject: [PATCH 27/36] [ticket/14168] Add services_attachment.yml to services.yml PHPBB3-14168 --- phpBB/config/default/container/services.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/phpBB/config/default/container/services.yml b/phpBB/config/default/container/services.yml index 9bbcdcbd57..49d2f21c43 100644 --- a/phpBB/config/default/container/services.yml +++ b/phpBB/config/default/container/services.yml @@ -1,4 +1,5 @@ imports: + - { resource: services_attachment.yml } - { resource: services_auth.yml } - { resource: services_avatar.yml } - { resource: services_captcha.yml } From 22b36d9d0e50fdc820912cca0164fccc7b5785a1 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 9 Oct 2015 08:03:53 +0200 Subject: [PATCH 28/36] [ticket/14168] Use correct docblock PHPBB3-14168 --- phpBB/phpbb/attachment/resync.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/phpBB/phpbb/attachment/resync.php b/phpBB/phpbb/attachment/resync.php index 699d37340d..6c2e0a8b0d 100644 --- a/phpBB/phpbb/attachment/resync.php +++ b/phpBB/phpbb/attachment/resync.php @@ -16,7 +16,7 @@ namespace phpbb\attachment; use \phpbb\db\driver\driver_interface; /** - * Attachment delete class + * Attachment resync class */ class resync { From c78dbd2eea2dae14b076d8d800474c8715982277 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 9 Oct 2015 09:45:28 +0200 Subject: [PATCH 29/36] [ticket/14168] Add attachment manager service PHPBB3-14168 --- .../default/container/services_attachment.yml | 8 ++ phpBB/phpbb/attachment/manager.php | 99 +++++++++++++++++++ 2 files changed, 107 insertions(+) create mode 100644 phpBB/phpbb/attachment/manager.php diff --git a/phpBB/config/default/container/services_attachment.yml b/phpBB/config/default/container/services_attachment.yml index 8e3fbe8f35..5506b2ec90 100644 --- a/phpBB/config/default/container/services_attachment.yml +++ b/phpBB/config/default/container/services_attachment.yml @@ -10,6 +10,14 @@ services: - @attachment.resync - %core.root_path% + attachment.manager: + class: phpbb\attachment\manager + scope: prototype + arguments: + - @attachment.delete + - @attachment.resync + - @attachment.upload + attachment.resync: class: phpbb\attachment\resync scope: prototype diff --git a/phpBB/phpbb/attachment/manager.php b/phpBB/phpbb/attachment/manager.php new file mode 100644 index 0000000000..0aeadf3e3e --- /dev/null +++ b/phpBB/phpbb/attachment/manager.php @@ -0,0 +1,99 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +namespace phpbb\attachment; + +/** + * Attachment manager + */ +class manager +{ + /** @var delete Attachment delete class */ + protected $delete; + + /** @var resync Attachment resync class */ + protected $resync; + + /** @var upload Attachment upload class */ + protected $upload; + + /** + * Constructor for attachment manager + * + * @param delete $delete Attachment delete class + * @param resync $resync Attachment resync class + * @param upload $upload Attachment upload class + */ + public function __construct(delete $delete, resync $resync, upload $upload) + { + $this->delete = $delete; + $this->resync = $resync; + $this->upload = $upload; + } + + /** + * Wrapper method for deleting attachments + * + * @param string $mode can be: post|message|topic|attach|user + * @param mixed $ids can be: post_ids, message_ids, topic_ids, attach_ids, user_ids + * @param bool $resync set this to false if you are deleting posts or topics + * + * @return int|bool Number of deleted attachments or false if something + * went wrong during attachment deletion + */ + public function delete($mode, $id, $resync = true) + { + $this->delete->delete($mode, $id, $resync); + } + + /** + * Wrapper method for deleting attachments from filesystem + * + * @param string $filename Filename of attachment + * @param string $mode Delete mode + * @param bool $entry_removed Whether entry was removed. Defaults to false + * @return bool True if file was removed, false if not + */ + public function unlink($filename, $mode = 'file', $entry_removed = false) + { + $this->delete->unlink_attachment($filename, $mode, $entry_removed); + } + + /** + * Wrapper method for resyncing specified type + * + * @param string $type Type of resync + * @param array $ids IDs to resync + */ + public function resync($type, $ids) + { + $this->resync->resync($type, $ids); + } + + /** + * Wrapper method for uploading attachment + * + * @param string $form_name The form name of the file upload input + * @param int $forum_id The id of the forum + * @param bool $local Whether the file is local or not + * @param string $local_storage The path to the local file + * @param bool $is_message Whether it is a PM or not + * @param array $local_filedata An file data object created for the local file + * + * @return object filespec + */ + public function upload($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = []) + { + $this->upload->upload($form_name, $forum_id, $local, $local_storage, $is_message, $local_filedata); + } +} From 88033feb85f554936462b7652c80a7a815128d2b Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 9 Oct 2015 11:15:21 +0200 Subject: [PATCH 30/36] [ticket/14168] Fix tests after rebase PHPBB3-14168 --- tests/attachment/delete_test.php | 5 +++-- tests/content_visibility/delete_post_test.php | 2 +- tests/functions_user/delete_user_test.php | 2 +- tests/privmsgs/delete_user_pms_test.php | 2 +- 4 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/attachment/delete_test.php b/tests/attachment/delete_test.php index 8db6487542..f1835dd37a 100644 --- a/tests/attachment/delete_test.php +++ b/tests/attachment/delete_test.php @@ -55,7 +55,8 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case ->method('exists') ->willReturn(true); $this->phpbb_root_path = $phpbb_root_path; - $this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->filesystem, $this->resync, $phpbb_root_path); + $this->dispatcher = new \phpbb_mock_event_dispatcher(); + $this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->dispatcher, $this->filesystem, $this->resync, $phpbb_root_path); } public function data_attachment_delete() @@ -120,7 +121,7 @@ class phpbb_attachment_delete_test extends \phpbb_database_test_case ->method('exists') ->willReturn($exists_success); - $this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->filesystem, $this->resync, $this->phpbb_root_path); + $this->attachment_delete = new \phpbb\attachment\delete($this->config, $this->db, $this->dispatcher, $this->filesystem, $this->resync, $this->phpbb_root_path); $this->assertSame($expected, $this->attachment_delete->unlink_attachment('foobar')); } } diff --git a/tests/content_visibility/delete_post_test.php b/tests/content_visibility/delete_post_test.php index c383ebfa2f..f174b99d51 100644 --- a/tests/content_visibility/delete_post_test.php +++ b/tests/content_visibility/delete_post_test.php @@ -312,7 +312,7 @@ class phpbb_content_visibility_delete_post_test extends phpbb_database_test_case $lang_loader = new \phpbb\language\language_file_loader($phpbb_root_path, $phpEx); $lang = new \phpbb\language\language($lang_loader); $user = new \phpbb\user($lang, '\phpbb\datetime'); - $attachment_delete = new \phpbb\attachment\delete($config, $db, new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path); + $attachment_delete = new \phpbb\attachment\delete($config, $db, new \phpbb_mock_event_dispatcher(), new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path); $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); diff --git a/tests/functions_user/delete_user_test.php b/tests/functions_user/delete_user_test.php index f28c7286e3..1a5a4f672e 100644 --- a/tests/functions_user/delete_user_test.php +++ b/tests/functions_user/delete_user_test.php @@ -36,7 +36,7 @@ class phpbb_functions_user_delete_user_test extends phpbb_database_test_case $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); - $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete($config, $db, new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path)); + $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete($config, $db, new \phpbb_mock_event_dispatcher(), new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path)); $phpbb_container->set( 'auth.provider.db', new phpbb_mock_auth_provider() diff --git a/tests/privmsgs/delete_user_pms_test.php b/tests/privmsgs/delete_user_pms_test.php index 1da4b76f9f..329c401316 100644 --- a/tests/privmsgs/delete_user_pms_test.php +++ b/tests/privmsgs/delete_user_pms_test.php @@ -91,7 +91,7 @@ class phpbb_privmsgs_delete_user_pms_test extends phpbb_database_test_case $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); - $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db, new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path)); + $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db, new \phpbb_mock_event_dispatcher(), new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path)); phpbb_delete_user_pms($delete_user); From 53008c87828746c316019e758641a2a4e37f522b Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 9 Oct 2015 12:14:19 +0200 Subject: [PATCH 31/36] [ticket/14168] Fix tabs in manager and add test file PHPBB3-14168 --- phpBB/phpbb/attachment/manager.php | 142 ++++++++++++++--------------- tests/attachment/manager_test.php | 69 ++++++++++++++ 2 files changed, 140 insertions(+), 71 deletions(-) create mode 100644 tests/attachment/manager_test.php diff --git a/phpBB/phpbb/attachment/manager.php b/phpBB/phpbb/attachment/manager.php index 0aeadf3e3e..414a8edfdd 100644 --- a/phpBB/phpbb/attachment/manager.php +++ b/phpBB/phpbb/attachment/manager.php @@ -18,82 +18,82 @@ namespace phpbb\attachment; */ class manager { - /** @var delete Attachment delete class */ - protected $delete; + /** @var delete Attachment delete class */ + protected $delete; - /** @var resync Attachment resync class */ - protected $resync; + /** @var resync Attachment resync class */ + protected $resync; - /** @var upload Attachment upload class */ - protected $upload; + /** @var upload Attachment upload class */ + protected $upload; - /** - * Constructor for attachment manager - * - * @param delete $delete Attachment delete class - * @param resync $resync Attachment resync class - * @param upload $upload Attachment upload class - */ - public function __construct(delete $delete, resync $resync, upload $upload) - { - $this->delete = $delete; - $this->resync = $resync; - $this->upload = $upload; - } + /** + * Constructor for attachment manager + * + * @param delete $delete Attachment delete class + * @param resync $resync Attachment resync class + * @param upload $upload Attachment upload class + */ + public function __construct(delete $delete, resync $resync, upload $upload) + { + $this->delete = $delete; + $this->resync = $resync; + $this->upload = $upload; + } - /** - * Wrapper method for deleting attachments - * - * @param string $mode can be: post|message|topic|attach|user - * @param mixed $ids can be: post_ids, message_ids, topic_ids, attach_ids, user_ids - * @param bool $resync set this to false if you are deleting posts or topics - * - * @return int|bool Number of deleted attachments or false if something - * went wrong during attachment deletion - */ - public function delete($mode, $id, $resync = true) - { - $this->delete->delete($mode, $id, $resync); - } + /** + * Wrapper method for deleting attachments + * + * @param string $mode can be: post|message|topic|attach|user + * @param mixed $ids can be: post_ids, message_ids, topic_ids, attach_ids, user_ids + * @param bool $resync set this to false if you are deleting posts or topics + * + * @return int|bool Number of deleted attachments or false if something + * went wrong during attachment deletion + */ + public function delete($mode, $id, $resync = true) + { + return $this->delete->delete($mode, $id, $resync); + } - /** - * Wrapper method for deleting attachments from filesystem - * - * @param string $filename Filename of attachment - * @param string $mode Delete mode - * @param bool $entry_removed Whether entry was removed. Defaults to false - * @return bool True if file was removed, false if not - */ - public function unlink($filename, $mode = 'file', $entry_removed = false) - { - $this->delete->unlink_attachment($filename, $mode, $entry_removed); - } + /** + * Wrapper method for deleting attachments from filesystem + * + * @param string $filename Filename of attachment + * @param string $mode Delete mode + * @param bool $entry_removed Whether entry was removed. Defaults to false + * @return bool True if file was removed, false if not + */ + public function unlink($filename, $mode = 'file', $entry_removed = false) + { + return $this->delete->unlink_attachment($filename, $mode, $entry_removed); + } - /** - * Wrapper method for resyncing specified type - * - * @param string $type Type of resync - * @param array $ids IDs to resync - */ - public function resync($type, $ids) - { - $this->resync->resync($type, $ids); - } + /** + * Wrapper method for resyncing specified type + * + * @param string $type Type of resync + * @param array $ids IDs to resync + */ + public function resync($type, $ids) + { + $this->resync->resync($type, $ids); + } - /** - * Wrapper method for uploading attachment - * - * @param string $form_name The form name of the file upload input - * @param int $forum_id The id of the forum - * @param bool $local Whether the file is local or not - * @param string $local_storage The path to the local file - * @param bool $is_message Whether it is a PM or not - * @param array $local_filedata An file data object created for the local file - * - * @return object filespec - */ - public function upload($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = []) - { - $this->upload->upload($form_name, $forum_id, $local, $local_storage, $is_message, $local_filedata); - } + /** + * Wrapper method for uploading attachment + * + * @param string $form_name The form name of the file upload input + * @param int $forum_id The id of the forum + * @param bool $local Whether the file is local or not + * @param string $local_storage The path to the local file + * @param bool $is_message Whether it is a PM or not + * @param array $local_filedata An file data object created for the local file + * + * @return object filespec + */ + public function upload($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = []) + { + return $this->upload->upload($form_name, $forum_id, $local, $local_storage, $is_message, $local_filedata); + } } diff --git a/tests/attachment/manager_test.php b/tests/attachment/manager_test.php new file mode 100644 index 0000000000..f71ccfbb6c --- /dev/null +++ b/tests/attachment/manager_test.php @@ -0,0 +1,69 @@ + + * @license GNU General Public License, version 2 (GPL-2.0) + * + * For full copyright and license information, please see + * the docs/CREDITS.txt file. + * + */ + +class phpbb_attachment_manager_test extends \phpbb_test_case +{ + protected $delete; + protected $resync; + protected $upload; + + public function setUp() + { + $this->delete = $this->getMockBuilder('\phpbb\attachment\delete') + ->disableOriginalConstructor() + ->setMethods(['delete', 'unlink']) + ->getMock(); + $this->resync = $this->getMockBuilder('\phpbb\attachment\resync') + ->disableOriginalConstructor() + ->setMethods(['resync']) + ->getMock(); + $this->upload = $this->getMockBuilder('\phpbb\attachment\upload') + ->disableOriginalConstructor() + ->setMethods(['upload']) + ->getMock(); + } + + protected function get_manager() + { + return new \phpbb\attachment\manager($this->delete, $this->resync, $this->upload); + } + + public function data_delete() + { + return array( + [ + ['foo', [1, 2, 3], false], + ['foo', [1, 2, 3], false], + true, + ], + [ + ['foo', [1, 2, 3], true], + ['foo', [1, 2, 3]], + true, + ], + ); + } + + /** + * @dataProvider data_delete + */ + public function test_delete($input, $input_manager, $output) + { + $mock = $this->delete->expects($this->atLeastOnce()) + ->method('delete'); + $mock = call_user_func_array([$mock, 'with'], $input); + $mock->willReturn($output); + $manager = $this->get_manager(); + $this->assertSame($output, call_user_func_array([$manager, 'delete'], $input_manager)); + } +} From b90783a296bc15427e9f29a5ed4d0912afcfa6b4 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 9 Oct 2015 12:29:00 +0200 Subject: [PATCH 32/36] [ticket/14168] Add new test method and more tests One test method to rule them all. PHPBB3-14168 --- tests/attachment/manager_test.php | 45 ++++++++++++++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/tests/attachment/manager_test.php b/tests/attachment/manager_test.php index f71ccfbb6c..85a6465ab9 100644 --- a/tests/attachment/manager_test.php +++ b/tests/attachment/manager_test.php @@ -21,7 +21,7 @@ class phpbb_attachment_manager_test extends \phpbb_test_case { $this->delete = $this->getMockBuilder('\phpbb\attachment\delete') ->disableOriginalConstructor() - ->setMethods(['delete', 'unlink']) + ->setMethods(['delete', 'unlink_attachment']) ->getMock(); $this->resync = $this->getMockBuilder('\phpbb\attachment\resync') ->disableOriginalConstructor() @@ -66,4 +66,47 @@ class phpbb_attachment_manager_test extends \phpbb_test_case $manager = $this->get_manager(); $this->assertSame($output, call_user_func_array([$manager, 'delete'], $input_manager)); } + + public function data_manager() + { + return array( + array( + 'delete', + 'unlink_attachment', + 'unlink', + ['foo'], + ['foo', 'file', false], + true, + ), + array( + 'delete', + 'unlink_attachment', + 'unlink', + ['foo', 'bar'], + ['foo', 'bar', false], + true, + ), + array( + 'delete', + 'unlink_attachment', + 'unlink', + ['foo', 'bar', true], + ['foo', 'bar', true], + true, + ), + ); + } + + /** + * @dataProvider data_manager + */ + public function test_manager($class, $method_class, $method_manager, $input_manager, $input_method, $output) + { + $mock = call_user_func_array([$this->{$class}, 'expects'], [$this->atLeastOnce()]); + $mock = $mock->method($method_class); + $mock = call_user_func_array([$mock, 'with'], $input_method); + $mock->willReturn($output); + $manager = $this->get_manager(); + $this->assertSame($output, call_user_func_array([$manager, $method_manager], $input_manager)); + } } From 52dccd3dba0808e4e19aa8407da1d4c75fe241cd Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 9 Oct 2015 14:33:12 +0200 Subject: [PATCH 33/36] [ticket/14168] Add more test cases for attachment manager PHPBB3-14168 --- tests/attachment/manager_test.php | 81 +++++++++++++++++++------------ 1 file changed, 50 insertions(+), 31 deletions(-) diff --git a/tests/attachment/manager_test.php b/tests/attachment/manager_test.php index 85a6465ab9..01a6049602 100644 --- a/tests/attachment/manager_test.php +++ b/tests/attachment/manager_test.php @@ -38,35 +38,6 @@ class phpbb_attachment_manager_test extends \phpbb_test_case return new \phpbb\attachment\manager($this->delete, $this->resync, $this->upload); } - public function data_delete() - { - return array( - [ - ['foo', [1, 2, 3], false], - ['foo', [1, 2, 3], false], - true, - ], - [ - ['foo', [1, 2, 3], true], - ['foo', [1, 2, 3]], - true, - ], - ); - } - - /** - * @dataProvider data_delete - */ - public function test_delete($input, $input_manager, $output) - { - $mock = $this->delete->expects($this->atLeastOnce()) - ->method('delete'); - $mock = call_user_func_array([$mock, 'with'], $input); - $mock->willReturn($output); - $manager = $this->get_manager(); - $this->assertSame($output, call_user_func_array([$manager, 'delete'], $input_manager)); - } - public function data_manager() { return array( @@ -77,6 +48,7 @@ class phpbb_attachment_manager_test extends \phpbb_test_case ['foo'], ['foo', 'file', false], true, + true, ), array( 'delete', @@ -85,6 +57,7 @@ class phpbb_attachment_manager_test extends \phpbb_test_case ['foo', 'bar'], ['foo', 'bar', false], true, + true, ), array( 'delete', @@ -93,6 +66,52 @@ class phpbb_attachment_manager_test extends \phpbb_test_case ['foo', 'bar', true], ['foo', 'bar', true], true, + true, + ), + array( + 'delete', + 'delete', + 'delete', + ['foo', [1, 2, 3]], + ['foo', [1, 2, 3], true], + true, + true, + ), + array( + 'delete', + 'delete', + 'delete', + ['foo', [1, 2, 3], false], + ['foo', [1, 2, 3], false], + true, + true, + ), + array( + 'resync', + 'resync', + 'resync', + ['foo', [1, 2, 3]], + ['foo', [1, 2, 3]], + true, + null, + ), + array( + 'upload', + 'upload', + 'upload', + ['foo', 1], + ['foo', 1, false, '', false, []], + true, + true, + ), + array( + 'upload', + 'upload', + 'upload', + ['foo', 1, true, 'bar', true, ['filename' => 'foobar']], + ['foo', 1, true, 'bar', true, ['filename' => 'foobar']], + true, + true, ), ); } @@ -100,12 +119,12 @@ class phpbb_attachment_manager_test extends \phpbb_test_case /** * @dataProvider data_manager */ - public function test_manager($class, $method_class, $method_manager, $input_manager, $input_method, $output) + public function test_manager($class, $method_class, $method_manager, $input_manager, $input_method, $return, $output) { $mock = call_user_func_array([$this->{$class}, 'expects'], [$this->atLeastOnce()]); $mock = $mock->method($method_class); $mock = call_user_func_array([$mock, 'with'], $input_method); - $mock->willReturn($output); + $mock->willReturn($return); $manager = $this->get_manager(); $this->assertSame($output, call_user_func_array([$manager, $method_manager], $input_manager)); } From a0167ad41037d7fe9214ca36c4c7097177361a6b Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Fri, 9 Oct 2015 14:35:40 +0200 Subject: [PATCH 34/36] [ticket/14168] Fix docblock in manager PHPBB3-14168 --- phpBB/phpbb/attachment/manager.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/phpBB/phpbb/attachment/manager.php b/phpBB/phpbb/attachment/manager.php index 414a8edfdd..9dcd3c5c92 100644 --- a/phpBB/phpbb/attachment/manager.php +++ b/phpBB/phpbb/attachment/manager.php @@ -51,9 +51,9 @@ class manager * @return int|bool Number of deleted attachments or false if something * went wrong during attachment deletion */ - public function delete($mode, $id, $resync = true) + public function delete($mode, $ids, $resync = true) { - return $this->delete->delete($mode, $id, $resync); + return $this->delete->delete($mode, $ids, $resync); } /** From 49312f05f88ca58346cbb00c2f03f01fd2a3d56d Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 12 Oct 2015 11:34:11 +0200 Subject: [PATCH 35/36] [ticket/14168] Use attachment manager instead of separate classes PHPBB3-14168 --- phpBB/includes/acp/acp_attachments.php | 12 +++++----- phpBB/includes/acp/acp_forums.php | 8 +++---- phpBB/includes/acp/acp_users.php | 16 ++++++------- phpBB/includes/functions_admin.php | 24 +++++++++---------- phpBB/includes/functions_posting.php | 8 +++---- phpBB/includes/functions_privmsgs.php | 16 ++++++------- phpBB/includes/message_parser.php | 22 ++++++++--------- phpBB/includes/ucp/ucp_attachments.php | 8 +++---- tests/attachment/manager_test.php | 8 +++---- tests/content_visibility/delete_post_test.php | 3 ++- tests/functions_user/delete_user_test.php | 3 ++- tests/privmsgs/delete_user_pms_test.php | 3 ++- 12 files changed, 67 insertions(+), 64 deletions(-) diff --git a/phpBB/includes/acp/acp_attachments.php b/phpBB/includes/acp/acp_attachments.php index 49b8ea462c..e2090f3cd5 100644 --- a/phpBB/includes/acp/acp_attachments.php +++ b/phpBB/includes/acp/acp_attachments.php @@ -39,8 +39,8 @@ class acp_attachments /** @var \phpbb\filesystem\filesystem_interface */ protected $filesystem; - /** @var \phpbb\attachment\delete */ - protected $attachment_delete; + /** @var \phpbb\attachment\manager */ + protected $attachment_manager; public $id; public $u_action; @@ -58,7 +58,7 @@ class acp_attachments $this->user = $user; $this->phpbb_container = $phpbb_container; $this->filesystem = $phpbb_filesystem; - $this->attachment_delete = $phpbb_container->get('attachment.delete'); + $this->attachment_manager = $phpbb_container->get('attachment.manager'); $user->add_lang(array('posting', 'viewtopic', 'acp/attachments')); @@ -926,11 +926,11 @@ class acp_attachments $delete_files = array(); while ($row = $db->sql_fetchrow($result)) { - $this->attachment_delete->unlink_attachment($row['physical_filename'], 'file'); + $this->attachment_manager->unlink($row['physical_filename'], 'file'); if ($row['thumbnail']) { - $this->attachment_delete->unlink_attachment($row['physical_filename'], 'thumbnail'); + $this->attachment_manager->unlink($row['physical_filename'], 'thumbnail'); } $delete_files[$row['attach_id']] = $row['real_filename']; @@ -1095,7 +1095,7 @@ class acp_attachments } $db->sql_freeresult($result); - if ($num_deleted = $this->attachment_delete->delete('attach', $delete_files)) + if ($num_deleted = $this->attachment_manager->delete('attach', $delete_files)) { if (sizeof($delete_files) != $num_deleted) { diff --git a/phpBB/includes/acp/acp_forums.php b/phpBB/includes/acp/acp_forums.php index 905a885a56..5d20664b31 100644 --- a/phpBB/includes/acp/acp_forums.php +++ b/phpBB/includes/acp/acp_forums.php @@ -1809,10 +1809,10 @@ class acp_forums } $db->sql_freeresult($result); - /** @var \phpbb\attachment\delete $attachment_delete */ - $attachment_delete = $phpbb_container->get('attachment.delete'); - $attachment_delete->delete('topic', $topic_ids, false); - unset($attachment_delete); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $attachment_manager->delete('topic', $topic_ids, false); + unset($attachment_manager); // Delete shadow topics pointing to topics in this forum delete_topic_shadows($forum_id); diff --git a/phpBB/includes/acp/acp_users.php b/phpBB/includes/acp/acp_users.php index ab0eda507e..f26c0d3f93 100644 --- a/phpBB/includes/acp/acp_users.php +++ b/phpBB/includes/acp/acp_users.php @@ -543,10 +543,10 @@ class acp_users if (confirm_box(true)) { - /** @var \phpbb\attachment\delete $attachment_delete */ - $attachment_delete = $phpbb_container->get('attachment.delete'); - $attachment_delete->delete('user', $user_id); - unset($attachment_delete); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $attachment_manager->delete('user', $user_id); + unset($attachment_manager); $phpbb_log->add('admin', $user->data['user_id'], $user->ip, 'LOG_USER_DEL_ATTACH', false, array($user_row['username'])); trigger_error($user->lang['USER_ATTACHMENTS_REMOVED'] . adm_back_link($this->u_action . '&u=' . $user_id)); @@ -2129,10 +2129,10 @@ class acp_users } $db->sql_freeresult($result); - /** @var \phpbb\attachment\delete $attachment_delete */ - $attachment_delete = $phpbb_container->get('attachment.delete'); - $attachment_delete->delete('attach', $marked); - unset($attachment_delete); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $attachment_manager->delete('attach', $marked); + unset($attachment_manager); $message = (sizeof($log_attachments) == 1) ? $user->lang['ATTACHMENT_DELETED'] : $user->lang['ATTACHMENTS_DELETED']; diff --git a/phpBB/includes/functions_admin.php b/phpBB/includes/functions_admin.php index d44c60e5ec..bca451deb6 100644 --- a/phpBB/includes/functions_admin.php +++ b/phpBB/includes/functions_admin.php @@ -1024,10 +1024,10 @@ function delete_posts($where_type, $where_ids, $auto_sync = true, $posted_sync = $search->index_remove($post_ids, $poster_ids, $forum_ids); - /** @var \phpbb\attachment\delete $attachment_delete */ - $attachment_delete = $phpbb_container->get('attachment.delete'); - $attachment_delete->delete('post', $post_ids, false); - unset($attachment_delete); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $attachment_manager->delete('post', $post_ids, false); + unset($attachment_manager); /** * Perform additional actions during post(s) deletion @@ -1124,11 +1124,11 @@ function delete_attachments($mode, $ids, $resync = true) { global $phpbb_container; - /** @var \phpbb\attachment\delete $attachment_delete */ - $attachment_delete = $phpbb_container->get('attachment.delete'); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $num_deleted = $attachment_manager->delete($mode, $ids, $resync); - $num_deleted = $attachment_delete->delete($mode, $ids, $resync); - unset($attachment_delete); + unset($attachment_manager); return $num_deleted; } @@ -1253,10 +1253,10 @@ function phpbb_unlink($filename, $mode = 'file', $entry_removed = false) { global $phpbb_container; - /** @var \phpbb\attachment\delete $attachment_delete */ - $attachment_delete = $phpbb_container->get('attachment.delete'); - $unlink = $attachment_delete->unlink_attachment($filename, $mode, $entry_removed); - unset($attachment_delete); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $unlink = $attachment_manager->unlink($filename, $mode, $entry_removed); + unset($attachment_manager); return $unlink; } diff --git a/phpBB/includes/functions_posting.php b/phpBB/includes/functions_posting.php index c39b9470a3..297ac80170 100644 --- a/phpBB/includes/functions_posting.php +++ b/phpBB/includes/functions_posting.php @@ -406,10 +406,10 @@ function upload_attachment($form_name, $forum_id, $local = false, $local_storage { global $phpbb_container; - /** @var \phpbb\attachment\upload $attachment_upload */ - $attachment_upload = $phpbb_container->get('attachment.upload'); - $file = $attachment_upload->upload($form_name, $forum_id, $local, $local_storage, $is_message, $local_filedata); - unset($attachment_upload); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $file = $attachment_manager->upload($form_name, $forum_id, $local, $local_storage, $is_message, $local_filedata); + unset($attachment_manager); return $file; } diff --git a/phpBB/includes/functions_privmsgs.php b/phpBB/includes/functions_privmsgs.php index fa84a5ed3c..ee18e85657 100644 --- a/phpBB/includes/functions_privmsgs.php +++ b/phpBB/includes/functions_privmsgs.php @@ -1153,10 +1153,10 @@ function delete_pm($user_id, $msg_ids, $folder_id) if (sizeof($delete_ids)) { // Check if there are any attachments we need to remove - /** @var \phpbb\attachment\delete $attachment_delete */ - $attachment_delete = $phpbb_container->get('attachment.delete'); - $attachment_delete->delete('message', $delete_ids, false); - unset($attachment_delete); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $attachment_manager->delete('message', $delete_ids, false); + unset($attachment_manager); $sql = 'DELETE FROM ' . PRIVMSGS_TABLE . ' WHERE ' . $db->sql_in_set('msg_id', $delete_ids); @@ -1361,10 +1361,10 @@ function phpbb_delete_users_pms($user_ids) if (!empty($delete_ids)) { // Check if there are any attachments we need to remove - /** @var \phpbb\attachment\delete $attachment_delete */ - $attachment_delete = $phpbb_container->get('attachment.delete'); - $attachment_delete->delete('message', $delete_ids, false); - unset($attachment_delete); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $attachment_manager->delete('message', $delete_ids, false); + unset($attachment_manager); $sql = 'DELETE FROM ' . PRIVMSGS_TABLE . ' WHERE ' . $db->sql_in_set('msg_id', $delete_ids); diff --git a/phpBB/includes/message_parser.php b/phpBB/includes/message_parser.php index 05ab115141..059037168d 100644 --- a/phpBB/includes/message_parser.php +++ b/phpBB/includes/message_parser.php @@ -1571,9 +1571,9 @@ class parse_message extends bbcode_firstpass { if ($num_attachments < $cfg['max_attachments'] || $auth->acl_get('a_') || $auth->acl_get('m_', $forum_id)) { - /** @var \phpbb\attachment\upload $attachment_upload */ - $attachment_upload = $phpbb_container->get('attachment.upload'); - $filedata = $attachment_upload->upload($form_name, $forum_id, false, '', $is_message); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $filedata = $attachment_manager->upload($form_name, $forum_id, false, '', $is_message); $error = $filedata['error']; if ($filedata['post_attach'] && !sizeof($error)) @@ -1643,8 +1643,8 @@ class parse_message extends bbcode_firstpass if ($index !== false && !empty($this->attachment_data[$index])) { - /** @var \phpbb\attachment\delete $attachment_delete */ - $attachment_delete = $phpbb_container->get('attachment.delete'); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); // delete selected attachment if ($this->attachment_data[$index]['is_orphan']) @@ -1660,11 +1660,11 @@ class parse_message extends bbcode_firstpass if ($row) { - $attachment_delete->unlink_attachment($row['physical_filename'], 'file'); + $attachment_manager->unlink($row['physical_filename'], 'file'); if ($row['thumbnail']) { - $attachment_delete->unlink_attachment($row['physical_filename'], 'thumbnail'); + $attachment_manager->unlink($row['physical_filename'], 'thumbnail'); } $db->sql_query('DELETE FROM ' . ATTACHMENTS_TABLE . ' WHERE attach_id = ' . (int) $this->attachment_data[$index]['attach_id']); @@ -1672,7 +1672,7 @@ class parse_message extends bbcode_firstpass } else { - $attachment_delete->delete('attach', $this->attachment_data[$index]['attach_id']); + $attachment_manager->delete('attach', $this->attachment_data[$index]['attach_id']); } unset($this->attachment_data[$index]); @@ -1692,9 +1692,9 @@ class parse_message extends bbcode_firstpass { if ($num_attachments < $cfg['max_attachments'] || $auth->acl_gets('m_', 'a_', $forum_id)) { - /** @var \phpbb\attachment\upload $attachment_upload */ - $attachment_upload = $phpbb_container->get('attachment.upload'); - $filedata = $attachment_upload->upload($form_name, $forum_id, false, '', $is_message); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $filedata = $attachment_manager->upload($form_name, $forum_id, false, '', $is_message); $error = array_merge($error, $filedata['error']); if (!sizeof($error)) diff --git a/phpBB/includes/ucp/ucp_attachments.php b/phpBB/includes/ucp/ucp_attachments.php index 64cad1aa90..b8cb3c4100 100644 --- a/phpBB/includes/ucp/ucp_attachments.php +++ b/phpBB/includes/ucp/ucp_attachments.php @@ -70,10 +70,10 @@ class ucp_attachments if (confirm_box(true)) { - /** @var \phpbb\attachment\delete $attachment_delete */ - $attachment_delete = $phpbb_container->get('attachment.delete'); - $attachment_delete->delete('attach', $delete_ids); - unset($attachment_delete); + /** @var \phpbb\attachment\manager $attachment_manager */ + $attachment_manager = $phpbb_container->get('attachment.manager'); + $attachment_manager->delete('attach', $delete_ids); + unset($attachment_manager); meta_refresh(3, $this->u_action); $message = ((sizeof($delete_ids) == 1) ? $user->lang['ATTACHMENT_DELETED'] : $user->lang['ATTACHMENTS_DELETED']) . '

' . sprintf($user->lang['RETURN_UCP'], '', ''); diff --git a/tests/attachment/manager_test.php b/tests/attachment/manager_test.php index 01a6049602..47d7f38b1d 100644 --- a/tests/attachment/manager_test.php +++ b/tests/attachment/manager_test.php @@ -74,8 +74,8 @@ class phpbb_attachment_manager_test extends \phpbb_test_case 'delete', ['foo', [1, 2, 3]], ['foo', [1, 2, 3], true], - true, - true, + 5, + 5, ), array( 'delete', @@ -83,8 +83,8 @@ class phpbb_attachment_manager_test extends \phpbb_test_case 'delete', ['foo', [1, 2, 3], false], ['foo', [1, 2, 3], false], - true, - true, + 2, + 2, ), array( 'resync', diff --git a/tests/content_visibility/delete_post_test.php b/tests/content_visibility/delete_post_test.php index f174b99d51..ba0a21a1a4 100644 --- a/tests/content_visibility/delete_post_test.php +++ b/tests/content_visibility/delete_post_test.php @@ -319,7 +319,8 @@ class phpbb_content_visibility_delete_post_test extends phpbb_database_test_case $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); $phpbb_container->set('content.visibility', new \phpbb\content_visibility($auth, $config, $phpbb_dispatcher, $db, $user, $phpbb_root_path, $phpEx, FORUMS_TABLE, POSTS_TABLE, TOPICS_TABLE, USERS_TABLE)); - $phpbb_container->set('attachment.delete', $attachment_delete); + // Works as a workaround for tests + $phpbb_container->set('attachment.manager', $attachment_delete); delete_post($forum_id, $topic_id, $post_id, $data, $is_soft, $reason); diff --git a/tests/functions_user/delete_user_test.php b/tests/functions_user/delete_user_test.php index 1a5a4f672e..6d2e1fa20a 100644 --- a/tests/functions_user/delete_user_test.php +++ b/tests/functions_user/delete_user_test.php @@ -36,7 +36,8 @@ class phpbb_functions_user_delete_user_test extends phpbb_database_test_case $phpbb_dispatcher = new phpbb_mock_event_dispatcher(); $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); - $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete($config, $db, new \phpbb_mock_event_dispatcher(), new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path)); + // Works as a workaround for tests + $phpbb_container->set('attachment.manager', new \phpbb\attachment\delete($config, $db, new \phpbb_mock_event_dispatcher(), new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path)); $phpbb_container->set( 'auth.provider.db', new phpbb_mock_auth_provider() diff --git a/tests/privmsgs/delete_user_pms_test.php b/tests/privmsgs/delete_user_pms_test.php index 329c401316..9d6ba7a917 100644 --- a/tests/privmsgs/delete_user_pms_test.php +++ b/tests/privmsgs/delete_user_pms_test.php @@ -91,7 +91,8 @@ class phpbb_privmsgs_delete_user_pms_test extends phpbb_database_test_case $phpbb_container = new phpbb_mock_container_builder(); $phpbb_container->set('notification_manager', new phpbb_mock_notification_manager()); - $phpbb_container->set('attachment.delete', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db, new \phpbb_mock_event_dispatcher(), new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path)); + // Works as a workaround for tests + $phpbb_container->set('attachment.manager', new \phpbb\attachment\delete(new \phpbb\config\config(array()), $db, new \phpbb_mock_event_dispatcher(), new \phpbb\filesystem\filesystem(), new \phpbb\attachment\resync($db), $phpbb_root_path)); phpbb_delete_user_pms($delete_user); From bb2634b5e5bdee53118eff8cdba3fea73932ff47 Mon Sep 17 00:00:00 2001 From: Marc Alexander Date: Mon, 12 Oct 2015 12:11:26 +0200 Subject: [PATCH 36/36] [ticket/14168] Correctly state return type of upload and upload_attachment PHPBB3-14168 --- phpBB/includes/functions_posting.php | 2 +- phpBB/phpbb/attachment/manager.php | 2 +- phpBB/phpbb/attachment/upload.php | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/phpBB/includes/functions_posting.php b/phpBB/includes/functions_posting.php index 297ac80170..2bd30a383f 100644 --- a/phpBB/includes/functions_posting.php +++ b/phpBB/includes/functions_posting.php @@ -400,7 +400,7 @@ function posting_gen_topic_types($forum_id, $cur_topic_type = POST_NORMAL) * @param bool $is_message Whether it is a PM or not * @param array $local_filedata A filespec object created for the local file * -* @return \phpbb\files\filespec +* @return array File data array */ function upload_attachment($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = false) { diff --git a/phpBB/phpbb/attachment/manager.php b/phpBB/phpbb/attachment/manager.php index 9dcd3c5c92..3c47171b2f 100644 --- a/phpBB/phpbb/attachment/manager.php +++ b/phpBB/phpbb/attachment/manager.php @@ -90,7 +90,7 @@ class manager * @param bool $is_message Whether it is a PM or not * @param array $local_filedata An file data object created for the local file * - * @return object filespec + * @return array File data array */ public function upload($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = []) { diff --git a/phpBB/phpbb/attachment/upload.php b/phpBB/phpbb/attachment/upload.php index 2d49e05b71..957558768b 100644 --- a/phpBB/phpbb/attachment/upload.php +++ b/phpBB/phpbb/attachment/upload.php @@ -104,7 +104,7 @@ class upload * @param bool $is_message Whether it is a PM or not * @param array $local_filedata An file data object created for the local file * - * @return object filespec + * @return array File data array */ public function upload($form_name, $forum_id, $local = false, $local_storage = '', $is_message = false, $local_filedata = array()) {