kiklop74/recommendations.md Secret

## recommendations.md

      
    Raw
  

              recommendations.md
            
          
    Modifications mod_bigbluebutton plugin
Suggestions for code modifications in mod_bigbluebutton plugin
All suggestions are based on the master branch version of the code.


All includes of config.php should use require instead of require_once. This incorrect practice persisted for years. (importance low)
  require_once('config.php');
  // Should be refactored to:
  require('config.php');


All places that have dirname(dirname(dirname(__FILE__))) should be transformed to __DIR__.'/../../../' (importance low)
  require_once(dirname(dirname(dirname(__FILE__))).'/config.php');
  // Should be transformed into:
  require(__DIR__.'/../../../config.php');


The page that presents content to the user should not have any functions or classes implemented inside it. All such could should be moved at least to separate lib files or refactored completely where needed. (importance high)
The files in question are:

mod/bigbluebuttonbn/view.php
mod/bigbluebuttonbn/bbb_view.php
mod/bigbluebuttonbn/bbb_broker.php
mod/bigbluebuttonbn/import_view.php
mod/bigbluebuttonbn/index.php


Given that current master branch of the plugin lists Moodle 3.2 as minimal supported version we should try to move more of the user presented content to use Mustache templates. Benefints are obvious and numerous.


It would be also benefitial to refactor all YUI plugins to AMD format. In cases where YUI functionality is required it can still be used within AMD plugin. Here is a small example of YUI Datatable inside Moodle AMD plugin:
  // local_foo/external plugin.
  define(['jquery', 'core/yui', 'core/log'],
      function ($, Yi, log) {
  
          function init_datatable() {
              Yi.use('datasource-io', 'datasource-jsonschema', 'datatable',
                  function (Y) {
                      var dataSource = new Y.DataSource.IO({
                          source: 'https://jsonplaceholder.typicode.com/posts',
                          ioConfig: {
                              method: 'GET',
                              headers: {
                                  'Accept': 'application/json'
                              },
                              timeout: 1000
                          }
                      });
  
                      dataSource.plug(
                          Y.Plugin.DataSourceJSONSchema,
                          {
                              schema: {
                                  resultFields: [{key:'id'}, {key:'title'}, {key:'body'}]
                              }
                          }
                      );
  
                      var table = new Y.DataTable({
                          columns: [
                              {key: 'id'   , label: 'Id'     },
                              {key: 'title', label: 'Country'},
                              {key: 'body' , label: 'Name'   }
                          ],
                          caption: 'Data'
                      });
  
                      table.plug(Y.Plugin.DataTableDataSource, {datasource: dataSource});
  
                      table.datasource.load();
  
                      dataSource.after('response', function() {
                          table.render('.example');
                      });
                  }
              ); // End YUI.
          }
  
          return {
              table: null,
              init: function () {
                  init_datatable();
              }
          };
  
      } // End Main function.
  );

  // This would be later invoked in the page like this:
  $PAGE->requires->js_call_amd('local_foo/external', 'init');


Walkthrough for refactoring of mod/bigbluebuttonbn/view.php
Step 1 - cleaning includes
require_once(dirname(dirname(dirname(__FILE__))).'/config.php');
require_once(dirname(__FILE__).'/locallib.php');

Becomes:
require(__DIR__.'/../../../config.php');
require_once(__DIR__.'/locallib.php');

Step 2 - properly using print_error
$viewinstance = bigbluebuttonbn_view_validator($id, $bn);
if (!$viewinstance) {
    print_error(get_string('view_error_url_missing_parameters', 'bigbluebuttonbn'));
}

Becomes:
$viewinstance = bigbluebuttonbn_view_validator($id, $bn);
if (!$viewinstance) {
    print_error('view_error_url_missing_parameters', 'mod_bigbluebuttonbn');
}

Step 2a - cleaning bigbluebuttonbn_view_validator function
function bigbluebuttonbn_view_validator($id, $bigbluebuttonbnid) {
    if ($id) {
        return bigbluebuttonbn_view_instance_id($id);
    }
    if ($bigbluebuttonbnid) {
        return bigbluebuttonbn_view_instance_bigbluebuttonbn($bigbluebuttonbnid);
    }
    return;
}

Should become: (it is essentially the same but there is no ambiguity (implicit null on empty return)
function bigbluebuttonbn_view_validator($id, $bigbluebuttonbnid) {
    $result = null;
    if ($id) {
        $result =  bigbluebuttonbn_view_instance_id($id);
    } else if ($bigbluebuttonbnid) {
        $result = bigbluebuttonbn_view_instance_bigbluebuttonbn($bigbluebuttonbnid);
    }
    return $result;
}

Step 3 - removing unneeded code
if (is_null($serverversion)) {
    if ($bbbsession['administrator']) {
        print_error('view_error_unable_join', 'bigbluebuttonbn',
            $CFG->wwwroot.'/admin/settings.php?section=modsettingbigbluebuttonbn');
        exit;
    }
    if ($bbbsession['moderator']) {
        print_error('view_error_unable_join_teacher', 'bigbluebuttonbn',
            $CFG->wwwroot.'/course/view.php?id='.$bigbluebuttonbn->course);
        exit;
    }
    print_error('view_error_unable_join_student', 'bigbluebuttonbn',
        $CFG->wwwroot.'/course/view.php?id='.$bigbluebuttonbn->course);
    exit;
}

Should become: (no need for exit given that print_error throws exception)
if ($serverversion === null) {
    $errmsg = 'view_error_unable_join_student';
    $errurl = '/course/view.php';
    $errurlparams = ['id' => $bigbluebuttonbn->course];
    if ($bbbsession['administrator']) {
        $errmsg = 'view_error_unable_join';
        $errurl = '/admin/settings.php';
        $errurlparams = ['section' => 'modsettingbigbluebuttonbn'];
    } else if ($bbbsession['moderator']) {
        $errmsg = 'view_error_unable_join_teacher';
    }
    print_error($errmsg, 'mod_bigbluebuttonbn', new moodle_url($errurl, $errurlparams));
}

Step 4 - standard page setting
// Print the page header.
$PAGE->set_context($context);
$PAGE->set_url($CFG->wwwroot.'/mod/bigbluebuttonbn/view.php', array('id' => $cm->id));
$PAGE->set_title(format_string($bigbluebuttonbn->name));
$PAGE->set_cacheable(false);
$PAGE->set_heading($course->fullname);
$PAGE->set_pagelayout('incourse');

Becomes:
// Print the page header.
$PAGE->set_context($context);
$PAGE->set_url('/mod/bigbluebuttonbn/view.php', array('id' => $cm->id)); // Relative url is accepted here.
$PAGE->set_title($bigbluebuttonbn->name); // No need for format_string, given that it is already executed in set_title.
$PAGE->set_cacheable(false);
$PAGE->set_heading($course->fullname);
$PAGE->set_pagelayout('incourse');

Step 5 - optimizing checks
// Validate if the user is in a role allowed to join.
if (!has_capability('moodle/category:manage', $context) && !has_capability('mod/bigbluebuttonbn:join', $context)) {
    echo $OUTPUT->header();
    if (isguestuser()) {
        echo $OUTPUT->confirm('<p>'.get_string('view_noguests', 'bigbluebuttonbn').'</p>'.get_string('liketologin'),
            get_login_url(), $CFG->wwwroot.'/course/view.php?id='.$course->id);
    } else {
        echo $OUTPUT->confirm('<p>'.get_string('view_nojoin', 'bigbluebuttonbn').'</p>'.get_string('liketologin'),
            get_login_url(), $CFG->wwwroot.'/course/view.php?id='.$course->id);
    }
    echo $OUTPUT->footer();
    exit;
}

Becomes:
// Validate if the user is in a role allowed to join.
if (!has_all_capabilities(['moodle/category:manage', 'mod/bigbluebuttonbn:join'], $context)) {
    echo $OUTPUT->header();
    $msgtext = isguestuser() ? 'view_noguests' : 'view_nojoin';
    echo $OUTPUT->confirm(
        sprintf('<p>%s</p>%s'), get_string($msgtext, 'mod_bigbluebuttonbn', get_string('liketologin')),
        get_login_url(),
        new moodle_url('/course/view.php', ['id' => $course->id])
    );
    echo $OUTPUT->footer();
    exit;
}

Step 6 - URL generation
// Operation URLs.
$bbbsession['bigbluebuttonbnURL'] = $CFG->wwwroot . '/mod/bigbluebuttonbn/view.php?id=' . $bbbsession['cm']->id;
$bbbsession['logoutURL'] = $CFG->wwwroot . '/mod/bigbluebuttonbn/bbb_view.php?action=logout&id='.$id .
    '&bn=' . $bbbsession['bigbluebuttonbn']->id;
$bbbsession['recordingReadyURL'] = $CFG->wwwroot . '/mod/bigbluebuttonbn/bbb_broker.php?action=recording_' .
    'ready&bigbluebuttonbn=' . $bbbsession['bigbluebuttonbn']->id;
$bbbsession['meetingEventsURL'] = $CFG->wwwroot . '/mod/bigbluebuttonbn/bbb_broker.php?action=meeting' .
    '_events&bigbluebuttonbn=' . $bbbsession['bigbluebuttonbn']->id;
$bbbsession['joinURL'] = $CFG->wwwroot . '/mod/bigbluebuttonbn/bbb_view.php?action=join&id=' . $id .
    '&bn=' . $bbbsession['bigbluebuttonbn']->id;

Becomes:
$bbbsession['bigbluebuttonbnURL'] = new moodle_url('/mod/bigbluebuttonbn/view.php', ['id' => $bbbsession['cm']->id]);
$bbbsession['logoutURL'] = new moodle_url(
    '/mod/bigbluebuttonbn/bbb_view.php',
    ['action' => 'logout', 'id' => $id, 'bn' => $bbbsession['bigbluebuttonbn']->id]
);
$bbbsession['recordingReadyURL'] = new moodle_url(
    '/mod/bigbluebuttonbn/bbb_broker.php',
    ['action' => 'recording_ready', 'bigbluebuttonbn' => $bbbsession['bigbluebuttonbn']->id]
);
$bbbsession['meetingEventsURL'] = new moodle_url(
    '/mod/bigbluebuttonbn/bbb_broker.php',
    ['action' => 'meeting_events', 'bigbluebuttonbn' => $bbbsession['bigbluebuttonbn']->id]
);
$bbbsession['joinURL'] = new moodle_url(
    '/mod/bigbluebuttonbn/bbb_view.php',
    ['action' => 'join', 'id' => $id, 'bn' => $bbbsession['bigbluebuttonbn']->id]
);

This is safer approach as all variables are properly encoded and checked and it is cleaner for dev maintennace.
Step 7 - Move all functions from view.php to new library file
This can be performed in several ways.
New include file
Move all functions as they are into new file viewlib.php. Add appropriate include to the view.php
Autoloaded class
Move all functions as they are into new abstract class located in classes/locallib/view.php.
Add appropriate use directive to the view.php
Example:
// Top of the view.php.
use mod_bigbluebuttonbn\locallib\view;

// Somewhere in the code.
view::bigbluebuttonbn_view_message_box(... parameters);

Step 8 - Refactor completely view.php to use Mustache template
This would be the ideal case that would require most of the work.
Walkthrough refactoring of mod/bigbluebuttonbn/index.php
Step 1 - cleaning includes
require_once(dirname(dirname(dirname(__FILE__))).'/config.php');
require_once(dirname(__FILE__).'/locallib.php');

Becomes:
require(__DIR__.'/../../../config.php');
require_once(__DIR__.'/locallib.php');

Step 2 - passing output to renderer
In this case given that html_table is being used it is structurally better to transform this into renderable.


Create class mod_bigbluebuttonbn\output\index that will implement renderable
  namespace mod_bigbluebuttonbn\output;
  
  use renderable;
  use html_table;
  use html_writer;
  use stdClass;
  use coding_exception;
  
  defined('MOODLE_INTERNAL') || die();
  
  require_once($CFG->dirroot.'/mod/bigbluebuttonbn/locallib.php');
  
  class index implements renderable {
  
      /** @var html_table */
      public $table = null;
  
      /**
       * index constructor.
       * @param  stdClass $course
       * @throws coding_exception
       */
      public function __construct($course) {
          global $DB;
  
          // Get all the appropriate data.
          if (!$bigbluebuttonbns = get_all_instances_in_course('bigbluebuttonbn', $course)) {
              notice('There are no instances of bigbluebuttonbn', "../../course/view.php?id=$course->id");
          }
  
          // Print the list of instances.
          $timenow = time();
          $strweek = get_string('week');
          $strtopic = get_string('topic');
          $headingname = get_string('index_heading_name', 'bigbluebuttonbn');
          $headinggroup = get_string('index_heading_group', 'bigbluebuttonbn');
          $headingusers = get_string('index_heading_users', 'bigbluebuttonbn');
          $headingviewer = get_string('index_heading_viewer', 'bigbluebuttonbn');
          $headingmoderator = get_string('index_heading_moderator', 'bigbluebuttonbn');
          $headingactions = get_string('index_heading_actions', 'bigbluebuttonbn');
          $headingrecording = get_string('index_heading_recording', 'bigbluebuttonbn');
  
          $table = new html_table();
          $table->head = array($strweek, $headingname, $headinggroup, $headingusers, $headingviewer, $headingmoderator,
              $headingrecording, $headingactions);
          $table->align = array('center', 'left', 'center', 'center', 'center', 'center', 'center');
  
          foreach ($bigbluebuttonbns as $bigbluebuttonbn) {
              if ($bigbluebuttonbn->visible) {
                  $cm = get_coursemodule_from_id('bigbluebuttonbn', $bigbluebuttonbn->coursemodule, 0, false, MUST_EXIST);
                  // User roles.
                  $participantlist = bigbluebuttonbn_get_participant_list($bigbluebuttonbn, $context);
                  $moderator = bigbluebuttonbn_is_moderator($context, $participantlist);
                  $administrator = is_siteadmin();
                  $canmoderate = ($administrator || $moderator);
                  // Add a the data for the bigbluebuttonbn instance.
                  $groupobj = null;
                  if (groups_get_activity_groupmode($cm) > 0) {
                      $groupobj = (object) array('id' => 0, 'name' => get_string('allparticipants'));
                  }
                  $table->data[] = self::bigbluebuttonbn_index_display_room($canmoderate, $course, $bigbluebuttonbn, $groupobj);
                  // Add a the data for the groups belonging to the bigbluebuttonbn instance, if any.
                  $groups = groups_get_activity_allowed_groups($cm);
                  foreach ($groups as $group) {
                      $table->data[] = self::bigbluebuttonbn_index_display_room($canmoderate, $course, $bigbluebuttonbn, $group);
                  }
              }
          }
  
          $this->table = $table;
      }
  
      /**
       * Displays the general view.
       *
       * @param boolean $moderator
       * @param object $course
       * @param object $bigbluebuttonbn
       * @param object $groupobj
       * @return array
       */
      public static function bigbluebuttonbn_index_display_room($moderator, $course, $bigbluebuttonbn, $groupobj = null) {
          $meetingid = $bigbluebuttonbn->meetingid.'-'.$course->id.'-'.$bigbluebuttonbn->id;
          $paramgroup = '';
          $groupname = '';
          if ($groupobj) {
              $meetingid .= '['.$groupobj->id.']';
              $paramgroup = '&group='.$groupobj->id;
              $groupname = $groupobj->name;
          }
          $meetinginfo = bigbluebuttonbn_get_meeting_info_array($meetingid);
          if (empty($meetinginfo)) {
              // The server was unreachable.
              print_error(get_string('index_error_unable_display', 'bigbluebuttonbn'));
              return;
          }
          if (isset($meetinginfo['messageKey']) && $meetinginfo['messageKey'] == 'checksumError') {
              // There was an error returned.
              print_error(get_string('index_error_checksum', 'bigbluebuttonbn'));
              return;
          }
          // Output Users in the meeting.
          $joinurl = '<a href="view.php?id='.$bigbluebuttonbn->coursemodule.$paramgroup.'">'.format_string($bigbluebuttonbn->name).'</a>';
          $group = $groupname;
          $users = '';
          $viewerlist = '';
          $moderatorlist = '';
          $recording = '';
          $actions = '';
          // The meeting info was returned.
          if (array_key_exists('running', $meetinginfo) && $meetinginfo['running'] == 'true') {
              $users = self::bigbluebuttonbn_index_display_room_users($meetinginfo);
              $viewerlist = self::bigbluebuttonbn_index_display_room_users_attendee_list($meetinginfo, 'VIEWER');
              $moderatorlist = self::bigbluebuttonbn_index_display_room_users_attendee_list($meetinginfo, 'MODERATOR');
              $recording = self::bigbluebuttonbn_index_display_room_recordings($meetinginfo);
              $actions = self::bigbluebuttonbn_index_display_room_actions($moderator, $course, $bigbluebuttonbn, $groupobj);
          }
          return array($bigbluebuttonbn->section, $joinurl, $group, $users, $viewerlist, $moderatorlist, $recording, $actions);
      }
  
      /**
       * Count the number of users in the meeting.
       *
       * @param array $meetinginfo
       * @return integer
       */
      public static function bigbluebuttonbn_index_display_room_users($meetinginfo) {
          $users = '';
          if (count($meetinginfo['attendees']) && count($meetinginfo['attendees']->attendee)) {
              $users = count($meetinginfo['attendees']->attendee);
          }
          return $users;
      }
  
      /**
       * Returns attendee list.
       *
       * @param array $meetinginfo
       * @param string $role
       * @return string
       */
      public static function bigbluebuttonbn_index_display_room_users_attendee_list($meetinginfo, $role) {
          $attendeelist = '';
          if (count($meetinginfo['attendees']) && count($meetinginfo['attendees']->attendee)) {
              $attendeecount = 0;
              foreach ($meetinginfo['attendees']->attendee as $attendee) {
                  if ($attendee->role == $role) {
                      $attendeelist .= ($attendeecount++ > 0 ? ', ' : '').$attendee->fullName;
                  }
              }
          }
          return $attendeelist;
      }
  
      /**
       * Returns indication of recording enabled.
       *
       * @param array $meetinginfo
       * @return string
       */
      public static function bigbluebuttonbn_index_display_room_recordings($meetinginfo) {
          $recording = '';
          if (isset($meetinginfo['recording']) && $meetinginfo['recording'] === 'true') {
              // If it has been set when meeting created, set the variable on/off.
              $recording = get_string('index_enabled', 'bigbluebuttonbn');
          }
          return $recording;
      }
  
      /**
       * Returns room actions.
       *
       * @param boolean $moderator
       * @param object $course
       * @param object $bigbluebuttonbn
       * @param object $groupobj
       * @return string
       */
      public static function bigbluebuttonbn_index_display_room_actions($moderator, $course, $bigbluebuttonbn, $groupobj = null) {
          $actions = '';
          if ($moderator) {
              $actions .= '<form name="form1" method="post" action="">'."\n";
              $actions .= '  <INPUT type="hidden" name="id" value="'.$course->id.'">'."\n";
              $actions .= '  <INPUT type="hidden" name="a" value="'.$bigbluebuttonbn->id.'">'."\n";
              if ($groupobj != null) {
                  $actions .= '  <INPUT type="hidden" name="g" value="'.$groupobj->id.'">'."\n";
              }
              $actions .= '  <INPUT type="submit" name="submit" value="' .
                  get_string('view_conference_action_end', 'bigbluebuttonbn') .
                  '" class="btn btn-primary btn-sm" onclick="return confirm(\'' .
                  get_string('index_confirm_end', 'bigbluebuttonbn') . '\')">' . "\n";
              $actions .= '</form>'."\n";
          }
          return $actions;
      }
  }


Create class mod_bigbluebuttonbn\output\renderer that will implement rendering
  namespace mod_bigbluebuttonbn\output;
  
  use html_writer;
  use html_table;
  use plugin_renderer_base;
  
  defined('MOODLE_INTERNAL') || die();
  
  class renderer extends plugin_renderer_base {
  
      /**
       * @param  index $indexobj
       * @return string
       */
      protected function render_index(index $indexobj) {
          return html_writer::table($indexobj->table);
      }
  
  }


Alter the index.php to look more or less like this:
  require(__DIR__.'/../../../config.php');
  require_once(__DIR__.'/locallib.php');
  
  $id = required_param('id', PARAM_INT);
  $a = optional_param('a', 0, PARAM_INT);
  $g = optional_param('g', 0, PARAM_INT);
  
  if (!$id) {
      print_error('You must specify a course_module ID or an instance ID');
      return;
  }
  
  $course = $DB->get_record('course', array('id' => $id), '*', MUST_EXIST);
  
  require_login($course, true);
  
  $context = context_course::instance($course->id);
  
  // Print the header.
  $PAGE->set_url('/mod/bigbluebuttonbn/index.php', array('id' => $id));
  $PAGE->set_title(get_string('modulename', 'bigbluebuttonbn'));
  $PAGE->set_heading($course->fullname);
  $PAGE->set_cacheable(false);
  $PAGE->set_pagelayout('incourse');
  
  $PAGE->navbar->add(get_string('modulename', 'bigbluebuttonbn'), "index.php?id=$course->id");
  
  $submit = optional_param('submit', '', PARAM_TEXT);
  if ($submit === 'end') {
      // A request to end the meeting.
      $bigbluebuttonbn = $DB->get_record('bigbluebuttonbn', array('id' => $a), '*', MUST_EXIST);
      if (!$bigbluebuttonbn) {
          print_error("BigBlueButton ID $a is incorrect");
      }
      $course = $DB->get_record('course', array('id' => $bigbluebuttonbn->course), '*', MUST_EXIST);
      $cm = get_coursemodule_from_instance('bigbluebuttonbn', $bigbluebuttonbn->id, $course->id, false, MUST_EXIST);
      // User roles.
      $participantlist = bigbluebuttonbn_get_participant_list($bigbluebuttonbn, $context);
      $moderator = bigbluebuttonbn_is_moderator($context, $participantlist);
      $administrator = is_siteadmin();
      if ($moderator || $administrator) {
          bigbluebuttonbn_event_log(\mod_bigbluebuttonbn\event\events::$events['meeting_end'], $bigbluebuttonbn);
          echo get_string('index_ending', 'bigbluebuttonbn');
          $meetingid = $bigbluebuttonbn->meetingid.'-'.$course->id.'-'.$bigbluebuttonbn->id;
          if ($g != '0') {
              $meetingid .= '['.$g.']';
          }
  
          bigbluebuttonbn_end_meeting($meetingid, $bigbluebuttonbn->moderatorpass);
          redirect('index.php?id='.$id);
      }
  }
  
  /** @var mod_bigbluebuttonbn\output\renderer $renderer */
  $renderer = $PAGE->get_renderer('mod_bigbluebuttonbn');
  
  echo $OUTPUT->header();
  echo $OUTPUT->heading(get_string('index_heading', 'bigbluebuttonbn'));
  echo $renderer->render(new \mod_bigbluebuttonbn\output\index($course));
  echo $OUTPUT->footer();