Problem/Motivation

When a course is set up in free navigation, and you skip over two lessons, TrainingManager::getBackNavDeltas attempts to call a method on a NULL object.

Steps to reproduce

Set up a multi-lesson, multi-activity course.
Set it as free navigation.
Take the course, skip over two consecutive lessons you've never taken before.

Proposed resolution

Add additional NULL checking in the method.

Remaining tasks

Create patch.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#2 3571120-getBackNavDeltas-2.patch684 bytessocketwench

Issue fork lms-3571120

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

socketwench created an issue. See original summary.

socketwench’s picture

socketwench commented
Status: Active » Needs review
StatusFileSize
new 3571120-getBackNavDeltas-2.patch684 bytes

This patch adds further NULL checking for the method. From testing, passing 0 here on a free navigation course appears to work. I'm not sure if it's the appropriate fix or not.

graber’s picture

graber commented
Status: Needs review » Needs work

That redirects to the first activity of the previous lesson instead of the last one. We need to initialize the lesson if the status is NULL instead. Can you update the solution and create a MR?
Should be something like:

      if ($previous_lesson_status === NULL) {
        try {
          $previous_lesson_status = $this->initializeLesson($course_status, $lesson_item);
        }
        catch (TrainingException $e) {
          // If the previous lesson couldn't be initialized, backwards navigation is not possible.
          return [];
        }
      }

We should probably also just return early in LmsLessonHandlerBase::checkRequirements() if $course_status->getCourse()->hasFreeNavigation(). Would probably be nice to inform course creators that the lesson reference mandatory setting has no effect in free navigation mode..

socketwench opened merge request !143

socketwench’s picture

socketwench commented
Status: Needs work » Needs review

Changed this to a try/catch with a fast return on the recommendation of Graber.

Also added a fast return for LMSLessonHandlerBase::checkRequirements in the case the course has free navigation.

graber’s picture

graber commented
Status: Needs review » Needs work

Left some comments in the MR.

graber’s picture

graber commented
Status: Needs work » Reviewed & tested by the community

  • graber committed 65e62f4a on 1.1.x authored by socketwench 
    Issue #3571120: TrainingManager::getBackNavDeltas breaks in free...
graber’s picture

graber commented
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

lv46gl’s picture

lv46gl commented

Dear Graber!

Solution #3 is good, but after setting the value of $previous_lesson_status, $deltas['activity'] will not be calculated, so we get an error at the end of the method:
Warning: Undefined array key "activity" message in Drupal\lms\TrainingManager->getBackNavDeltas()

The function currently looks like this:

    if ($deltas['current_activity'] === 0) {
      $lesson_item = $course_status->getCourse()->getLessonItem($deltas['lesson']);
      if ($lesson_item === NULL) {
        return [];
      }
      $previous_lesson_status = $this->loadLessonStatus($course_status->id(), $lesson_item->target_id);
      if ($previous_lesson_status === NULL) {
        try {
          $previous_lesson_status = $this->initializeLesson($course_status, $lesson_item);
        }
        catch (TrainingException $e) {
          // Backwards navigation is not possible
          // if the previous lesson couldn't be initialized.
          return [];
        }
      }
      else {
        $deltas['activity'] = $previous_lesson_status->getActivityCount() - 1;
      }
    }
    else {
      $deltas['activity'] = $deltas['current_activity'] - 1;
    }

    $reason = $this->checkActivityAccess($course_status, $deltas);
    return $reason !== NULL ? [] : [
      'lesson' => $deltas['lesson'],
      'activity' => $deltas['activity'],
    ];

Perhaps it should be more like this:

    if ($deltas['current_activity'] === 0) {
      $lesson_item = $course_status->getCourse()->getLessonItem($deltas['lesson']);
      if ($lesson_item === NULL) {
        return [];
      }
      $previous_lesson_status = $this->loadLessonStatus($course_status->id(), $lesson_item->target_id);
      if ($previous_lesson_status === NULL) {
        try {
          $previous_lesson_status = $this->initializeLesson($course_status, $lesson_item);
        }
        catch (TrainingException $e) {
          // Backwards navigation is not possible
          // if the previous lesson couldn't be initialized.
          return [];
        }
        $deltas['activity'] = 0;
      }
      else {
        $deltas['activity'] = $previous_lesson_status->getActivityCount() - 1;
      }
    }
    else {
      $deltas['activity'] = $deltas['current_activity'] - 1;
    }

    $reason = $this->checkActivityAccess($course_status, $deltas);
    return $reason !== NULL ? [] : [
      'lesson' => $deltas['lesson'],
      'activity' => $deltas['activity'],
    ];
leeksoup’s picture

leeksoup commented

I am getting this error as well.

#12 suggests adding
$deltas['activity'] = 0;
right below the try / catch.

Is the right or reasonable fix?