WR #490867 auth_outage Review changes - code cleanup: fix exception chaining, remove dead code, code style fixes, initialise variable, add missing PHPDoc, add database indexes

bootstrap.php

@@ -69,6 +69,7 @@ if (is_callable('auth_outage_bootstrap_callback')) {
 }
 
 // 3) Check for allowed scripts or IPs during outages.
+$outageinfo = false;
 if (!empty($_SERVER['REQUEST_URI'])) {
     $rooturl = parse_url($CFG->wwwroot);
     $path = '';

classes/calendar/calendar.php

@@ -40,16 +40,18 @@ class calendar {
     /**
      * Create an event on the calendar for this outage.
      * @param outage $outage Outage to be added to the calendar.
+     * @return void
      */
-    public static function create(outage $outage) {
+    public static function create(outage $outage): void {
         calendar_event::create(self::create_data($outage));
     }
 
     /**
      * Updates an event on the calendar based on this outage.
      * @param outage $outage Outage to be updated in the calendar.
+     * @return void
      */
-    public static function update(outage $outage) {
+    public static function update(outage $outage): void {
         $event = self::load($outage->id);
 
         if (is_null($event)) {
@@ -63,8 +65,9 @@ class calendar {
     /**
      * Removes an event from the calendar related to this outage.
      * @param int $outageid Id of outage to be deleted from the calendar.
+     * @return void
      */
-    public static function delete($outageid) {
+    public static function delete(int $outageid): void {
         $event = self::load($outageid);
 
         // If not found (was not created before) ignore it.

classes/dml/outagedb.php

@@ -48,6 +48,7 @@ class outagedb {
 
     /**
      * Gets all outage entries.
+     * @return outage[]
      */
     public static function get_all() {
         global $DB;
@@ -361,7 +362,7 @@ class outagedb {
         $data = $DB->get_records_select(
             'auth_outage',
             'starttime <= :datetime1 AND :datetime2 <= stoptime AND finished IS NULL',
-            ['datetime1' => $time, 'datetime2' => $time, 'datetime3' => $time],
+            ['datetime1' => $time, 'datetime2' => $time],
             'starttime ASC, stoptime DESC, title ASC',
             '*',
             0,

classes/local/cli/cli_exception.php

@@ -79,6 +79,6 @@ class cli_exception extends Exception {
      * @param Exception|null $previous Another exception as reference or null.
      */
     public function __construct($message, $code = 1, ?Exception $previous = null) {
-        parent::__construct('*ERROR* ' . $message, $code, $previous = null);
+        parent::__construct('*ERROR* ' . $message, $code, $previous);
     }
 }

classes/local/cli/clibase.php

@@ -114,7 +114,6 @@ abstract class clibase {
      * Change session to admin user.
      */
     protected function become_admin_user() {
-        global $DB;
         $user = get_admin();
         unset($user->description);
         unset($user->access);

classes/local/cli/create.php

@@ -28,8 +28,7 @@ use coding_exception;
  * @copyright  2016 Catalyst IT
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
-class create extends clibase
-{
+class create extends clibase {
     /**
      * @var mixed[] Defaults to use if given option is null.
      */
@@ -99,6 +98,7 @@ class create extends clibase
 
     /**
      * Executes the CLI.
+     * @throws cli_exception
      */
     public function execute() {
         // Help always overrides any other parameter.
@@ -270,31 +270,4 @@ class create extends clibase
         return $option;
     }
 
-    /**
-     * Ensures the given option is or can be converted to a bool.
-     * @param mixed $option The parameter to check.
-     * @param string $param Name of that parameter.
-     * @return bool The converted parameter.
-     * @throws cli_exception
-     */
-    private function merge_options_check_parameters_bool($option, $param) {
-        if (is_bool($option)) {
-            return $option;
-        }
-
-        if (is_string($option)) {
-            $option = strtoupper($option);
-            if (in_array($option, ['0', 'FALSE', 'NO', 'N'])) {
-                return false;
-            }
-            if (in_array($option, ['1', 'TRUE', 'YES', 'Y'])) {
-                return true;
-            }
-        }
-
-        throw new cli_exception(
-            get_string('clierrorinvalidvaluenotbool', 'auth_outage', ['param' => $param]),
-            cli_exception::ERROR_PARAMETER_INVALID
-        );
-    }
 }

classes/local/cli/finish.php

@@ -56,6 +56,7 @@ class finish extends clibase {
 
     /**
      * Executes the CLI.
+     * @throws cli_exception
      */
     public function execute() {
         // Help always overrides any other parameter.

classes/local/cli/waitforit.php

@@ -101,6 +101,7 @@ class waitforit extends clibase {
         $outage = $this->get_outage();
 
         while ($sleep = $this->wait_for_outage_to_start($outage)) {
+            $sleep = max(1, $sleep);
             if (is_null($this->sleepcallback)) {
                 $this->verbose('Sleeping for ' . $sleep . ' second(s).');
                 sleep($sleep);

classes/local/controllers/infopage.php

@@ -123,10 +123,7 @@ class infopage {
         ];
         require($CFG->dirroot . '/auth/outage/views/info/content.php');
 
-        // Moodle 2.7 did not check for CLI mode, which was fixed later.
-        if (!($CFG->branch == '27' && CLI_SCRIPT)) {
-            echo $OUTPUT->footer();
-        }
+        echo $OUTPUT->footer();
         $CFG->svgicons = $previoussvg;
     }
 

classes/local/outagelib.php

@@ -51,6 +51,7 @@ class outagelib {
     /**
      * Fetches page.
      * @param string $file file to be fetched
+     * @return array{contents: string|false, mime: string}
      */
     public static function fetch_page($file) {
         global $CFG;
@@ -69,6 +70,7 @@ class outagelib {
 
     /**
      * Resets inject called to allow the code to be regenerated.
+     * @return void
      */
     public static function reset_injectcalled() {
         self::$injectcalled = false;
@@ -77,6 +79,7 @@ class outagelib {
     /**
      * Given a time, usually now, when is the next outage window?
      * @param int $time time for next window
+     * @return int
      */
     public static function get_next_window($time = null) {
 

classes/task/update_static_page.php

@@ -37,7 +37,10 @@ class update_static_page extends scheduled_task {
     }
 
     /**
-     * Executes the event.
+     * Executes the task: regenerates the maintenance static page for the next scheduled outage.
+     *
+     * @throws \coding_exception
+     * @throws \file_exception
      */
     public function execute() {
         outagelib::prepare_next_outage();

db/install.xml

@@ -21,9 +21,14 @@
       </FIELDS>
       <KEYS>
         <KEY NAME="primary" TYPE="primary" FIELDS="id"/>
+        <KEY NAME="fk_createdby" TYPE="foreign" FIELDS="createdby" REFTABLE="user" REFFIELDS="id"/>
+        <KEY NAME="fk_modifiedby" TYPE="foreign" FIELDS="modifiedby" REFTABLE="user" REFFIELDS="id"/>
       </KEYS>
       <INDEXES>
         <INDEX NAME="start_stop_title" UNIQUE="false" FIELDS="starttime, stoptime, title"/>
+        <INDEX NAME="ix_stoptime_finished" UNIQUE="false" FIELDS="stoptime, finished"/>
+        <INDEX NAME="ix_createdby" UNIQUE="false" FIELDS="createdby"/>
+        <INDEX NAME="ix_modifiedby" UNIQUE="false" FIELDS="modifiedby"/>
       </INDEXES>
     </TABLE>
   </TABLES>