Add function param/return types, fix a lot more phpstorm complaints

5 files changed, 79 insertions, 84 deletions

diff --git a/modules-available/roomplanner/inc/composedroom.inc.php b/modules-available/roomplanner/inc/composedroom.inc.php
index cdd50984..3ee892db 100644
--- a/modules-available/roomplanner/inc/composedroom.inc.php
+++ b/modules-available/roomplanner/inc/composedroom.inc.php
@@ -98,7 +98,7 @@ class ComposedRoom extends Room
 		return $this->orientation;
 	}
 
-	public function subLocationIds()
+	public function subLocationIds(): array
 	{
 		return $this->list;
 	}
@@ -108,7 +108,7 @@ class ComposedRoom extends Room
 		return $this->controlRoom;
 	}
 
-	public function machineCount()
+	public function machineCount(): int
 	{
 		$sum = 0;
 		foreach ($this->list as $lid) {
@@ -117,10 +117,9 @@ class ComposedRoom extends Room
 		return $sum;
 	}
 
-	public function getSize(&$width, &$height)
+	public function getSize(?int &$width, ?int &$height)
 	{
 		$horz = ($this->orientation == 'horizontal');
-		$width = $height = 0;
 		foreach ($this->list as $locId) {
 			self::$rooms[$locId]->getSize($w, $h);
 			$width = $horz ? $width + $w : max($width, $w);
@@ -128,7 +127,7 @@ class ComposedRoom extends Room
 		}
 	}
 
-	public function getIniClientSection(&$i, $offX = 0, $offY = 0)
+	public function getIniClientSection(int &$i, int $offX = 0, int $offY = 0)
 	{
 		if (!$this->enabled)
 			return false;
@@ -154,10 +153,10 @@ class ComposedRoom extends Room
 		return $out;
 	}
 
-	public function getShiftedArray($offX = 0, $offY = 0)
+	public function getShiftedArray(int $offX = 0, int $offY = 0): ?array
 	{
 		if (!$this->enabled)
-			return false;
+			return null;
 		if ($this->orientation == 'horizontal') {
 			$x = 1;
 			$y = 0;
@@ -168,7 +167,7 @@ class ComposedRoom extends Room
 		$ret = [];
 		foreach ($this->list as $locId) {
 			$new = self::$rooms[$locId]->getShiftedArray($offX, $offY);
-			if ($new !== false) {
+			if ($new !== null) {
 				$ret = array_merge($ret, $new);
 				self::$rooms[$locId]->getSize($w, $h);
 				$offX += $w * $x;
@@ -176,7 +175,7 @@ class ComposedRoom extends Room
 			}
 		}
 		if (empty($ret))
-			return false;
+			return null;
 		return $ret;
 	}
 
@@ -194,12 +193,12 @@ class ComposedRoom extends Room
 		return false;
 	}
 
-	public function isLeaf()
+	public function isLeaf(): bool
 	{
 		return false;
 	}
 
-	public function shouldSkip()
+	public function shouldSkip(): bool
 	{
 		return !$this->enabled;
 	}
diff --git a/modules-available/roomplanner/inc/pvsgenerator.inc.php b/modules-available/roomplanner/inc/pvsgenerator.inc.php
index f23dcb20..f3c5c838 100644
--- a/modules-available/roomplanner/inc/pvsgenerator.inc.php
+++ b/modules-available/roomplanner/inc/pvsgenerator.inc.php
@@ -3,7 +3,7 @@
 class PvsGenerator
 {
 
-	public static function generate()
+	public static function generate(): string
 	{
 		/* collect names and build room blocks - filter empty rooms while at it */
 		$roomNames = array();
@@ -36,7 +36,7 @@ class PvsGenerator
 	 * @param Room $room room/location data as fetched from db
 	 * @return string|false .ini section for room, or false if room is empty
 	 */
-	private static function generateRoomBlock($room)
+	private static function generateRoomBlock(Room $room)
 	{
 		$room->getSize($sizeX, $sizeY);
 		if ($sizeX === 0 || $sizeY === 0)
@@ -91,13 +91,13 @@ class PvsGenerator
 		}
 		// Load room
 		$room = Room::get($locationId);
-		if ($room === false)
+		if ($room === null)
 			return false;
 		$room->getSize($sizeX, $sizeY);
 		if ($sizeX === 0 || $sizeY === 0)
 			return false; // Empty
 
-		$machines = $room->getShiftedArray();
+		$machines = $room->getShiftedArray() ?? [];
 		$ORIENTATION = ['north' => 2, 'east' => 3, 'south' => 0, 'west' => 1];
 		if (is_string($highlightUuid)) {
 			$highlightUuid = strtoupper($highlightUuid);
@@ -176,14 +176,12 @@ class PvsGenerator
 	/**
 	 * Get display name for manager of given locationId.
 	 * Hook for "runmode" module to resolve mode name.
-	 * @param $locationId
-	 * @return bool|string
 	 */
-	public static function getManagerName($locationId)
+	public static function getManagerName(int $locationId): ?string
 	{
 		$names = Location::getNameChain($locationId);
 		if ($names === false)
-			return false;
+			return null;
 		return implode(' / ', $names);
 	}
 
diff --git a/modules-available/roomplanner/inc/room.inc.php b/modules-available/roomplanner/inc/room.inc.php
index 1a7a80ae..880cb6d0 100644
--- a/modules-available/roomplanner/inc/room.inc.php
+++ b/modules-available/roomplanner/inc/room.inc.php
@@ -29,10 +29,10 @@ abstract class Room
 					FROM location_roomplan lr
 					LEFT JOIN machine m ON (lr.tutoruuid = m.machineuuid)');
 		foreach ($ret as $row) {
-			$row = self::loadSingleRoom($row);
-			if ($row === false)
+			$room = self::loadSingleRoom($row);
+			if ($room === null)
 				continue;
-			self::$rooms[$row->locationId] = $row;
+			self::$rooms[$room->locationId] = $room;
 		}
 		foreach (self::$rooms as $room) {
 			$room->sanitize();
@@ -42,14 +42,14 @@ abstract class Room
 	/**
 	 * Instantiate ComposedRoom or MachineGroup depending on contents of $row
 	 * @param array $row DB row from location_roomplan.
-	 * @return Room|false Room instance, false on error
+	 * @return ?Room Room instance, null on error
 	 */
-	private static function loadSingleRoom($row)
+	private static function loadSingleRoom(array $row): ?Room
 	{
 		$locations = Location::getLocationsAssoc();
 		settype($row['locationid'], 'int');
 		if (!isset($locations[$row['locationid']]))
-			return false;
+			return null;
 		if ($locations[$row['locationid']]['isleaf'])
 			return new SimpleRoom($row);
 		return new ComposedRoom($row, false);
@@ -59,7 +59,7 @@ abstract class Room
 	 * Get array of all rooms with room plan
 	 * @return Room[]
 	 */
-	public static function getAll()
+	public static function getAll(): array
 	{
 		self::init();
 		return self::$rooms;
@@ -68,9 +68,9 @@ abstract class Room
 	/**
 	 * Get room instance for given location
 	 * @param int $locationId room to get
-	 * @return Room|false requested room, false if not configured or not found
+	 * @return ?Room requested room, false if not configured or not found
 	 */
-	public static function get($locationId)
+	public static function get(int $locationId): ?Room
 	{
 		if (self::$rooms === null) {
 			$room = Database::queryFirst(
@@ -79,8 +79,10 @@ abstract class Room
 					LEFT JOIN machine m ON (lr.tutoruuid = m.machineuuid)
 					WHERE lr.locationid = :lid', ['lid' => $locationId]);
 			if ($room === false)
-				return false;
+				return null;
 			$room = self::loadSingleRoom($room);
+			if ($room === null)
+				return null;
 			// If it's a leaf room we probably don't need any other rooms, return it
 			if ($room->isLeaf())
 				return $room;
@@ -89,7 +91,7 @@ abstract class Room
 		}
 		if (isset(self::$rooms[$locationId]))
 			return self::$rooms[$locationId];
-		return false;
+		return null;
 	}
 
 	public function __construct($row)
@@ -102,35 +104,34 @@ abstract class Room
 	/**
 	 * @return int number of machines in this room
 	 */
-	abstract public function machineCount();
+	abstract public function machineCount(): int;
 
 	/**
 	 * Size of this room, returned by reference.
-	 * @param int $width OUT width of room
-	 * @param int $height OUT height of room
+	 *
+	 * @param int|null $width OUT width of room
+	 * @param int|null $height OUT height of room
 	 */
-	abstract public function getSize(&$width, &$height);
+	abstract public function getSize(?int &$width, ?int &$height);
 
 	/**
 	 * Get clients in this room in .ini format for PVS.
 	 * Adjusted so the top/left client is at (0|0), which
 	 * is further adjustable with $offX and $offY.
+	 *
 	 * @param int $i offset for indexing clients
 	 * @param int $offX positional X offset for clients
 	 * @param int $offY positional Y offset for clients
 	 * @return string|false
 	 */
-	abstract public function getIniClientSection(&$i, $offX = 0, $offY = 0);
+	abstract public function getIniClientSection(int &$i, int $offX = 0, int $offY = 0);
 
 	/**
 	 * Get clients in this room as array.
 	 * Adjusted so the top/left client is at (0|0), which
 	 *is further adjustable with $offX and $offY.
-	 * @param int $offX
-	 * @param int $offY
-	 * @return array
 	 */
-	abstract public function getShiftedArray($offX = 0, $offY = 0);
+	abstract public function getShiftedArray(int $offX = 0, int $offY = 0): ?array;
 
 	/**
 	 * @return string|false IP address of manager.
@@ -145,12 +146,12 @@ abstract class Room
 	/**
 	 * @return bool true if this is a simple/leaf room, false for composed rooms.
 	 */
-	abstract public function isLeaf();
+	abstract public function isLeaf(): bool;
 
 	/**
 	 * @return bool should this room be skipped from output? true for empty SimpleRoom or disabled ComposedRoom.
 	 */
-	abstract public function shouldSkip();
+	abstract public function shouldSkip(): bool;
 
 	/**
 	 * Sanitize this room's data.
@@ -160,7 +161,7 @@ abstract class Room
 	/**
 	 * @return string get room's name.
 	 */
-	public function locationName()
+	public function locationName(): string
 	{
 		return $this->locationName;
 	}
@@ -168,7 +169,7 @@ abstract class Room
 	/**
 	 * @return int get room's id.
 	 */
-	public function locationId()
+	public function locationId(): int
 	{
 		return $this->locationId;
 	}
diff --git a/modules-available/roomplanner/inc/simpleroom.inc.php b/modules-available/roomplanner/inc/simpleroom.inc.php
index 43ae43ca..b4d3e744 100644
--- a/modules-available/roomplanner/inc/simpleroom.inc.php
+++ b/modules-available/roomplanner/inc/simpleroom.inc.php
@@ -11,6 +11,7 @@ class SimpleRoom extends Room
 
 	private $tutorIp = false;
 
+	/** @var ?string */
 	private $managerIp = false;
 
 	public function __construct($row)
@@ -55,27 +56,29 @@ class SimpleRoom extends Room
 		}
 	}
 
-	public function machineCount()
+	public function machineCount(): int
 	{
 		return count($this->machines);
 	}
 
-	public function getSize(&$width, &$height)
+	public function getSize(?int &$width, ?int &$height)
 	{
 		if (empty($this->machines)) {
 			$width = $height = 0;
 			return;
 		}
+		$minX = $minY = $maxX = $maxY = 0;
 		$this->boundingBox($minX, $minY, $maxX, $maxY);
 		// client's size that cannot be configured as of today
 		$width = max($maxX - $minX + self::CLIENT_SIZE, 1);
 		$height = max($maxY - $minY + self::CLIENT_SIZE, 1);
 	}
 
-	public function getIniClientSection(&$i, $offX = 0, $offY = 0)
+	public function getIniClientSection(int &$i, int $offX = 0, int $offY = 0): string
 	{
 		/* output individual client positions, shift coordinates to requested position */
 		$out = '';
+		$minX = $minY = $maxX = $maxY = 0;
 		$this->boundingBox($minX, $minY, $maxX, $maxY);
 		foreach ($this->machines as $pos) {
 			$i++;
@@ -86,10 +89,11 @@ class SimpleRoom extends Room
 		return $out;
 	}
 
-	public function getShiftedArray($offX = 0, $offY = 0)
+	public function getShiftedArray(int $offX = 0, int $offY = 0): ?array
 	{
 		/* output individual client positions, shift coordinates to requested position */
 		$ret = [];
+		$minX = $minY = $maxX = $maxY = 0;
 		$this->boundingBox($minX, $minY, $maxX, $maxY);
 		foreach ($this->machines as $pos) {
 			$pos['gridCol'] += $offX - $minX;
@@ -100,7 +104,7 @@ class SimpleRoom extends Room
 		return $ret;
 	}
 
-	private function boundingBox(&$minX, &$minY, &$maxX, &$maxY)
+	private function boundingBox(int &$minX, int &$minY, int &$maxX, int &$maxY): void
 	{
 		if ($this->bb !== false) {
 			$minX = $this->bb[0];
@@ -130,12 +134,12 @@ class SimpleRoom extends Room
 		return $this->tutorIp;
 	}
 
-	public function isLeaf()
+	public function isLeaf(): bool
 	{
 		return true;
 	}
 
-	public function shouldSkip()
+	public function shouldSkip(): bool
 	{
 		return empty($this->machines);
 	}
diff --git a/modules-available/roomplanner/page.inc.php b/modules-available/roomplanner/page.inc.php
index 7ac6890d..53e8bf0b 100644
--- a/modules-available/roomplanner/page.inc.php
+++ b/modules-available/roomplanner/page.inc.php
@@ -82,22 +82,17 @@ class Page_Roomplanner extends Page
 		$config = Database::queryFirst('SELECT roomplan, managerip, tutoruuid
 			FROM location_roomplan
 			WHERE locationid = :locationid', ['locationid' => $this->locationid]);
+		if ($config === false) {
+			$config = ['dedicatedmgr' => false, 'managerip' => ''];
+		}
 		$runmode = RunMode::getForMode(Page::getModule(), $this->locationid, true);
-		if (empty($runmode)) {
-			$config['dedicatedmgr'] = false;
-		} else {
+		if (!empty($runmode)) {
 			$runmode = array_pop($runmode);
 			$config['managerip'] = $runmode['clientip'];
 			$config['manageruuid'] = $runmode['machineuuid'];
 			$data = json_decode($runmode['modedata'], true);
 			$config['dedicatedmgr'] = (isset($data['dedicatedmgr']) && $data['dedicatedmgr']);
 		}
-		if ($config !== false) {
-			$managerIp = $config['managerip'] ?? '';
-			$dediMgr = $config['dedicatedmgr'] ? 'checked' : '';
-		} else {
-			$dediMgr = $managerIp = '';
-		}
 		$furniture = $this->getFurniture($config);
 		$subnetMachines = $this->getPotentialMachines();
 		$machinesOnPlan = $this->getMachinesOnPlan($config['tutoruuid'] ?? '');
@@ -105,8 +100,8 @@ class Page_Roomplanner extends Page
 		$canEdit = User::hasPermission('edit', $this->locationid);
 		$params = [
 			'location' => $this->location,
-			'managerip' => $managerIp,
-			'dediMgrChecked' => $dediMgr,
+			'managerip' =>  $config['managerip'],
+			'dediMgrChecked' => $config['dedicatedmgr'] ? 'checked' : '',
 			'subnetMachines' => json_encode($subnetMachines),
 			'locationid' => $this->locationid,
 			'roomConfiguration' => json_encode($roomConfig),
@@ -229,11 +224,9 @@ class Page_Roomplanner extends Page
 		if ($leaf !== $this->isLeaf) {
 			if ($isAjax) {
 				die('Leaf mode mismatch. Did you restructure locations while editing this room?');
-			} else {
-				Message::addError('leaf-mode-mismatch');
-				Util::redirect("?do=roomplanner&locationid={$this->locationid}&action=show");
 			}
-			return;
+			Message::addError('leaf-mode-mismatch');
+			Util::redirect("?do=roomplanner&locationid={$this->locationid}&action=show");
 		}
 		if ($this->isLeaf) {
 			$this->saveLeafRoom($isAjax);
@@ -250,10 +243,9 @@ class Page_Roomplanner extends Page
 		if (!is_array($config) || !isset($config['furniture']) || !isset($config['computers'])) {
 			if ($isAjax) {
 				die('JSON data incomplete');
-			} else {
-				Message::addError('json-data-invalid');
-				Util::redirect("?do=roomplanner&locationid={$this->locationid}&action=show");
 			}
+			Message::addError('json-data-invalid');
+			Util::redirect("?do=roomplanner&locationid={$this->locationid}&action=show");
 		}
 		$tutorUuid = Request::post('tutoruuid', '', 'string');
 		if (empty($tutorUuid)) {
@@ -263,10 +255,9 @@ class Page_Roomplanner extends Page
 			if ($ret === false) {
 				if ($isAjax) {
 					die('Invalid tutor UUID');
-				} else {
-					Message::addError('invalid-tutor-uuid');
-					Util::redirect("?do=roomplanner&locationid={$this->locationid}&action=show");
 				}
+				Message::addError('invalid-tutor-uuid');
+				Util::redirect("?do=roomplanner&locationid={$this->locationid}&action=show");
 			}
 		}
 		$this->saveRoomConfig($config['furniture'], $tutorUuid);
@@ -282,10 +273,9 @@ class Page_Roomplanner extends Page
 		if ($res === false) {
 			if ($isAjax) {
 				die('Error writing config to DB');
-			} else {
-				Message::addError('db-error');
-				Util::redirect("?do=roomplanner&locationid={$this->locationid}&action=show");
 			}
+			Message::addError('db-error');
+			Util::redirect("?do=roomplanner&locationid={$this->locationid}&action=show");
 		}
 	}
 
@@ -302,7 +292,7 @@ class Page_Roomplanner extends Page
 	 * @param array $computers Deserialized json from browser with all the computers
 	 * @param array $oldComputers Deserialized old roomplan from database, used to find removed computers
 	 */
-	protected function saveComputerConfig($computers, $oldComputers)
+	protected function saveComputerConfig(array $computers, array $oldComputers)
 	{
 
 		$oldUuids = [];
@@ -323,12 +313,12 @@ class Page_Roomplanner extends Page
 			if (!isset($computer['gridRow'])) {
 				$computer['gridRow'] = 0;
 			} else {
-				$this->sanitizeNumber($computer['gridRow'], 0, 32 * 4);
+				Util::clamp($computer['gridRow'], 0, 32 * 4);
 			}
 			if (!isset($computer['gridCol'])) {
 				$computer['gridCol'] = 0;
 			} else {
-				$this->sanitizeNumber($computer['gridCol'], 0, 32 * 4);
+				Util::clamp($computer['gridCol'], 0, 32 * 4);
 			}
 
 			$position = json_encode(['gridRow' => $computer['gridRow'],
@@ -374,17 +364,20 @@ class Page_Roomplanner extends Page
 		}
 	}
 
-	protected function getFurniture($config)
+	protected function getFurniture(array $config): array
 	{
-		if ($config === false)
-			return array();
-		$config = json_decode($config['roomplan'] ?? '', true);
+		if (empty($config['roomplan']))
+			return [];
+		$config = json_decode($config['roomplan'], true);
 		if (!is_array($config))
-			return array();
+			return [];
 		return $config;
 	}
 
-	protected function getMachinesOnPlan($tutorUuid)
+	/**
+	 * @return array{computers: array}
+	 */
+	protected function getMachinesOnPlan(string $tutorUuid): array
 	{
 		$result = Database::simpleQuery('SELECT machineuuid, macaddr, clientip, hostname, position
 				FROM machine
@@ -418,7 +411,7 @@ class Page_Roomplanner extends Page
 		return ['computers' => $machines];
 	}
 
-	protected function getPotentialMachines()
+	protected function getPotentialMachines(): array
 	{
 		$result = Database::simpleQuery('SELECT m.machineuuid, m.macaddr, m.clientip, m.hostname, l.locationname AS otherroom, m.fixedlocationid
 			FROM machine m