Merge pull request #178 from iMattPro/admin-perms

Code inspection fixes

ad/manager.php

@@ -60,7 +60,7 @@ public function get_ad($ad_id)
 		$data = $this->db->sql_fetchrow($result);
 		$this->db->sql_freeresult($result);
 
-		return $data !== false ? $data : array();
+		return $data !== false ? $data : [];
 	}
 
 	/**
@@ -183,8 +183,8 @@ public function increment_ad_clicks($ad_id)
 	/**
 	 * Insert new advertisement to the database
 	 *
-	 * @param    array $data New ad data
-	 * @return    int        New advertisement ID
+	 * @param  array $data New ad data
+	 * @return int        New advertisement ID
 	 */
 	public function insert_ad($data)
 	{
@@ -195,7 +195,7 @@ public function insert_ad($data)
 		// add a row to ads table
 		$sql = 'INSERT INTO ' . $this->ads_table . ' ' . $this->db->sql_build_array('INSERT', $data);
 		$this->db->sql_query($sql);
-		$ad_id = $this->db->sql_nextid();
+		$ad_id = (int) $this->db->sql_nextid();
 
 		$this->insert_ad_group_data($ad_id, $ad_groups);
 
@@ -212,7 +212,7 @@ public function insert_ad($data)
 	public function update_ad($ad_id, $data)
 	{
 		// extract ad groups here because it gets filtered in intersect_ad_data()
-		$ad_groups = isset($data['ad_groups']) ? $data['ad_groups'] : array();
+		$ad_groups = $data['ad_groups'] ?? [];
 		$data = $this->intersect_ad_data($data);
 
 		$sql = 'UPDATE ' . $this->ads_table . '
@@ -269,7 +269,7 @@ public function remove_ad_owner(array $user_ids)
 	 */
 	public function get_ad_locations($ad_id)
 	{
-		$ad_locations = array();
+		$ad_locations = [];
 
 		$sql = 'SELECT location_id
 			FROM ' . $this->ad_locations_table . '
@@ -293,13 +293,13 @@ public function get_ad_locations($ad_id)
 	 */
 	public function insert_ad_locations($ad_id, $ad_locations)
 	{
-		$sql_ary = array();
+		$sql_ary = [];
 		foreach ($ad_locations as $ad_location)
 		{
-			$sql_ary[] = array(
+			$sql_ary[] = [
 				'ad_id'			=> $ad_id,
 				'location_id'	=> $ad_location,
-			);
+			];
 		}
 		$this->db->sql_multi_insert($this->ad_locations_table, $sql_ary);
 	}
@@ -325,7 +325,7 @@ public function delete_ad_locations($ad_id)
 	 */
 	public function load_memberships($user_id)
 	{
-		$memberships = array();
+		$memberships = [];
 		$sql = 'SELECT group_id
 			FROM ' . USER_GROUP_TABLE . '
 			WHERE user_id = ' . (int) $user_id . '
@@ -371,7 +371,7 @@ public function load_groups($ad_id)
 	 */
 	protected function intersect_ad_data($data)
 	{
-		return array_intersect_key($data, array(
+		return array_intersect_key($data, [
 			'ad_name'			=> '',
 			'ad_note'			=> '',
 			'ad_code'			=> '',
@@ -384,7 +384,7 @@ protected function intersect_ad_data($data)
 			'ad_owner'			=> '',
 			'ad_content_only'	=> '',
 			'ad_centering'		=> '',
-		));
+		]);
 	}
 
 	/**
@@ -428,13 +428,13 @@ protected function sql_random()
 	 */
 	protected function insert_ad_group_data($ad_id, $ad_groups)
 	{
-		$sql_ary = array();
+		$sql_ary = [];
 		foreach ($ad_groups as $group)
 		{
-			$sql_ary[] = array(
+			$sql_ary[] = [
 				'ad_id'		=> $ad_id,
 				'group_id'	=> $group,
-			);
+			];
 		}
 		$this->db->sql_multi_insert($this->ad_group_table, $sql_ary);
 	}

composer.json

@@ -24,7 +24,7 @@
 		}
 	],
 	"require": {
-		"php": ">=5.4",
+		"php": ">=7.1.3",
 		"composer/installers": "~1.0"
 	},
 	"require-dev": {

controller/helper.php

@@ -82,7 +82,7 @@ public function __construct(\phpbb\user $user, \phpbb\user_loader $user_loader,
 	public function assign_data($data, $errors)
 	{
 		$this->assign_locations($data['ad_locations']);
-		$this->assign_groups((isset($data['ad_id']) ? $data['ad_id'] : 0), (isset($data['ad_groups']) ? $data['ad_groups'] : array()));
+		$this->assign_groups(($data['ad_id'] ?? 0), ($data['ad_groups'] ?? array()));
 
 		$errors = array_map(array($this->language, 'lang'), $errors);
 		$this->template->assign_vars(array(

ext.php

@@ -12,10 +12,10 @@
 
 class ext extends \phpbb\extension\base
 {
-	const DATE_FORMAT = 'Y-m-d';
-	const MAX_NAME_LENGTH = 255;
-	const DEFAULT_PRIORITY = 5;
-	const AD_BLOCK_MODES = [0, 1, 2];
+	public const DATE_FORMAT = 'Y-m-d';
+	public const MAX_NAME_LENGTH = 255;
+	public const DEFAULT_PRIORITY = 5;
+	public const AD_BLOCK_MODES = [0, 1, 2];
 
 	/**
 	 * {@inheritdoc}

tests/ad/delete_ad_locations_test.php

@@ -33,16 +33,14 @@ public function test_delete_ad_locations_no_ad()
 	{
 		$manager = $this->get_manager();
 
-		$sql = 'SELECT COUNT(ad_id) as total_ad_locations
-			FROM phpbb_ad_locations';
+		$sql = 'SELECT COUNT(ad_id) as total_ad_locations FROM phpbb_ad_locations';
+
 		$result = $this->db->sql_query($sql);
 		$total_ad_locations = $this->db->sql_fetchfield('total_ad_locations');
 		$this->db->sql_freeresult($result);
 
 		$manager->delete_ad_locations(0);
 
-		$sql = 'SELECT COUNT(ad_id) as total_ad_locations
-			FROM phpbb_ad_locations';
 		$result = $this->db->sql_query($sql);
 		self::assertEquals($this->db->sql_fetchfield('total_ad_locations'), $total_ad_locations);
 		$this->db->sql_freeresult($result);

tests/analyser/analyser_base.php

@@ -24,9 +24,6 @@ class analyser_base extends \phpbb_test_case
 	/** @var \phpbb\language\language */
 	protected $lang;
 
-	/**
-	 * {@inheritDoc}
-	 */
 	protected static function setup_extensions()
 	{
 		return array('phpbb/ads');

tests/banner/banner_base.php

@@ -24,9 +24,6 @@ class banner_base extends \phpbb_test_case
 	/** @var \PHPUnit\Framework\MockObject\MockObject|\phpbb\files\filespec */
 	protected $file;
 
-	/**
-	 * {@inheritDoc}
-	 */
 	protected static function setup_extensions()
 	{
 		return array('phpbb/ads');

tests/controller/admin_controller_test.php

@@ -672,7 +672,7 @@ public function test_action_edit_no_submit($ad_id)
 		}
 		else
 		{
-			$ad_locations = !$ad_id ? false : array(
+			$ad_locations = array(
 				'above_footer',
 				'above_header',
 			);
@@ -955,7 +955,7 @@ public function test_ad_enable($ad_id, $enable, $is_ajax, $err_msg)
 
 		$this->manager->expects(self::once())
 			->method('update_ad')
-			->willReturn($ad_id ? true : false);
+			->willReturn((bool) $ad_id);
 
 		$this->request->expects(self::once())
 			->method('is_ajax')
@@ -1015,8 +1015,8 @@ public function test_action_delete($ad_id, $ad_owner, $error, $confirm)
 		$this->request
 			->expects(self::exactly($confirm ? 2 : 4))
 			->method('variable')
-			->withConsecutive(...[['action', ''], ['id', 0], ['i', ''], ['mode', '']])
-			->willReturnOnConsecutiveCalls(...['delete', $ad_id, '', '']);
+			->withConsecutive(['action', ''], ['id', 0], ['i', ''], ['mode', ''])
+			->willReturnOnConsecutiveCalls('delete', $ad_id, '', '');
 
 		if (!$confirm)
 		{
@@ -1041,7 +1041,7 @@ public function test_action_delete($ad_id, $ad_owner, $error, $confirm)
 				->willReturn(array('id' => $ad_id, 'ad_owner' => $ad_owner, 'ad_name' => ''));
 			$this->manager->expects(self::once())
 				->method('delete_ad')
-				->willReturn($ad_id ? true : false);
+				->willReturn((bool) $ad_id);
 			$this->manager->expects(($ad_owner ? self::once() : self::never()))
 				->method('get_ads_by_owner')
 				->with($ad_owner)

tests/controller/admin_input_test.php

@@ -142,7 +142,7 @@ public function get_form_data_data()
 	 */
 	public function test_get_form_data($valid_form, $data, $ad_owner_expected, $errors)
 	{
-		list($ad_name, $ad_note, $ad_code, $ad_enabled, $ad_locations, $ad_start_date, $ad_end_date, $ad_priority, $ad_content_only, $ad_views_limit, $ad_clicks_limit, $ad_owner, $ad_groups, $ad_centering) = $data;
+		[$ad_name, $ad_note, $ad_code, $ad_enabled, $ad_locations, $ad_start_date, $ad_end_date, $ad_priority, $ad_content_only, $ad_views_limit, $ad_clicks_limit, $ad_owner, $ad_groups, $ad_centering] = $data;
 
 		self::$valid_form = $valid_form;
 		$input_controller = $this->get_input_controller();

tests/controller/helper_test.php

@@ -308,7 +308,7 @@ public function test_assign_locations($ad_locations)
 					'LOCATION_ID'   => 'top_of_page_1',
 					'LOCATION_DESC' => 'Location #1 desc',
 					'LOCATION_NAME' => 'Location #1',
-					'S_SELECTED'    => $ad_locations ? in_array('top_of_page_1', $ad_locations) : false,
+					'S_SELECTED'    => $ad_locations && in_array('top_of_page_1', $ad_locations),
 				]],
 				['ad_locations', [
 					'CATEGORY_NAME'  => 'CAT_BOTTOM_OF_PAGE',
@@ -317,7 +317,7 @@ public function test_assign_locations($ad_locations)
 					'LOCATION_ID'   => 'bottom_of_page_1',
 					'LOCATION_DESC' => 'Location #2 desc',
 					'LOCATION_NAME' => 'Location #2',
-					'S_SELECTED'    => $ad_locations ? in_array('bottom_of_page_1', $ad_locations) : false,
+					'S_SELECTED'    => $ad_locations && in_array('bottom_of_page_1', $ad_locations),
 				]]
 			);
 

tests/controller/visual_demo_test.php

@@ -131,8 +131,8 @@ public function test_controller($action, $is_ajax, $status_code, $cookie_time)
 		if (!$is_ajax)
 		{
 			// Throws E_WARNING in PHP 8.0+ and E_USER_WARNING in earlier versions
-			$exceptionName = version_compare(PHP_VERSION, '8.0', '<') ? \PHPUnit\Framework\Error\Error::class : \PHPUnit\Framework\Error\Warning::class;
-			$errno = version_compare(PHP_VERSION, '8.0', '<') ? E_USER_WARNING : E_WARNING;
+			$exceptionName = PHP_VERSION_ID < 80000 ? \PHPUnit\Framework\Error\Error::class : \PHPUnit\Framework\Error\Warning::class;
+			$errno = PHP_VERSION_ID < 80000 ? E_USER_WARNING : E_WARNING;
 			$this->expectException($exceptionName);
 			$this->expectExceptionCode($errno);
 		}

tests/event/visual_demo_test.php

@@ -37,7 +37,6 @@ public function test_visual_demo($in_visual_demo)
 		$this->user->page['page_name'] = 'viewtopic';
 
 		$this->request
-			->expects(self::any())
 			->method('is_set')
 			->withConsecutive(
 				[$this->config['cookie_name'] . '_phpbb_ads_visual_demo', \phpbb\request\request_interface::COOKIE],

tests/location/location_base.php

@@ -24,9 +24,6 @@ class location_base extends \phpbb_test_case
 	/** @var \phpbb\request\request|\PHPUnit\Framework\MockObject\MockObject */
 	protected $request;
 
-	/**
-	 * {@inheritDoc}
-	 */
 	protected static function setup_extensions()
 	{
 		return array('phpbb/ads');