Merge pull request #290 from marc1706/ticket/289

[ticket/289] Fix some scrutinizer issues

acp/portal_module.php

@@ -15,7 +15,7 @@ class portal_module
 	public $new_config = array();
 	protected $c_class;
 	protected $db, $user, $cache, $template, $display_vars, $config, $phpbb_root_path, $phpbb_admin_path, $phpEx, $phpbb_container;
-	protected $root_path, $mod_version_check, $request;
+	protected $root_path, $version_check, $request, $php_ext;
 	public $module_column = array();
 
 	/** @var \phpbb\di\service_collection Portal modules */
@@ -40,21 +40,21 @@ class portal_module
 		$this->request = $request;
 		$this->phpbb_root_path = $phpbb_root_path;
 		$this->phpbb_admin_path = $phpbb_admin_path;
-		$this->php_ex = $phpEx;
+		$this->php_ext = $phpEx;
 		$this->phpbb_container = $phpbb_container;
-		$this->mod_version_check = $this->phpbb_container->get('board3.version.check');
+		$this->version_check = $this->phpbb_container->get('board3.version.check');
 		$this->register_modules($this->phpbb_container->get('board3.module_collection'));
 		define('PORTAL_MODULES_TABLE', $this->phpbb_container->getParameter('board3.modules.table'));
 		define('PORTAL_CONFIG_TABLE', $this->phpbb_container->getParameter('board3.config.table'));
 
 		if (!function_exists('column_string_const'))
 		{
-			include($this->root_path . 'includes/functions_modules.' . $this->php_ex);
+			include($this->root_path . 'includes/functions_modules.' . $this->php_ext);
 		}
 
 		if(!function_exists('obtain_portal_config'))
 		{
-			include($this->root_path . 'includes/functions.' . $this->php_ex);
+			include($this->root_path . 'includes/functions.' . $this->php_ext);
 		}
 	}
 
@@ -108,7 +108,7 @@ class portal_module
 							$class = 'portal_' . $module_data['module_classname'] . '_module';
 							if (!class_exists($class))
 							{
-								include($this->root_path . 'portal/modules/portal_' . $module_data['module_classname'] . '.' . $this->php_ex);
+								include($this->root_path . 'portal/modules/portal_' . $module_data['module_classname'] . '.' . $this->php_ext);
 							}
 							if (class_exists($class))
 							{
@@ -166,7 +166,7 @@ class portal_module
 				else
 				{
 					// only show the mod version check if we are on the General Settings page
-					$this->mod_version_check->version_check();
+					$this->version_check->version_check();
 				}
 
 				$this->new_config = $this->config;
@@ -189,7 +189,7 @@ class portal_module
 				// Reset module
 				$reset_module = $this->request->variable('module_reset', 0);
 
-				if($reset_module)
+				if($reset_module && !empty($module_data))
 				{
 					$this->reset_module($id, $mode, $module_id, $module_data);
 				}
@@ -235,7 +235,6 @@ class portal_module
 				{
 					$module_permission = $this->request->variable('permission-setting', array(0 => ''));
 					$groups_ary = array();
-					$img_error = '';
 
 					// get groups and check if the selected groups actually exist
 					$sql = 'SELECT group_id
@@ -277,7 +276,7 @@ class portal_module
 
 					if(isset($module_name))
 					{
-						if ($module_data['module_classname'] !== '\board3\portal\modules\custom')
+						if (isset($module_data) && $module_data['module_classname'] !== '\board3\portal\modules\custom')
 						{
 							add_log('admin', 'LOG_PORTAL_CONFIG', $module_name);
 						}
@@ -286,7 +285,7 @@ class portal_module
 					{
 						add_log('admin', 'LOG_PORTAL_CONFIG', $this->user->lang['ACP_PORTAL_' . strtoupper($mode) . '_INFO']);
 					}
-					trigger_error($this->user->lang['CONFIG_UPDATED'] . ((!empty($img_error) ? '<br /><br />' . $this->user->lang['MODULE_IMAGE_ERROR'] . '<br />' . $img_error : '')) . adm_back_link(($module_id) ? append_sid("{$this->phpbb_root_path}adm/index.{$this->php_ex}", 'i=\board3\portal\acp\portal_module&amp;mode=modules') : $this->u_action));
+					trigger_error($this->user->lang['CONFIG_UPDATED'] . ((!empty($img_error) ? '<br /><br />' . $this->user->lang['MODULE_IMAGE_ERROR'] . '<br />' . $img_error : '')) . adm_back_link(($module_id) ? append_sid("{$this->phpbb_root_path}adm/index.{$this->php_ext}", 'i=\board3\portal\acp\portal_module&amp;mode=modules') : $this->u_action));
 				}
 
 				// show custom HTML files on the settings page of the modules instead of the standard board3 portal one, if chosen by module
@@ -407,7 +406,8 @@ class portal_module
 					$this->module_delete($id, $mode, $action, $module_id);
 				}
 
-				$add_module = key($this->request->variable('add', array('' => '')));
+				$add_list = $this->request->variable('add', array('' => ''));
+				$add_module = key($add_list);
 				$add_column = $this->request->variable('add_column', column_string_num($add_module));
 				if ($add_column)
 				{
@@ -472,13 +472,14 @@ class portal_module
 						$this->cache->purge(); // make sure we don't get errors after re-adding a module
 
 						// if something went wrong, handle the errors accordingly and undo the above query
-						if (sizeof($error) && $error != true)
+						if (!empty($error) && $error != true)
 						{
 							if (is_array($error))
 							{
+								$error_output = '';
 								foreach($error as $cur_error)
 								{
-									$error_output = $cur_error . '<br />';
+									$error_output .= $cur_error . '<br />';
 								}
 							}
 							else
@@ -487,6 +488,7 @@ class portal_module
 							}
 
 							$sql = 'DELETE FROM ' . PORTAL_MODULES_TABLE . ' WHERE module_id = ' . (int) $module_id;
+							$this->db->sql_query($sql);
 
 							trigger_error($error_output . adm_back_link($this->u_action));
 						}
@@ -497,7 +499,7 @@ class portal_module
 					}
 
 					$this->template->assign_var('S_EDIT', true);
-					$fileinfo = array();
+					$fileinfo = $name_ary = array();
 					$column_string = column_num_string($add_column);
 
 					// Find new modules
@@ -693,11 +695,11 @@ class portal_module
 					{
 						trigger_error($this->user->lang['FORM_INVALID'] . adm_back_link($this->u_action), E_USER_WARNING);
 					}
-					include($this->root_path . 'includes/functions_upload.' . $this->php_ex);
+					include($this->root_path . 'includes/functions_upload.' . $this->php_ext);
 					// Default upload path is portal/upload/
 					$upload_path = $this->root_path . 'portal/upload/';
 
-					$portal_upload = new portal_upload($upload_path, $this->u_action);
+					new portal_upload($upload_path, $this->u_action);
 
 					$this->tpl_name = 'portal/acp_portal_upload_module';
 					$this->page_title = $this->user->lang['ACP_PORTAL_UPLOAD'];
@@ -781,7 +783,7 @@ class portal_module
 
 			$this->cache->destroy('config');
 			$this->cache->destroy('portal_config');
-			$portal_config = obtain_portal_config(); // we need to prevent duplicate entry errors
+			obtain_portal_config(); // we need to prevent duplicate entry errors
 			$this->c_class->install($module_id);
 			$this->cache->purge();
 
@@ -943,6 +945,7 @@ class portal_module
 		}
 
 		$this->c_class = $this->modules[$module_data['module_classname']];
+		$move_action = 0;
 
 		if ($module_data !== false && $module_data['module_column'] > column_string_num('left'))
 		{
@@ -1038,6 +1041,7 @@ class portal_module
 		}
 
 		$this->c_class = $this->modules[$module_data['module_classname']];
+		$move_action = 0;
 
 		if ($module_data !== false && $module_data['module_column'] < column_string_num('right'))
 		{

config/services.yml

@@ -30,7 +30,7 @@ services:
             - %board3.modules.table%
 
     board3.version.check:
-        class: board3\portal\includes\mod_version_check
+        class: board3\portal\includes\version_check
         arguments:
             - %board3.version_data%
             - @config

includes/functions.php

@@ -555,7 +555,7 @@ function get_portal_tracking_info($fetch_news)
 {
 	global $config, $request, $user;
 
-	$last_read = $topic_ids = $forum_ids = $tracking_info = $rev_forum_ids = array();
+	$last_read = $topic_ids = $forum_ids = $tracking_info = $rev_forum_ids = $user_lastmark = array();
 
 	/**
 	* group everything by the forum IDs
@@ -614,7 +614,10 @@ function get_portal_tracking_info($fetch_news)
 				// @todo: also check if $user_lastmark has been defined for this specific forum_id
 				foreach ($topic_ids as $topic_id)
 				{
-					$last_read[$topic_id] = (!isset($last_read[$topic_id]) || $user_lastmark[$rev_forum_ids[$topic_id]] > $last_read[$topic_id]) ? $user_lastmark[$rev_forum_ids[$topic_id]] : $last_read[$topic_id];
+					if (!isset($last_read[$topic_id]) || (isset($user_lastmark[$rev_forum_ids[$topic_id]]) && $user_lastmark[$rev_forum_ids[$topic_id]] > $last_read[$topic_id]))
+					{
+						$last_read[$topic_id] =  $user_lastmark[$rev_forum_ids[$topic_id]];
+					}
 				}
 			}
 		}
@@ -628,11 +631,11 @@ function get_portal_tracking_info($fetch_news)
 				{
 					if (STRIP)
 					{
-						$tracking_topics = stripslashes($this->request->variable($config['cookie_name'] . '_track', '', true, \phpbb\request\request_interface::COOKIE));
+						$tracking_topics = stripslashes($request->variable($config['cookie_name'] . '_track', '', true, \phpbb\request\request_interface::COOKIE));
 					}
 					else
 					{
-						$tracking_topics = $this->request->variable($config['cookie_name'] . '_track', '', true, \phpbb\request\request_interface::COOKIE);
+						$tracking_topics = $request->variable($config['cookie_name'] . '_track', '', true, \phpbb\request\request_interface::COOKIE);
 					}
 				}
 				else
@@ -686,7 +689,14 @@ function get_portal_tracking_info($fetch_news)
 }
 
 /**
-* check if the entered source file actually exists
+* Check if the entered source file actually exists
+*
+* @param string	$value		Filename of file to check
+* @param string	$key		Key of the acp setting (unused here)
+* @param int	$module_id	Module ID of this module
+* @param bool	$force_error	Whether an error message should be triggered on
+*				errors.
+* @return bool|string False if file exists, an error string if file doesn't exist.
 */
 function check_file_src($value, $key, $module_id, $force_error = true)
 {

includes/version_check.php

@@ -9,10 +9,10 @@
 
 namespace board3\portal\includes;
 
-class mod_version_check
+class version_check
 {
 	/**
-	* @var version_data
+	* @var array version_data
 	*/
 	protected $version_data;
 
@@ -22,12 +22,12 @@ class mod_version_check
 	protected $config;
 
 	/**
-	* @var phpbb_root_path
+	* @var string phpbb_root_path
 	*/
 	protected $phpbb_root_path;
 
 	/**
-	* @var phpEx
+	* @var string PHP file extension
 	*/
 	protected $php_ext;
 
@@ -42,7 +42,7 @@ class mod_version_check
 	protected $user;
 
 	/**
-	* Construct a mod_version_check object
+	* Construct a version_check object
 	*
 	* @param array $version_data Version data
 	* @param \phpbb\config\config $config phpBB config
@@ -74,23 +74,74 @@ class mod_version_check
 			include($this->phpbb_root_path . 'includes/functions_admin.' . $this->php_ext);
 		}
 
-		$var = $this->version_data;
+		// Fill with bogus data
+		$this->get_empty_data($mod_version, $data);
 
+		// Get version info from server
+		$this->get_version_info($mod_version, $data);
+
+		// remove spaces from the version in the mod file stored locally
+		$version = $this->config[str_replace(' ', '', $this->version_data['version'])];
+		if ($return_version)
+		{
+			return $version;
+		}
+
+		$version_compare = (version_compare($version, $mod_version, '<')) ? false : true;
+
+		$this->template->assign_block_vars('mods', array(
+			'ANNOUNCEMENT'		=> (string) $data['announcement'],
+			'AUTHOR'			=> $this->version_data['author'],
+			'CURRENT_VERSION'	=> $version,
+			'DESCRIPTION'		=> (string) $data['description'],
+			'DOWNLOAD'			=> (string) $data['download'],
+			'LATEST_VERSION'	=> $mod_version,
+			'TITLE'				=> (string) $data['title'],
+
+			'UP_TO_DATE'		=> sprintf((!$version_compare) ? $this->user->lang['NOT_UP_TO_DATE'] : $this->user->lang['UP_TO_DATE'], $data['title']),
+
+			'S_UP_TO_DATE'		=> $version_compare,
+
+			'U_AUTHOR'			=> 'http://www.phpbb.com/community/memberlist.php?mode=viewprofile&un=' . $this->version_data['author'],
+		));
+	}
+
+	/**
+	* Fill variables with empty bogus data
+	*
+	* @param string	$mod_version Mod version
+	* @param array	$data Array containing mod info
+	*
+	* @return null
+	*/
+	protected function get_empty_data(&$mod_version, &$data)
+	{
+		// Fill with bogus data
+		$mod_version = $this->user->lang['NO_INFO'];
+		$data = array(
+			'title'			=> $this->version_data['title'],
+			'description'	=> $this->user->lang['NO_INFO'],
+			'download'		=> $this->user->lang['NO_INFO'],
+			'announcement'	=> $this->user->lang['NO_INFO'],
+		);
+	}
+
+	/**
+	* Get version info from remote server
+	*
+	* @param string	$mod_version Mod version
+	* @param array	$data Array containing mod info
+	*
+	* @return null
+	*/
+	protected function get_version_info(&$mod_version, &$data)
+	{
 		// Get current and latest version
 		$errstr = '';
 		$errno = 0;
+		$var = $this->version_data;
 
-		if (!$return_version)
-		{
-			$mod_version = $this->user->lang['NO_INFO'];
-			$data = array(
-				'title'			=> $var['title'],
-				'description'	=> $this->user->lang['NO_INFO'],
-				'download'		=> $this->user->lang['NO_INFO'],
-				'announcement'	=> $this->user->lang['NO_INFO'],
-			);
-		}
-		$file = get_remote_file($var['file'][0], '/' . $var['file'][1], $var['file'][2], $errstr, $errno);
+		$file = get_remote_file($this->version_data['file'][0], '/' . $this->version_data['file'][1], $this->version_data['file'][2], $errstr, $errno);
 
 		if ($file)
 		{
@@ -110,30 +161,5 @@ class mod_version_check
 				);
 			}
 		}
-
-		// remove spaces from the version in the mod file stored locally
-		$version = $this->config[str_replace(' ', '', $var['version'])];
-		if ($return_version)
-		{
-			return $version;
-		}
-
-		$version_compare = (version_compare($version, $mod_version, '<')) ? false : true;
-
-		$this->template->assign_block_vars('mods', array(
-			'ANNOUNCEMENT'		=> (string) $data['announcement'],
-			'AUTHOR'			=> $var['author'],
-			'CURRENT_VERSION'	=> $version,
-			'DESCRIPTION'		=> (string) $data['description'],
-			'DOWNLOAD'			=> (string) $data['download'],
-			'LATEST_VERSION'	=> $mod_version,
-			'TITLE'				=> (string) $data['title'],
-
-			'UP_TO_DATE'		=> sprintf((!$version_compare) ? $this->user->lang['NOT_UP_TO_DATE'] : $this->user->lang['UP_TO_DATE'], $data['title']),
-
-			'S_UP_TO_DATE'		=> $version_compare,
-
-			'U_AUTHOR'			=> 'http://www.phpbb.com/community/memberlist.php?mode=viewprofile&un=' . $var['author'],
-		));
 	}
 }

modules/announcements.php

@@ -62,10 +62,10 @@ class announcements extends module_base
 	/** @var \phpbb\request\request */
 	protected $request;
 
-	/** @var php file extension */
+	/** @var string php file extension */
 	protected $php_ext;
 
-	/** @var phpbb root path */
+	/** @var string phpbb root path */
 	protected $phpbb_root_path;
 
 	/** @var \phpbb\user */

tests/unit/acp/move_module_test.php

@@ -26,7 +26,7 @@ class phpbb_acp_move_module_test extends \board3\portal\tests\testframework\data
 		$phpbb_container = new \phpbb_mock_container_builder();
 		// Mock version check
 		$phpbb_container->set('board3.version.check',
-			$this->getMockBuilder('\board3\portal\includes\mod_version_check')
+			$this->getMockBuilder('\board3\portal\includes\version_check')
 				->disableOriginalConstructor()
 				->getMock());
 		// Mock module service collection

tests/unit/functions/version_check_test.php

@@ -33,7 +33,7 @@ class phpbb_functions_version_check_test extends \board3\portal\tests\testframew
 			'NOT_UP_TO_DATE'	=> 'NOT_UP_TO_DATE',
 			'UP_TO_DATE'		=> 'UP_TO_DATE',
 		));
-		$this->version_check = new \board3\portal\includes\mod_version_check($version_data, $config, $phpbb_root_path, $phpEx, $this->template, $user);
+		$this->version_check = new \board3\portal\includes\version_check($version_data, $config, $phpbb_root_path, $phpEx, $this->template, $user);
 	}
 
 	public function test_version_check()