diff --git a/pinc/Activity.inc b/pinc/Activity.inc index 2bf76a334..124081744 100644 --- a/pinc/Activity.inc +++ b/pinc/Activity.inc @@ -29,7 +29,24 @@ $ACCESS_CRITERIA = [ */ class Activities { - private static $_activities = []; + // `$_activities` contains a mapping from a specific type of Activity (e.g. an + // Activity, a Stage, a Round or a Pool) to the instances of those. They are mapped + // by their name, called IDs). The logic accounts for the class hierarchy: + // Stage derives from Activity so any instance will be on both the Stage and + // the Activity lists. + // + // This poses a fundamental issue with adding types to this logic: + // the type of each list depends on its name. + // + // A simple solution would be to return the top-level class Activity (to match how + // `$_activities` is typed) but that confuses (rightly) PHPStan and would require a + // lot of new type checks whenever callers manipulate these classes. + // + // Thus for now, we don't return types for several getters that manipulate + // `$_activities`. Over time, we will have to find a way to disentangle + // the different lists to add proper types. + /** @var array> */ + private static array $_activities = []; // This maps the static container class names to the class types // they contain. This makes the functions in this base class return the @@ -41,12 +58,12 @@ class Activities "Pools" => "Pool", ]; - private static function _get_class_mapping() + private static function _get_class_mapping(): string { return self::$container_to_class_mapping[static::class]; } - public static function add(&$activity) + public static function add(Activity &$activity): void { foreach (array_merge([get_class($activity)], class_parents($activity)) as $class) { assert(!isset(self::$_activities[$class][$activity->id]), "Duplicate activity ID; IDs must be unique"); @@ -54,18 +71,27 @@ class Activities } } - public static function get_all() + /** + * The return of this function is untyped for now (see the comment above `$_activities` for why). + */ + public static function get_all(): array { return self::$_activities[self::_get_class_mapping()]; } - public static function get_ids() + /** @return string[] */ + public static function get_ids(): array { // get_all() will ensure they are the right object types return array_keys(self::get_all()); } - protected static function _get($attribute, $value) + /** + * The return of this function is untyped for now (see the comment above `$_activities` for why). + * + * @param int|string $value + */ + protected static function _get(string $attribute, $value) { // fast-track lookup by ID if ($attribute == "id") { @@ -86,7 +112,10 @@ class Activities return null; } - public static function get_by_id($id) + /** + * The return of this function is untyped for now (see the comment above `$_activities` for why). + */ + public static function get_by_id(string $id) { return self::$_activities[self::_get_class_mapping()][$id] ?? null; } @@ -179,7 +208,7 @@ class Activity Activities::add($this); } - public function __toString() + public function __toString(): string { return $this->id; } @@ -207,7 +236,8 @@ class Activity * If $n_pages_completed is non-null, use it as the number of pages * that the user has completed. Otherwise, consult the database. */ - public function user_access($username, $n_pages_completed = null) + // TODO(jchaffraix): Add a class for the UserAccess object for type soundness. + public function user_access(?string $username, $n_pages_completed = null): object { if (is_null($username)) { $uao = new StdClass(); // user access object @@ -351,7 +381,11 @@ class Activity // -------------------------------------------------------------------------- -function get_user_scores($username, $criteria) +/** + * @param array $criteria + * @return array + */ +function get_user_scores(string $username, array $criteria): array { if (!$criteria) { return []; @@ -360,13 +394,13 @@ function get_user_scores($username, $criteria) $user_obj = $username == User::current_username() ? User::load_current() : new User($username); $user_scores = []; - foreach ($criteria as $criterion_code => $criterion_descr) { + foreach (array_keys($criteria) as $criterion_code) { $user_scores[$criterion_code] = get_user_score($user_obj, $criterion_code); } return $user_scores; } -function get_user_score($user_obj, $criterion_code) +function get_user_score(User $user_obj, string $criterion_code): float { if ($criterion_code == 'days since reg') { $user_score = round((time() - $user_obj->date_created) / 86400, 1); @@ -384,7 +418,7 @@ function get_user_score($user_obj, $criterion_code) // -------------------------------------------------------------------------- -function show_user_access_object($uao, $will_autogrant = false) +function show_user_access_object(object $uao, bool $will_autogrant = false): void { global $code_url; @@ -489,7 +523,7 @@ function show_user_access_object($uao, $will_autogrant = false) } } -function grant_user_access_if_sat(&$uao) +function grant_user_access_if_sat(object &$uao): void { $activity = Activities::get_by_id($uao->activity_id); if ($uao->request_status == 'sat-available' && @@ -505,7 +539,7 @@ function grant_user_access_if_sat(&$uao) // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX -function show_user_access_chart($username) +function show_user_access_chart(?string $username): void { global $ACCESS_CRITERIA, $code_url; @@ -682,7 +716,7 @@ function show_user_access_chart($username) } } -function td_w_bgcolor($text, $bool) +function td_w_bgcolor(string $text, bool $bool): void { $cell_class = $bool ? 'satisfied' : 'not_satisfied'; echo "$text"; diff --git a/pinc/Pool.inc b/pinc/Pool.inc index 1e879bb7c..90ac5a402 100644 --- a/pinc/Pool.inc +++ b/pinc/Pool.inc @@ -7,7 +7,7 @@ include_once($relPath.'Stage.inc'); */ class Pools extends Stages { - public static function get_by_state($state) + public static function get_by_state(string $state): ?Pool { return self::_get("states", $state); } @@ -77,12 +77,12 @@ class Pool extends Stage // --------------------------- -function get_Pool_for_id($pool_id) +function get_Pool_for_id(string $pool_id): ?Pool { return Pools::get_by_id($pool_id); } -function get_Pool_for_state($state) +function get_Pool_for_state(string $state): ?Pool { return Pools::get_by_state($state); } diff --git a/pinc/Round.inc b/pinc/Round.inc index aeca0f468..4825b6bfc 100644 --- a/pinc/Round.inc +++ b/pinc/Round.inc @@ -7,7 +7,8 @@ include_once($relPath.'Stage.inc'); */ class Rounds extends Stages { - public static function get_page_states() + /** @return string[] */ + public static function get_page_states(): array { $page_states = []; foreach (self::get_all() as $round) { @@ -18,22 +19,22 @@ class Rounds extends Stages return $page_states; } - public static function get_by_number($number) + public static function get_by_number(int $number): ?Round { return self::_get("round_number", $number); } - public static function get_by_project_state($state) + public static function get_by_project_state(string $state): ?Round { return self::_get("project_states", $state); } - public static function get_by_page_state($state) + public static function get_by_page_state(string $state): ?Round { return self::_get("page_states", $state); } - public static function get_last() + public static function get_last(): Round { return self::get_by_id(array_key_last(self::get_all())); } @@ -208,25 +209,26 @@ class Round extends Stage $this->mentee_round = null; } - public function is_a_mentee_round() + public function is_a_mentee_round(): bool { return !is_null($this->mentor_round); } - public function is_a_mentor_round() + + public function is_a_mentor_round(): bool { return !is_null($this->mentee_round); } // ----------- - public function has_a_daily_page_limit() + public function has_a_daily_page_limit(): bool { return !is_null($this->daily_page_limit); } // ----------- - public function get_honorific_for_page_tally($page_tally) + public function get_honorific_for_page_tally(int $page_tally): string { // Note that krsort($this->honorifics) put it in descending order. foreach ($this->honorifics as $threshold => $honorific) { @@ -243,7 +245,7 @@ class Round extends Stage // ----------- - public function validate_user_can_access($user) + public function validate_user_can_access(string $user): void { global $code_url; @@ -262,7 +264,7 @@ class Round extends Stage // --------------------------- -function get_Round_for_round_id($round_id) +function get_Round_for_round_id(string $round_id): ?Round { return Rounds::get_by_id($round_id); } @@ -275,21 +277,21 @@ function get_Round_for_round_id($round_id) * If `$round_number` is a valid proofreading-round number, * return the appropriate Round instance. Otherwise, return NULL. */ -function get_Round_for_round_number($round_number) +function get_Round_for_round_number(int $round_number): ?Round { return Rounds::get_by_number($round_number); } // --------------------------- -function get_Round_for_project_state($project_state) +function get_Round_for_project_state(string $project_state): ?Round { return Rounds::get_by_project_state($project_state); } // --------------------------- -function get_Round_for_page_state($page_state) +function get_Round_for_page_state(string $page_state): ?Round { return Rounds::get_by_page_state($page_state); } diff --git a/pinc/Stage.inc b/pinc/Stage.inc index 409b1f2c9..d6db8ca88 100644 --- a/pinc/Stage.inc +++ b/pinc/Stage.inc @@ -67,12 +67,12 @@ class Stage extends Activity * Display a page-header, either an image (if available) or a textual title * for this stage. */ - public function page_header($title) + public function page_header(string $title): void { echo "

$title

\n" . get_page_header_image($this->id); } - public function page_top($uao) + public function page_top(object $uao): void { $this->page_header("{$this->id}: {$this->name}"); @@ -97,14 +97,14 @@ class Stage extends Activity // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX -function get_Stage_for_id($id) +function get_Stage_for_id(string $id): ?Stage { return Stages::get_by_id($id); } // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX -function user_can_work_in_stage($username, $stage_id) +function user_can_work_in_stage(string $username, string $stage_id): bool { $stage = get_Stage_for_id($stage_id); $uao = $stage->user_access($username); @@ -113,7 +113,8 @@ function user_can_work_in_stage($username, $stage_id) // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX -function get_stages_user_can_work_in($username) +/** @return Stage[] */ +function get_stages_user_can_work_in(string $username): array { $accessible_stages = []; foreach (Stages::get_all() as $stage) { @@ -131,8 +132,10 @@ function get_stages_user_can_work_in($username) * * If $include_accessible_stages is true, this array also includes * stages the user has access to. + * + * @return Stage[] */ -function get_stages_for_which_user_has_access_to_prereqs($username, $include_accessible_stages = false) +function get_stages_for_which_user_has_access_to_prereqs(string $username, bool $include_accessible_stages = false): array { $satisfied_prereq_stages = []; diff --git a/tools/proofers/my_projects.php b/tools/proofers/my_projects.php index 55bfef779..49c93c45e 100644 --- a/tools/proofers/my_projects.php +++ b/tools/proofers/my_projects.php @@ -604,7 +604,7 @@ function get_round_query_result($round_view, $round_sort, $round_column_specs, $ // and select on those $avail_states = []; foreach (get_stages_user_can_work_in($username) as $stage) { - if (Rounds::get_by_id($stage->id)) { + if ($stage instanceof Round) { $avail_states[] = $stage->project_available_state; } } diff --git a/tools/proofers/my_suggestions.php b/tools/proofers/my_suggestions.php index 090070b24..aae1a068f 100644 --- a/tools/proofers/my_suggestions.php +++ b/tools/proofers/my_suggestions.php @@ -595,7 +595,7 @@ function get_user_suggestion_criteria(?string $username, $flush_cache = false) $states_can_work_in = []; foreach (get_stages_user_can_work_in($username) as $stage_id => $stage) { // only include rounds - if (Rounds::get_by_id($stage_id)) { + if ($stage instanceof Round) { $states_can_work_in[$stage_id] = $stage->project_available_state; } }