Moodle
  1. Moodle
  2. MDL-43746

require_course_login: incorrect variable assignment

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.9, 2.4.6, 2.5.2, 2.6
    • Fix Version/s: 2.4.9, 2.5.5, 2.6.2
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      this cannot be tested in unittest, so you neeed to create some hacky script and test it manually:

      <?php
       
      require 'config.php';
       
      // Following two should not redirect to login page.
      //require_course_login($SITE);
      //require_course_login($SITE->id);
       
      // The next two should redirect to login page.
       
      $course = $DB->get_record('course', array('id'=>2));
      //require_course_login($course);
      //require_course_login($course->id);
       
      var_dump($USER->id);
      

      Show
      this cannot be tested in unittest, so you neeed to create some hacky script and test it manually: <?php   require 'config.php';   // Following two should not redirect to login page. //require_course_login($SITE); //require_course_login($SITE->id);   // The next two should redirect to login page.   $course = $DB->get_record('course', array('id'=>2)); //require_course_login($course); //require_course_login($course->id);   var_dump($USER->id);
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.6 Branch:
      w04_MDL-43746_m26_courselogin
    • Pull Master Branch:
      w04_MDL-43746_m27_courselogin
    • Story Points (Obsolete):
      2
    • Sprint:
      BACKEND Sprint 9

      Description

      In function require_course_login (/lib/moodlelib.php) $issite is never true if parameter $courseorid is not an object.

      $issite = (is_object($courseorid) and $courseorid->id == SITEID)
                or (!is_object($courseorid) and $courseorid == SITEID);

      is not working correctly because = has a higher precedence than and/or in PHP. It should be:

      $issite = (is_object($courseorid) && $courseorid->id == SITEID)
                || (!is_object($courseorid) && $courseorid == SITEID);

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Petr Skoda added a comment -

            Thanks a lot for the report.

            Show
            Petr Skoda added a comment - Thanks a lot for the report.
            Hide
            CiBoT added a comment -
            Show
            CiBoT added a comment - Results for MDL-43746 Remote repository: https://github.com/skodak/moodle.git Remote branch w03_ MDL-43746 _m24_courselogin to be integrated into upstream MOODLE_24_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/685 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/685/artifact/work/smurf.html Remote branch w03_ MDL-43746 _m25_courselogin to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/686 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/686/artifact/work/smurf.html Remote branch w03_ MDL-43746 _m26_courselogin to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/687 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/687/artifact/work/smurf.html Remote branch w03_ MDL-43746 _m27_courselogin to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/688 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/688/artifact/work/smurf.html
            Hide
            Frédéric Massart added a comment -

            +1 if you switch to && and ||!

            Show
            Frédéric Massart added a comment - +1 if you switch to && and ||!
            Hide
            Petr Skoda added a comment -

            no!

            Show
            Petr Skoda added a comment - no!
            Hide
            Eloy Lafuente (stronk7) 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
            Eloy Lafuente (stronk7) 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
            Jan Eberhardt added a comment -

            Ehmm... why "no!"?

            What's the point of it. "or" and "and" just leads to missinterpreted code and as far as I can see there's no point in insisting to leave it that way.

            So: Bump! And +1 if you switch to && and || !

            Show
            Jan Eberhardt added a comment - Ehmm... why "no!"? What's the point of it. "or" and "and" just leads to missinterpreted code and as far as I can see there's no point in insisting to leave it that way. So: Bump! And +1 if you switch to && and || !
            Hide
            Petr Skoda added a comment -

            no because some people are confused by AND and others by &&, both of them may lead to unexpected results if you do not know how to use them

            anyway my original if was using AND/OR before it was incorrectly refactored to = without the outer brackets

            Show
            Petr Skoda added a comment - no because some people are confused by AND and others by &&, both of them may lead to unexpected results if you do not know how to use them anyway my original if was using AND/OR before it was incorrectly refactored to = without the outer brackets
            Hide
            Jan Eberhardt added a comment -

            But the binding rules of "and" and "or" aren't that obvious and the behavior of || and && are in most programming languages the same, so that people, which are used to other languages may not know this issue.

            For that reason it's much more possible, that this will become an issue a second and a third time and ever when this line is edited... one proof of that is, that it happened (at least) once.

            Show
            Jan Eberhardt added a comment - But the binding rules of "and" and "or" aren't that obvious and the behavior of || and && are in most programming languages the same, so that people, which are used to other languages may not know this issue. For that reason it's much more possible, that this will become an issue a second and a third time and ever when this line is edited... one proof of that is, that it happened (at least) once.
            Hide
            Petr Skoda added a comment -

            or the other way around, if developers do not learn the difference between or and || they will be more likely to do similar mistake once they have to deal with and/or next time

            anyway we have already enough stupid coding style rules, please let's not pollute the guidelines with one more crazy rule, there are many weird things in PHP and Moodle coding style - learn to live with them

            Show
            Petr Skoda added a comment - or the other way around, if developers do not learn the difference between or and || they will be more likely to do similar mistake once they have to deal with and/or next time anyway we have already enough stupid coding style rules, please let's not pollute the guidelines with one more crazy rule, there are many weird things in PHP and Moodle coding style - learn to live with them
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Sam Hemelryk added a comment -

            Thanks Petr - this has been integrated now.

            Just noting that the use of `|| && or and` has been discussed re: our coding style and the decision has been made to NOT enforce any policy on it in favour of producing clear statements as Petr has done here by encompassing the whole statement.

            Show
            Sam Hemelryk added a comment - Thanks Petr - this has been integrated now. Just noting that the use of `|| && or and` has been discussed re: our coding style and the decision has been made to NOT enforce any policy on it in favour of producing clear statements as Petr has done here by encompassing the whole statement.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            I'm sorry but this should not be backported to 24_STABLE. This is a very old bug and that branch is out of support, only accepting security issues. So my +1 to rewrite, getting rid of it there. Feel free to discuss it, integrators.

            Show
            Eloy Lafuente (stronk7) added a comment - I'm sorry but this should not be backported to 24_STABLE. This is a very old bug and that branch is out of support, only accepting security issues. So my +1 to rewrite, getting rid of it there. Feel free to discuss it, integrators.
            Hide
            Mark Nelson added a comment -

            Works as expected, passing. Thanks Petr.

            Show
            Mark Nelson added a comment - Works as expected, passing. Thanks Petr.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Side note: As commented above, the 24_STABLE fix has been deleted and never happened.

            Show
            Eloy Lafuente (stronk7) added a comment - Side note: As commented above, the 24_STABLE fix has been deleted and never happened.
            Hide
            Marina Glancy added a comment -

            Thanks for your hard work. Your code has now become a part of Moodle!

            Show
            Marina Glancy added a comment - Thanks for your hard work. Your code has now become a part of Moodle!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Agile