From c2d95ffaeaa289752b4c7b6664b6ca112a02e350 Mon Sep 17 00:00:00 2001
From: Simon Rettberg
Date: Thu, 2 Nov 2023 19:08:58 +0100
Subject: Roundup of issues reported by PHPStorm

Mostly redundant checks, logic errors, dead code, etc.
---
 modules-available/sysconfig/addmodule.inc.php      |  2 +-
 .../sysconfig/addmodule_adauth.inc.php             |  2 --
 .../sysconfig/addmodule_branding.inc.php           | 11 ++++----
 .../sysconfig/addmodule_ldapauth.inc.php           |  2 --
 .../sysconfig/addmodule_screensaver.inc.php        | 11 ++++----
 .../sysconfig/inc/configmodule.inc.php             |  4 +--
 .../sysconfig/inc/configmodule/branding.inc.php    |  3 +--
 .../sysconfig/inc/configmodule/customodule.inc.php |  3 +--
 .../sysconfig/inc/configmodule/screensaver.inc.php |  4 +--
 modules-available/sysconfig/inc/configtgz.inc.php  | 26 +++++++++---------
 modules-available/sysconfig/inc/ppd.inc.php        | 31 +++++++++-------------
 modules-available/sysconfig/install.inc.php        |  2 +-
 12 files changed, 43 insertions(+), 58 deletions(-)

(limited to 'modules-available/sysconfig')

diff --git a/modules-available/sysconfig/addmodule.inc.php b/modules-available/sysconfig/addmodule.inc.php
index 477326b8..c887f432 100644
--- a/modules-available/sysconfig/addmodule.inc.php
+++ b/modules-available/sysconfig/addmodule.inc.php
@@ -31,7 +31,7 @@ abstract class AddModule_Base
 			Util::redirect('?do=SysConfig');
 		}
 		self::$instance = new $step();
-		if ($editId = $editId ? $editId : Request::any('edit')) {
+		if ($editId = $editId ?: Request::any('edit')) {
 			self::$instance->edit = ConfigModule::get($editId);
 			if (self::$instance->edit === false)
 				ErrorHandler::traceError('Invalid module id for editing');
diff --git a/modules-available/sysconfig/addmodule_adauth.inc.php b/modules-available/sysconfig/addmodule_adauth.inc.php
index 9f2a86f6..b7e1cca0 100644
--- a/modules-available/sysconfig/addmodule_adauth.inc.php
+++ b/modules-available/sysconfig/addmodule_adauth.inc.php
@@ -89,7 +89,6 @@ class AdAuth_CheckConnection extends AddModule_Base
 		));
 		if (!isset($this->scanTask['id'])) {
 			AddModule_Base::setStep('AdAuth_Start'); // Continues with AdAuth_Start for render()
-			return;
 		}
 	}
 
@@ -519,7 +518,6 @@ class AdAuth_Finish extends AddModule_Base
 
 		if ($this->edit === false) {
 			AddModule_Base::setStep('AddModule_Assign', $module->id());
-			return;
 		}
 
 	}
diff --git a/modules-available/sysconfig/addmodule_branding.inc.php b/modules-available/sysconfig/addmodule_branding.inc.php
index e6da9edb..f3a90e58 100644
--- a/modules-available/sysconfig/addmodule_branding.inc.php
+++ b/modules-available/sysconfig/addmodule_branding.inc.php
@@ -22,7 +22,6 @@ class Branding_ProcessFile extends AddModule_Base
 
 	private $task;
 	private $svgFile;
-	private $tarFile;
 
 	protected function preprocessInternal()
 	{
@@ -53,9 +52,9 @@ class Branding_ProcessFile extends AddModule_Base
 			Session::set('logo_name', $title);
 		}
 		chmod($this->svgFile, 0644);
-		$this->tarFile = '/tmp/bwlp-' . time() . '-' . mt_rand() . '.tgz';
+		$tarFile = '/tmp/bwlp-' . time() . '-' . mt_rand() . '.tgz';
 		$this->task = Taskmanager::submit('BrandingGenerator', array(
-				'tarFile' => $this->tarFile,
+				'tarFile' => $tarFile,
 				'svgFile' => $this->svgFile
 		));
 		$this->task = Taskmanager::waitComplete($this->task, 5000);
@@ -64,7 +63,7 @@ class Branding_ProcessFile extends AddModule_Base
 			Taskmanager::addErrorMessage($this->task);
 			Util::redirect('?do=SysConfig&action=addmodule&step=Branding_Start');
 		}
-		Session::set('logo_tgz', $this->tarFile);
+		Session::set('logo_tgz', $tarFile);
 	}
 
 	protected function renderInternal()
@@ -196,9 +195,9 @@ class Branding_Finish extends AddModule_Base
 	protected function preprocessInternal()
 	{
 		$title = Request::post('title');
-		if ($title === false || empty($title))
+		if (empty($title))
 			$title = Session::get('logo_name');
-		if ($title === false || empty($title)) {
+		if (empty($title)) {
 			Message::addError('missing-title'); // TODO: Ask for title again instead of starting over
 			Util::redirect('?do=SysConfig&action=addmodule&step=Branding_Start');
 		}
diff --git a/modules-available/sysconfig/addmodule_ldapauth.inc.php b/modules-available/sysconfig/addmodule_ldapauth.inc.php
index 88007f0c..fe5ff706 100644
--- a/modules-available/sysconfig/addmodule_ldapauth.inc.php
+++ b/modules-available/sysconfig/addmodule_ldapauth.inc.php
@@ -64,7 +64,6 @@ class LdapAuth_CheckConnection extends AddModule_Base
 		));
 		if (!isset($this->scanTask['id'])) {
 			AddModule_Base::setStep('LdapAuth_Start'); // Continues with LdapAuth_Start for render()
-			return;
 		}
 	}
 
@@ -290,7 +289,6 @@ class LdapAuth_Finish extends AddModule_Base
 
 		if ($this->edit === false) {
 			AddModule_Base::setStep('AddModule_Assign', $module->id());
-			return;
 		}
 	}
 
diff --git a/modules-available/sysconfig/addmodule_screensaver.inc.php b/modules-available/sysconfig/addmodule_screensaver.inc.php
index 9641bab3..24dfadd0 100644
--- a/modules-available/sysconfig/addmodule_screensaver.inc.php
+++ b/modules-available/sysconfig/addmodule_screensaver.inc.php
@@ -67,18 +67,17 @@ class Screensaver_Start extends AddModule_Base
 class Screensaver_Text extends AddModule_Base
 {
 	private $session_data;
-	private $id;
 
 	protected function preprocessInternal()
 	{
 		/* Load session data */
 		$this->session_data = Session::get('data');
-		$this->id = Request::post('id', '', 'string');
+		$id = Request::post('id', '', 'string');
 
-		if ($this->id === 'start') {
+		if ($id === 'start') {
 			Screensaver_Helper::processQssData($this->session_data);
-		} elseif ($this->id !== '') {
-			Screensaver_Helper::processScreensaverText($this->session_data, $this->id);
+		} elseif ($id !== '') {
+			Screensaver_Helper::processScreensaverText($this->session_data, $id);
 		}
 
 		$next = Request::post('next', $this->session_data['next'], 'string');
@@ -116,7 +115,7 @@ class Screensaver_Text extends AddModule_Base
 		 * Dictionary::translate('saver_DescriptionShutdown');
 		 */
 		$data['title'] = Dictionary::translate('saver_Title' . $tag, true);
-		$data['description'] = Dictionary::translate('saver_Description' . $tag, true);;
+		$data['description'] = Dictionary::translate('saver_Description' . $tag, true);
 		$data['msg_value'] = $this->session_data['messages']['General'][$next];
 		$data['msg_locked_value'] = $this->session_data['messages']['General'][$next . '-locked'];
 		$data['text_value'] = $this->session_data['texts']['text-' . $next];
diff --git a/modules-available/sysconfig/inc/configmodule.inc.php b/modules-available/sysconfig/inc/configmodule.inc.php
index 3b877d10..e414a6cf 100644
--- a/modules-available/sysconfig/inc/configmodule.inc.php
+++ b/modules-available/sysconfig/inc/configmodule.inc.php
@@ -470,11 +470,11 @@ abstract class ConfigModule
 			$status = 'MISSING';
 		else
 			$status = 'OUTDATED';
-		return Database::exec("UPDATE configtgz_module SET filepath = :filename, status = :status WHERE moduleid = :id LIMIT 1", array(
+		Database::exec("UPDATE configtgz_module SET filepath = :filename, status = :status WHERE moduleid = :id LIMIT 1", array(
 				'id' => $this->moduleId,
 				'filename' => $this->moduleArchive,
 				'status' => $status
-			)) !== false;
+			));
 	}
 
 	public function dateline_s()
diff --git a/modules-available/sysconfig/inc/configmodule/branding.inc.php b/modules-available/sysconfig/inc/configmodule/branding.inc.php
index 8990dbec..76968ff4 100644
--- a/modules-available/sysconfig/inc/configmodule/branding.inc.php
+++ b/modules-available/sysconfig/inc/configmodule/branding.inc.php
@@ -22,13 +22,12 @@ class ConfigModule_Branding extends ConfigModule
 		if (!$this->validateConfig()) {
 			return $this->archive() !== false && file_exists($this->archive()); // No new temp file given, old archive still exists, pretend it worked...
 		}
-		$task = Taskmanager::submit('MoveFile', array(
+		return Taskmanager::submit('MoveFile', array(
 			'source' => $this->tmpFile,
 			'destination' => $tgz,
 			'parentTask' => $parent,
 			'failOnParentFail' => false
 		));
-		return $task;
 	}
 
 	protected function moduleVersion()
diff --git a/modules-available/sysconfig/inc/configmodule/customodule.inc.php b/modules-available/sysconfig/inc/configmodule/customodule.inc.php
index 8b968336..56aa9277 100644
--- a/modules-available/sysconfig/inc/configmodule/customodule.inc.php
+++ b/modules-available/sysconfig/inc/configmodule/customodule.inc.php
@@ -34,13 +34,12 @@ class ConfigModule_CustomModule extends ConfigModule
 			// Nothing to do
 			return true;
 		}
-		$task = Taskmanager::submit('MoveFile', array(
+		return Taskmanager::submit('MoveFile', array(
 			'source' => $this->tmpFile,
 			'destination' => $tgz,
 			'parentTask' => $parent,
 			'failOnParentFail' => false
 		));
-		return $task;
 	}
 
 	protected function moduleVersion()
diff --git a/modules-available/sysconfig/inc/configmodule/screensaver.inc.php b/modules-available/sysconfig/inc/configmodule/screensaver.inc.php
index ed97941e..08a2c9a1 100644
--- a/modules-available/sysconfig/inc/configmodule/screensaver.inc.php
+++ b/modules-available/sysconfig/inc/configmodule/screensaver.inc.php
@@ -23,13 +23,11 @@ class ConfigModule_Screensaver extends ConfigModule
 		/* Give the Taskmanager the job and create the tgz */
 		$taskId = 'xscreensaver' . mt_rand() . '-' . microtime(true);
 
-		$task = Taskmanager::submit('MakeTarball', array(
+		return Taskmanager::submit('MakeTarball', array(
 			'id' => $taskId,
 			'files' => $this->getFileArray(),
 			'destination' => $tgz,
 		), false);
-
-		return $task;
 	}
 
 	protected function moduleVersion()
diff --git a/modules-available/sysconfig/inc/configtgz.inc.php b/modules-available/sysconfig/inc/configtgz.inc.php
index 67e02503..9ab3ffd3 100644
--- a/modules-available/sysconfig/inc/configtgz.inc.php
+++ b/modules-available/sysconfig/inc/configtgz.inc.php
@@ -10,7 +10,6 @@ class ConfigTgz
 
 	private function __construct()
 	{
-		;
 	}
 
 	public function id()
@@ -134,7 +133,7 @@ class ConfigTgz
 		return $task['id'];
 	}
 
-	public function delete()
+	public function delete(): bool
 	{
 		if ($this->configId === 0)
 			ErrorHandler::traceError('ConfigTgz::delete called with invalid config id!');
@@ -150,14 +149,14 @@ class ConfigTgz
 		return $ret !== false;
 	}
 	
-	public function markOutdated()
+	public function markOutdated(): void
 	{
 		if ($this->configId === 0)
 			ErrorHandler::traceError('ConfigTgz::markOutdated called with invalid config id!');
-		return $this->mark('OUTDATED');
+		$this->mark('OUTDATED');
 	}
 	
-	private function markUpdated($task)
+	private function markUpdated($task): void
 	{
 		if ($this->configId === 0)
 			ErrorHandler::traceError('ConfigTgz::markUpdated called with invalid config id!');
@@ -186,27 +185,28 @@ class ConfigTgz
 				'status' => 'OK',
 				'warnings' => $warnings,
 			]);
-			return 'OK';
+			return;
 		}
-		return $this->mark('OUTDATED');
+		$this->mark('OUTDATED');
 	}
 	
-	private function markFailed()
+	private function markFailed(): void
 	{
 		if ($this->configId === 0)
 			ErrorHandler::traceError('ConfigTgz::markFailed called with invalid config id!');
-		if ($this->file === false || !file_exists($this->file))
-			return $this->mark('MISSING');
-		return $this->mark('OUTDATED');
+		if ($this->file === false || !file_exists($this->file)) {
+			$this->mark('MISSING');
+		} else {
+			$this->mark('OUTDATED');
+		}
 	}
 	
-	private function mark($status)
+	private function mark($status): void
 	{
 		Database::exec("UPDATE configtgz SET status = :status WHERE configid = :configid LIMIT 1", [
 			'configid' => $this->configId,
 			'status' => $status,
 		]);
-		return $status;
 	}
 
 	/*
diff --git a/modules-available/sysconfig/inc/ppd.inc.php b/modules-available/sysconfig/inc/ppd.inc.php
index 8e125ed7..75608c56 100644
--- a/modules-available/sysconfig/inc/ppd.inc.php
+++ b/modules-available/sysconfig/inc/ppd.inc.php
@@ -308,8 +308,8 @@ class Ppd
 		$this->requiredKeywords = array();
 
 		// Parse
-		/* @var $rawOption \PpdOption */
-		/* @var $currentBlock \PpdBlockInternal */
+		/* @var PpdOption $rawOption */
+		/* @var PpdBlockInternal $currentBlock */
 		$currentBlock = false;
 		$inRawBlock = false; // True if in a multi-line InvocationValue or QuotedValue (3.6: Parsing Summary for Values)
 		$wantsEnd = false;
@@ -425,10 +425,7 @@ class Ppd
 						return;
 					}
 					// TODO: Check unique
-					$nb = new PpdBlockInternal($value, $valueTranslation, 'Group', $currentBlock, $lStart);
-					if ($currentBlock !== false) {
-						$currentBlock->childBlocks[] = $nb;
-					}
+					$nb = new PpdBlockInternal($value, $valueTranslation, 'Group', false, $lStart);
 					$currentBlock = $nb;
 					continue;
 				} elseif ($mainKeyword === 'OpenSubGroup') {
@@ -438,9 +435,7 @@ class Ppd
 					}
 					// TODO: Check unique
 					$nb = new PpdBlockInternal($value, $valueTranslation, 'SubGroup', $currentBlock, $lStart);
-					if ($currentBlock !== false) {
-						$currentBlock->childBlocks[] = $nb;
-					}
+					$currentBlock->childBlocks[] = $nb;
 					$currentBlock = $nb;
 					continue;
 				} elseif ($mainKeyword === 'OpenUI' || $mainKeyword === 'JCLOpenUI') {
@@ -542,7 +537,8 @@ class Ppd
 					$option = $this->getOption(substr($mainKeyword, 7), $currentBlock);
 					$option->default = new PpdOption($lStart, $len, $value, $valueTranslation);
 					continue;
-				} elseif (substr($mainKeyword, 0, 17) === 'FoomaticRIPOption') {
+				}
+				if (substr($mainKeyword, 0, 17) === 'FoomaticRIPOption') {
 					if ($optionKeyword === false) {
 						$this->warn($no, "$mainKeyword with no option keyword");
 					} elseif ($currentBlock !== false && isset($this->settings[$optionKeyword])) {
@@ -604,7 +600,7 @@ class Ppd
 			} elseif (strlen(trim($line)) !== 0) {
 				$this->warn($no, 'Invalid format; not empty and not starting with asterisk (*)');
 			}
-		}
+		} // end while loop over ppd contents
 		//
 		if ($currentBlock !== false) {
 			$this->error = 'Block ' . $currentBlock->id . ' (' . $currentBlock->type . ') was never closed.';
@@ -691,10 +687,9 @@ class Ppd
 
 	private function escapeBinaryArray($array)
 	{
-		$chars = array_reduce(array_unique($array), function ($carry, $item) {
+		return array_reduce(array_unique($array), function ($carry, $item) {
 			return $carry . '\x' . dechex(ord($item));
 		}, '');
-		return $chars;
 	}
 
 	private function unhexTranslation($lineNo, $translation)
@@ -957,7 +952,7 @@ class Ppd
 				// $start must lie before range start, otherwise we'd have hit the case above
 				$end = $ranges[$key][1];
 				unset($ranges[$key]);
-				continue;
+				//continue;
 			}
 		}
 		$ranges[] = array($start, $end);
@@ -1107,11 +1102,11 @@ class PpdBlockInternal
 	public $translation;
 	public $type;
 	/**
-	 * @var \PpdBlockInternal[]
+	 * @var PpdBlockInternal[]
 	 */
 	public $childBlocks = array();
 	/**
-	 * @var \PpdBlockInternal
+	 * @var PpdBlockInternal|false
 	 */
 	public $parent;
 
@@ -1148,10 +1143,10 @@ class PpdBlockInternal
 	}
 
 	/**
-	 * @param \PpdBlockInternal $block some other PpdBlock instance
+	 * @param PpdBlockInternal $block some other PpdBlock instance
 	 * @return bool true if this is a child of $block
 	 */
-	public function isChildOf($block)
+	public function isChildOf(PpdBlockInternal $block)
 	{
 		$parent = $this->parent;
 		while ($parent !== false) {
diff --git a/modules-available/sysconfig/install.inc.php b/modules-available/sysconfig/install.inc.php
index 6c9edba2..9d32137c 100644
--- a/modules-available/sysconfig/install.inc.php
+++ b/modules-available/sysconfig/install.inc.php
@@ -132,7 +132,7 @@ if ($list === false) {
 		if ($ad->moduleType() === 'SshConfig') {
 			// 2020-11-12: Split SshConfig into SshConfig and SshKey
 			$pubkey = $ad->getData('publicKey');
-			if ($pubkey !== false && !empty($pubkey)) {
+			if (!empty($pubkey)) {
 				error_log('Legacy module with pubkey ' . $ad->id());
 				$key = ConfigModule::getInstance('SshKey');
 				if ($key !== false) {
-- 
cgit v1.2.3-55-g7522