[locations] Cleanup Location class

There was a mess of functions which mostly, but not quite, did the same
things. Get rid of a couple of them and fix call sites to use
alternative ones that also fit the job.
While at it, add phpdoc and comments to the remaining functions, trying
to clarify what they were designed for.
Lastly, the return type of functions that retrieve a location id has
been changed from false|int to ?int (nullable types are just nicer).

1 files changed, 193 insertions, 189 deletions

diff --git a/modules-available/locations/inc/location.inc.php b/modules-available/locations/inc/location.inc.php
index 807f8577..d4978068 100644
--- a/modules-available/locations/inc/location.inc.php
+++ b/modules-available/locations/inc/location.inc.php
@@ -3,28 +3,111 @@
 class Location
 {
 
-	private static $flatLocationCache = false;
-	private static $assocLocationCache = false;
-	private static $treeCache = false;
-	private static $subnetMapCache = false;
+	/** @var ?array */
+	private static $flatLocationCache = null;
+	/** @var ?array */
+	private static $assocLocationCache = null;
 
-	public static function getTree(): array
+	/**
+	 * Get a nested array of locations. Each element in the array represents a top-level
+	 * location, and locations with have child-nodes will have a 'children' key that again
+	 * contains an array of location elements, with 'children' as appropriate, and so on.
+	 * If you pass one or more startLocationIds, the tree will be cut down to only contain
+	 * the locations given (plus any subtrees they have).
+	 * @return array<array{locationid: int, locationname: string, parentlocationd: int, children: array}>
+	 */
+	public static function getTree(int ...$startLocationId): array
 	{
-		if (self::$treeCache === false) {
-			self::$treeCache = self::queryLocations();
-			self::$treeCache = self::buildTree(self::$treeCache);
+		static $treeCache = null;
+		if ($treeCache === null) {
+			$treeCache = self::buildTree(self::queryLocations());
 		}
-		return self::$treeCache;
+		if (empty($startLocationId) || in_array(0, $startLocationId))
+			return $treeCache;
+		// Subtree, search
+		return self::filterTreeByLocations($treeCache, $startLocationId);
 	}
 
-	public static function queryLocations(): array
+	/**
+	 * Return a filtered tree that only contains the locations present in $idList, plus their respective
+	 * child locations in the 'children' array key, if applicable. Top level elements will only be those
+	 * contained in $idList, but not every location given in $idList will be a top level element, as they
+	 * might already be present in one of the nested 'children' arrays.
+	 * @param array $locationTree part of tree to search in
+	 * @param array $idList list of wanted ids
+	 * @return array<array{locationid: int, locationname: string, parentlocationd: int, children: array}> filtered tree
+	 */
+	private static function filterTreeByLocations(array $locationTree, array $idList): array
 	{
-		$res = Database::simpleQuery("SELECT locationid, parentlocationid, locationname FROM location");
-		$rows = array();
-		foreach ($res as $row) {
-			$rows[] = $row;
+		$ret = [];
+		foreach ($locationTree as $location) {
+			if (in_array($location['locationid'], $idList)) {
+				$ret[] = $location;
+			} elseif (!empty($location['children'])) {
+				$ret = array_merge($ret, self::filterTreeByLocations($location['children'], $idList));
+			}
+		}
+		return $ret;
+	}
+
+	/**
+	 * @return array<array{locationid: int, locationname: string, parentlocationd: int, children: array}>
+	 */
+	private static function buildTree(array $elements, int $parentId = 0): array
+	{
+		$branch = array();
+		foreach ($elements as $lid => $element) {
+			$lid = (int)$lid;
+			if ($lid === 0 || $lid === $parentId)
+				continue;
+			if ($element['parentlocationid'] === $parentId) {
+				$children = self::buildTree($elements, $lid);
+				if (!empty($children)) {
+					$element['children'] = $children;
+				}
+				$element['locationid'] = $lid;
+				$branch[] = $element;
+			}
+		}
+		ArrayUtil::sortByColumn($branch, 'locationname');
+		return $branch;
+	}
+
+	private static function queryLocations(): array
+	{
+		static $locationDbCache = null;
+		if ($locationDbCache === null) {
+			$locationDbCache = Database::queryIndexedList("SELECT locationid, parentlocationid, locationname FROM location");
+			foreach ($locationDbCache as &$loc) {
+				$loc['parentlocationid'] = (int)$loc['parentlocationid'];
+			}
+		}
+		return $locationDbCache;
+	}
+
+	/**
+	 * Get all location ids starting from the specified location id.
+	 * If $startId is 0, it will return all available location ids.
+	 * If $withStart is true, the starting location id will be included in the result.
+	 *
+	 * @param int $startId The location id to start from (default is 0 to get all location ids).
+	 * @param bool $withStart True to include the starting location id in the result (default is false).
+	 * @return int[] An array of location ids, including the starting location id if requested.
+	 */
+	public static function getAllLocationIds(int $startId = 0, bool $withStart = false): array
+	{
+		if ($startId === 0) {
+			$locs = array_keys(self::queryLocations());
+		} else {
+			$locs = self::getLocationsAssoc();
+			if (!isset($locs[$startId]))
+				return [];
+			$locs = $locs[$startId]['children'] ?? [];
+		}
+		if ($withStart) {
+			$locs[] = $startId;
 		}
-		return $rows;
+		return $locs;
 	}
 
 	/**
@@ -43,79 +126,79 @@ class Location
 	 */
 	public static function getName(int $locationId)
 	{
-		if (self::$assocLocationCache === false) {
-			self::getLocationsAssoc();
-		}
-		if (!isset(self::$assocLocationCache[$locationId]))
-			return false;
-		return self::$assocLocationCache[$locationId]['locationname'];
+		$locs = self::queryLocations();
+		return $locs[$locationId]['locationname'] ?? false;
 	}
 
 	/**
 	 * Get all the names of the given location and its parents, up
 	 * to the root element. Array keys will be locationids, value the names.
-	 * @return array|false locations, from furthest to nearest or false if locationId doesn't exist
+	 * @return ?array locations, from furthest to nearest or false if locationId doesn't exist
 	 */
-	public static function getNameChain(int $locationId)
+	public static function getNameChain(int $locationId): ?array
 	{
-		if (self::$assocLocationCache === false) {
-			self::getLocationsAssoc();
-		}
-		if (!isset(self::$assocLocationCache[$locationId]))
-			return false;
-		$ret = array();
-		while (isset(self::$assocLocationCache[$locationId])) {
-			$ret[$locationId] = self::$assocLocationCache[$locationId]['locationname'];
-			$locationId = self::$assocLocationCache[$locationId]['parentlocationid'];
+		$locs = self::queryLocations();
+		$ret = [];
+		while (isset($locs[$locationId])) {
+			$ret[$locationId] = $locs[$locationId]['locationname'];
+			$locationId = $locs[$locationId]['parentlocationid'];
 		}
+		if (empty($ret))
+			return null;
 		return array_reverse($ret, true);
 	}
 
-	public static function getLocationsAssoc()
+	/**
+	 * Get associative array of locations. Mostly used for working with locations, in contrast to ::getLocations()
+	 * @return array<array{locationid: int, locationname: string, parentlocationid: int, parents: int[], children: int[], directchildren: int[], depth: int, isleaf: bool}> All the locations
+	 */
+	public static function getLocationsAssoc(): array
 	{
-		if (self::$assocLocationCache === false) {
-			$rows = self::getTree();
-			self::$assocLocationCache = self::flattenTreeAssoc($rows);
+		if (self::$assocLocationCache === null) {
+			self::$assocLocationCache = self::flattenTreeAssoc(self::getTree());
 		}
 		return self::$assocLocationCache;
 	}
 
-	private static function flattenTreeAssoc($tree, $parents = array(), $depth = 0): array
+	/**
+	 * @return array<array{locationid: int, locationname: string, parentlocationid: int, parents: int[], children: int[], directchildren: int[], depth: int, isleaf: bool}> All the locations
+	 */
+	private static function flattenTreeAssoc(array $tree, array $parents = [], int $depth = 0): array
 	{
-		if ($depth > 20) {
-			ErrorHandler::traceError('Recursive location definition detected at ' . print_r($tree, true));
-		}
-		$output = array();
+		$output = [];
 		foreach ($tree as $node) {
-			$cc = empty($node['children']) ? array() : array_map(function ($item) { return (int)$item['locationid']; }, $node['children']);
-			$output[(int)$node['locationid']] = array(
-				'locationid' => (int)$node['locationid'],
-				'parentlocationid' => (int)$node['parentlocationid'],
+			$cc = empty($node['children']) ? [] : array_column($node['children'], 'locationid');
+			$output[$node['locationid']] = [
+				'locationid' => $node['locationid'],
+				'parentlocationid' => $node['parentlocationid'],
 				'parents' => $parents,
 				'children' => $cc,
 				'directchildren' => $cc,
 				'locationname' => $node['locationname'],
 				'depth' => $depth,
-				'isleaf' => true,
-			);
+				'isleaf' => empty($cc),
+			];
 			if (!empty($node['children'])) {
-				$childNodes = self::flattenTreeAssoc($node['children'], array_merge($parents, array((int)$node['locationid'])), $depth + 1);
-				$output[(int)$node['locationid']]['children'] = array_merge($output[(int)$node['locationid']]['children'],
+				$childNodes = self::flattenTreeAssoc($node['children'], array_merge($parents, [$node['locationid']]), $depth + 1);
+				$output[$node['locationid']]['children'] = array_merge($output[$node['locationid']]['children'],
 					array_reduce($childNodes, function ($carry, $item) {
 						return array_merge($carry, $item['children']);
-					}, array()));
+					}, []));
 				$output += $childNodes;
 			}
 		}
-		foreach ($output as &$entry) {
-			if (!isset($output[$entry['parentlocationid']]))
-				continue;
-			$output[$entry['parentlocationid']]['isleaf'] = false;
-		}
 		return $output;
 	}
 
+	private static function buildFlatTree(): void
+	{
+		if (self::$flatLocationCache === null) {
+			self::$flatLocationCache = self::flattenTree(self::getTree());
+		}
+	}
+
 	/**
+	 * get locations as flat array - mostly used for rendering a <select>.
 	 * @param int|int[] $selected Which locationIDs to mark as selected
 	 * @param int $excludeId Which locationID to exclude
 	 * @param bool $addNoParent Add entry for "no location" at the top
@@ -124,13 +207,8 @@ class Location
 	 */
 	public static function getLocations($selected = 0, int $excludeId = 0, bool $addNoParent = false, bool $keepArrayKeys = false): array
 	{
-		if (self::$flatLocationCache === false) {
-			$rows = self::getTree();
-			$rows = self::flattenTree($rows);
-			self::$flatLocationCache = $rows;
-		} else {
-			$rows = self::$flatLocationCache;
-		}
+		self::buildFlatTree();
+		$rows = self::$flatLocationCache;
 		$del = false;
 		unset($row);
 		$index = 0;
@@ -150,94 +228,37 @@ class Location
 			$row['sortIndex'] = $index++;
 		}
 		if ($addNoParent) {
-			array_unshift($rows, array(
+			$noParent = [
 				'locationid' => 0,
 				'locationname' => '-----',
 				'selected' => $selected === 0,
 				'locationpad' => '',
-			));
+			];
+			if ($keepArrayKeys) {
+				$rows = array_reverse($rows, true);
+				$rows[0] = $noParent;
+				$rows = array_reverse($rows, true);
+			} else {
+				array_unshift($rows, $noParent);
+			}
 		}
 		if ($keepArrayKeys)
 			return $rows;
 		return array_values($rows);
 	}
 
-	/**
-	 * Get nested array of all the locations and children of given locationid(s).
-	 *
-	 * @param int[]|int $idList List of location ids
-	 * @param ?array $locationTree used in recursive calls, don't pass
-	 * @return array list of passed locations plus their children
-	 */
-	public static function getRecursive($idList, ?array $locationTree = null): array
-	{
-		if (!is_array($idList)) {
-			$idList = array($idList);
-		}
-		if ($locationTree === null) {
-			$locationTree = self::getTree();
-		}
-		$ret = array();
-		foreach ($locationTree as $location) {
-			if (in_array($location['locationid'], $idList)) {
-				$ret[] = $location;
-			} elseif (!empty($location['children'])) {
-				$ret = array_merge($ret, self::getRecursive($idList, $location['children']));
-			}
-		}
-		return $ret;
-	}
-
-	/**
-	 * Get flat array of all the locations and children of given locationid(s).
-	 *
-	 * @param int[]|int $idList List of location ids
-	 * @return array list of passed locations plus their children
-	 */
-	public static function getRecursiveFlat($idList): array
-	{
-		$ret = self::getRecursive($idList);
-		if (!empty($ret)) {
-			$ret = self::flattenTree($ret);
-		}
-		return $ret;
-	}
-
-	public static function buildTree(array $elements, int $parentId = 0): array
-	{
-		$branch = array();
-		$sort = array();
-		foreach ($elements as $element) {
-			if ($element['locationid'] == 0 || $element['locationid'] == $parentId)
-				continue;
-			if ($element['parentlocationid'] == $parentId) {
-				$children = self::buildTree($elements, $element['locationid']);
-				if (!empty($children)) {
-					$element['children'] = $children;
-				}
-				$branch[] = $element;
-				$sort[] = $element['locationname'];
-			}
-		}
-		array_multisort($sort, SORT_ASC, $branch);
-		return $branch;
-	}
-
 	private static function flattenTree(array $tree, int $depth = 0): array
 	{
-		if ($depth > 20) {
-			ErrorHandler::traceError('Recursive location definition detected at ' . print_r($tree, true));
-		}
-		$output = array();
+		$output = [];
 		foreach ($tree as $node) {
-			$output[(int)$node['locationid']] = array(
+			$output[$node['locationid']] = [
 				'locationid' => $node['locationid'],
 				'parentlocationid' => $node['parentlocationid'],
 				'locationname' => $node['locationname'],
 				'locationpad' => str_repeat('--', $depth),
 				'isleaf'	=> empty($node['children']),
-				'depth' => $depth
-			);
+				'depth' => $depth,
+			];
 			if (!empty($node['children'])) {
 				$output += self::flattenTree($node['children'], $depth + 1);
 			}
@@ -247,39 +268,28 @@ class Location
 
 	public static function isLeaf(int $locationid): bool
 	{
-		$result = Database::queryFirst('SELECT COUNT(locationid) = 0 AS isleaf '
-			. 'FROM location '
-			. 'WHERE parentlocationid = :locationid', ['locationid' => $locationid]);
-		$result = $result['isleaf'];
-		return (bool)$result;
-	}
-
-	public static function extractIds(array $tree): array
-	{
-		$ids = array();
-		foreach ($tree as $node) {
-			$ids[] = (int)$node['locationid'];
-			if (!empty($node['children'])) {
-				$ids = array_merge($ids, self::extractIds($node['children']));
-			}
-		}
-		return $ids;
+		if (self::$flatLocationCache !== null)
+			return self::$flatLocationCache[$locationid]['isleaf'] ?? false;
+		if (self::$assocLocationCache !== null)
+			return self::$assocLocationCache[$locationid]['isleaf'] ?? false;
+		return !empty(self::getAllLocationIds($locationid));
 	}
 
 	/**
 	 * Get location id for given machine (by uuid)
 	 *
 	 * @param string $uuid machine uuid
-	 * @return false|int locationid, false if no match
+	 * @return ?int locationid, null if no match
 	 */
-	public static function getFromMachineUuid(string $uuid)
+	public static function getFromMachineUuid(string $uuid): ?int
 	{
 		// Only if we have the statistics module which supplies the machine table
 		if (Module::get('statistics') === false)
-			return false;
-		$ret = Database::queryFirst("SELECT locationid FROM machine WHERE machineuuid = :uuid", compact('uuid'));
+			return null;
+		$ret = Database::queryFirst("SELECT locationid FROM machine WHERE machineuuid = :uuid",
+			['uuid' => $uuid]);
 		if ($ret === false || !$ret['locationid'])
-			return false;
+			return null;
 		return (int)$ret['locationid'];
 	}
 
@@ -289,9 +299,9 @@ class Location
 	 *
 	 * @param string $ip IP address of client
 	 * @param bool $honorRoomPlanner consider a fixed location assigned manually by roomplanner
-	 * @return false|int locationid, or false if no match
+	 * @return ?int locationid, or null if no match
 	 */
-	public static function getFromIp(string $ip, bool $honorRoomPlanner = false)
+	public static function getFromIp(string $ip, bool $honorRoomPlanner = false): ?int
 	{
 		if (Module::get('statistics') !== false) {
 			// Shortcut - try to use subnetlocationid in machine table
@@ -308,7 +318,7 @@ class Location
 				if ($ret['loc'] > 0) {
 					return (int)$ret['loc'];
 				}
-				return false;
+				return null;
 			}
 		}
 		return self::mapIpToLocation($ip);
@@ -321,13 +331,13 @@ class Location
 	 *
 	 * @param string $ip IP address of client
 	 * @param ?string $uuid System-UUID of client
-	 * @return int|false location id, or false if none matches
+	 * @return ?int location id, or null if none matches
 	 */
-	public static function getFromIpAndUuid(string $ip, ?string $uuid)
+	public static function getFromIpAndUuid(string $ip, ?string $uuid): ?int
 	{
 		$locationId = false;
 		$ipLoc = self::getFromIp($ip);
-		if ($ipLoc !== false) {
+		if ($ipLoc !== null) {
 			// Set locationId to ipLoc for now, it will be overwritten later if another case applies.
 			$locationId = $ipLoc;
 			if ($uuid !== null) {
@@ -341,7 +351,7 @@ class Location
 		return $locationId;
 	}
 
-	public static function isFixedLocationValid($uuidLoc, $ipLoc): bool
+	public static function isFixedLocationValid(?int $uuidLoc, int $ipLoc): bool
 	{
 		if ($uuidLoc === false)
 			return false;
@@ -368,18 +378,16 @@ class Location
 	 */
 	public static function getLocationRootChain(int $locationId): array
 	{
-		if (self::$assocLocationCache === false) {
-			self::getLocationsAssoc();
-		}
-		if (!isset(self::$assocLocationCache[$locationId]))
+		$locs = self::getLocationsAssoc();
+		if (!isset($locs[$locationId]))
 			return [];
-		$chain = self::$assocLocationCache[$locationId]['parents'];
+		$chain = $locs[$locationId]['parents'];
 		$chain[] = $locationId;
 		return array_reverse($chain);
 	}
 
 	/**
-	 * @return array list of subnets as numeric array
+	 * @return array<array{locationid: int, startaddr: int, endaddr: int}> list of subnets
 	 */
 	public static function getSubnets(): array
 	{
@@ -393,7 +401,7 @@ class Location
 	}
 
 	/**
-	 * @return array assoc array mapping from locationid to subnets
+	 * @return array<array{locationid: int, subnets: array<array{startaddr:int, endaddr: int}>}> assoc array mapping from locationid to subnets
 	 */
 	public static function getSubnetsByLocation($recursive = false): array
 	{
@@ -429,48 +437,44 @@ class Location
 	 * random one will be returned.
 	 *
 	 * @param string $ip IP to look up
-	 * @return false|int locationid ip matches, false = no match
+	 * @return ?int locationid ip matches, null = no match
 	 */
-	public static function mapIpToLocation(string $ip)
+	public static function mapIpToLocation(string $ip): ?int
 	{
-		if (self::$subnetMapCache === false) {
-			self::$subnetMapCache = self::getSubnetsByLocation();
+		static $subnetMapCache = null;
+		if ($subnetMapCache === null) {
+			$subnetMapCache = self::getSubnetsByLocation();
 		}
 		$long = sprintf('%u', ip2long($ip));
-		$best = false;
+		$best = null;
 		$bestSize = 0;
-		foreach (self::$subnetMapCache as $lid => $data) {
-			if ($best !== false && self::$subnetMapCache[$lid]['depth'] < self::$subnetMapCache[$best]['depth'])
+		foreach ($subnetMapCache as $lid => $curLocation) {
+			if ($best !== null && $curLocation['depth'] < $subnetMapCache[$best]['depth'])
 				continue; // Don't even need to take a look
-			foreach ($data['subnets'] as $subnet) {
+			foreach ($curLocation['subnets'] as $subnet) {
 				if ($long < $subnet['startaddr'] || $long > $subnet['endaddr'])
 					continue; // Nope
-				if ($best !== false // Already have a best candidate
-						&& self::$subnetMapCache[$lid]['depth'] === self::$subnetMapCache[$best]['depth'] // Same depth
+				if ($best !== null // Already have a best candidate
+						&& $curLocation['depth'] === $subnetMapCache[$best]['depth'] // Same depth
 						&& $bestSize < $subnet['endaddr'] - $subnet['startaddr']) { // Old candidate has smaller subnet
 					// So we ignore this one as the old one is more specific
 					continue;
 				}
 				$bestSize = $subnet['endaddr'] - $subnet['startaddr'];
-				$best = $lid;
+				$best = (int)$lid;
 			}
 		}
-		if ($best === false)
-			return false;
-		return (int)$best;
+		return $best;
 	}
 
 	/**
-	 * @return false|int newly determined location
+	 * @return ?int newly determined location
 	 */
-	public static function updateMapIpToLocation(string $uuid, string $ip)
+	public static function updateMapIpToLocation(string $uuid, string $ip): ?int
 	{
 		$loc = self::mapIpToLocation($ip);
-		if ($loc === false) {
-			Database::exec("UPDATE machine SET subnetlocationid = NULL WHERE machineuuid = :uuid", compact('uuid'));
-		} else {
-			Database::exec("UPDATE machine SET subnetlocationid = :loc WHERE machineuuid = :uuid", compact('loc', 'uuid'));
-		}
+		Database::exec("UPDATE machine SET subnetlocationid = :loc WHERE machineuuid = :uuid",
+			['loc' => $loc, 'uuid' => $uuid]);
 		return $loc;
 	}