Moodle
  1. Moodle
  2. MDL-35787

PHP warnings while enrolling students to remote course and accessing remote course

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.3.3
    • Component/s: MNet
    • Labels:
    • Testing Instructions:
      Hide

      Pre-requisite:

      1. Two sites with mnet
      2. Minimum one course should have mnet enrolment enabled.

      Test 1

      1. Log in as admin to site which has mnet site
      2. Go to Remote enrolments client (Site administration -> Networking -> Remote enrolments client)
      3. Click "Edit enrolments" next to mnet site information
      4. Click "Edit enrolments" next to remote course
      5. No notice/warning should be visible

      Test 2:

      1. Enrol user 1 in remote course
      2. Login as user1 and go to my home page
      3. Click on remote course and you should be redirected normally (without any php warning)
      Show
      Pre-requisite: Two sites with mnet Minimum one course should have mnet enrolment enabled. Test 1 Log in as admin to site which has mnet site Go to Remote enrolments client (Site administration -> Networking -> Remote enrolments client) Click "Edit enrolments" next to mnet site information Click "Edit enrolments" next to remote course No notice/warning should be visible Test 2: Enrol user 1 in remote course Login as user1 and go to my home page Click on remote course and you should be redirected normally (without any php warning)
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-mdl-35787
    • Rank:
      44541

      Description

      Warning encountered while enrolling students in remote course
      Strict standards: Creating default object from empty value in /var/www/im/mnet/service/enrol/course.php on line 66 Call Stack: 0.0004 755464 1.

      {main}() /var/www/im/mnet/service/enrol/course.php:0
      Steps to reproduce:
      # Log in as admin to site which has mnet site
      # Go to Remote enrolments client (Site administration -> Networking -> Remote enrolments client)
      # Click "Edit enrolments" next to mnet site information
      # Click "Edit enrolments" next to remote course
      # Above warning is visble.

      Warning encountered while accessing remote course
      Strict standards: Only variables should be passed by reference in /var/www/m/auth/mnet/auth.php on line 381 Call Stack: 0.0002 666248 1. {main}

      () /var/www/m/auth/mnet/land.php:0 0.0951 30780144 2. auth_plugin_mnet->confirm_mnet_session() /var/www/m/auth/mnet/land.php:39
      Steps to reproduce:

      1. Enrol user 1 in remote course
      2. Login as user1 and go to my home page
      3. Click on remote course and you will see above warning.

        Activity

        Hide
        Ankit Agarwal added a comment -

        Hi Rajesh,
        The patch looks great. Just a small issue is that $studentroles can be false, which is going to create notices when used with reset().

        Feel free to push for integration once you have fixed that.
        Thanks

        Show
        Ankit Agarwal added a comment - Hi Rajesh, The patch looks great. Just a small issue is that $studentroles can be false, which is going to create notices when used with reset(). Feel free to push for integration once you have fixed that. Thanks
        Hide
        Rajesh Taneja added a comment -

        Thanks for the review Ankit,

        I don't see any possibility of user getting authenticated if user is not student in that course. Hence checking for role existence is not important. Although it doesn't harm to check for for the same. Added another commit to throw exception if student role is not found.

        FYI:
        If user is enrolled in a remote course (mnet) as student, then only he/she will see course on my home page and can access the course as student only. (Patch has an old comment which is not altered)

        Show
        Rajesh Taneja added a comment - Thanks for the review Ankit, I don't see any possibility of user getting authenticated if user is not student in that course. Hence checking for role existence is not important. Although it doesn't harm to check for for the same. Added another commit to throw exception if student role is not found. FYI: If user is enrolled in a remote course (mnet) as student, then only he/she will see course on my home page and can access the course as student only. (Patch has an old comment which is not altered)
        Hide
        Dan Poltawski added a comment -

        The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.
        This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody.
        This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
        Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao

        Show
        Dan Poltawski added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologizes for the inconvenient, this will be integrated next week. Thanks for your collaboration & ciao
        Hide
        Aparup Banerjee added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Dan Poltawski added a comment -

        The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

        TIA and ciao

        Show
        Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
        Hide
        Dan Poltawski added a comment -

        Thanks Raj, I've integrated this now.

        I'm wasn't 100% convinced by the new exception you introduced, because not having a role with archetype of student is a completely possible and acceptable situation and shouldn't be throwing an exception.

        But really this is a result of the underlying brokeness of this whole functionality, it shouldn't be 'guessing' who to enrol as a student like this, so i accepted your basic fix.

        Show
        Dan Poltawski added a comment - Thanks Raj, I've integrated this now. I'm wasn't 100% convinced by the new exception you introduced, because not having a role with archetype of student is a completely possible and acceptable situation and shouldn't be throwing an exception. But really this is a result of the underlying brokeness of this whole functionality, it shouldn't be 'guessing' who to enrol as a student like this, so i accepted your basic fix.
        Hide
        Dan Poltawski added a comment -

        Oh, I have just realised the error message you've used is misleading though.

        "Unknown role 'student'" is incorrect.

        The error is that there is no role with archetype student. Perhaps you could improve this and also add testing instructions for this scenario to be triggered.

        Show
        Dan Poltawski added a comment - Oh, I have just realised the error message you've used is misleading though. "Unknown role 'student'" is incorrect. The error is that there is no role with archetype student. Perhaps you could improve this and also add testing instructions for this scenario to be triggered.
        Hide
        Rajesh Taneja added a comment -

        Thanks Dan,

        AFAIR we have hard-coded check for student role and that's why I added this message. I will check this tomorrow and will correct this if required.

        Show
        Rajesh Taneja added a comment - Thanks Dan, AFAIR we have hard-coded check for student role and that's why I added this message. I will check this tomorrow and will correct this if required.
        Hide
        David Monllaó added a comment -

        Tested with a 23 and master, both latest from integration.git I've set up the environment following MDocs MNet page and I've followed the testing instructions without receiving any error nor debugging message.

        Show
        David Monllaó added a comment - Tested with a 23 and master, both latest from integration.git I've set up the environment following MDocs MNet page and I've followed the testing instructions without receiving any error nor debugging message.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

        (not really)

        Closing, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: