Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-43049

Typo in accesslib causes performance optimization to be skipped

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.6
    • Fix Version/s: 2.7
    • Component/s: Roles / Access
    • Labels:

      Description

      There appears to be a typo in get_user_accessdata() in lib/accesslib.php, L1116-1122:

          if (!empty($USER->acces['rdef']) and empty($ACCESSLIB_PRIVATE->rolepermissions)) {
              // share rdef from USER session with rolepermissions cache in order to conserve memory
              foreach($USER->acces['rdef'] as $k=>$v) {
                  $ACCESSLIB_PRIVATE->rolepermissions[$k] =& $USER->acces['rdef'][$k];
              }
              $ACCESSLIB_PRIVATE->accessdatabyuser[$USER->id] = $USER->acces;
          }
      

      I believe it should be $USER->access, not $USER->acces (all four references in this block).

      Because of the first condition the effect of this typo is that this block of code above is never run. While not necessary for the function to work it is there to save memory so assuming that the code is otherwise correct fixing the typo should reduce memory consumption.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            skodak Petr Skoda added a comment -

            Oooops, my fault, +1 to include it in master after the on-sync period. I suppose it would be dangerous for stables. Thanks a lot.

            Show
            skodak Petr Skoda added a comment - Oooops, my fault, +1 to include it in master after the on-sync period. I suppose it would be dangerous for stables. Thanks a lot.
            Hide
            poltawski Dan Poltawski added a comment -

            This seems like a bug and should be backported to the stable branches.

            Show
            poltawski Dan Poltawski added a comment - This seems like a bug and should be backported to the stable branches.
            Hide
            skodak Petr Skoda added a comment -

            It is not a bug, it works fine without this change, the risk of regression is too big, sorry, no backports.

            Show
            skodak Petr Skoda added a comment - It is not a bug, it works fine without this change, the risk of regression is too big, sorry, no backports.
            Hide
            poltawski Dan Poltawski added a comment -

            Petr, if the risk of regressions is so big then how should we test this?

            Show
            poltawski Dan Poltawski added a comment - Petr, if the risk of regressions is so big then how should we test this?
            Hide
            skodak Petr Skoda added a comment - - edited

            If I knew how to test it it would not be risky, right? This may fail especially when switching users on the fly such as in cron.
            Maybe this could also affect results when verifying permissions for other user than $USER.

            Show
            skodak Petr Skoda added a comment - - edited If I knew how to test it it would not be risky, right? This may fail especially when switching users on the fly such as in cron. Maybe this could also affect results when verifying permissions for other user than $USER.
            Hide
            dougiamas Martin Dougiamas added a comment -

            I really don't see how this could be such a big regression risk ... how about delaying backports just for a month or so?

            Show
            dougiamas Martin Dougiamas added a comment - I really don't see how this could be such a big regression risk ... how about delaying backports just for a month or so?
            Hide
            skodak Petr Skoda added a comment -

            +1 for delayed backports

            Show
            skodak Petr Skoda added a comment - +1 for delayed backports
            Hide
            poltawski Dan Poltawski added a comment -

            I've integrated into master and i'll open a backport request.

            (My point though, is I don't quite see how time helps here and if we're not sure, we shouldn't be doing it )

            Show
            poltawski Dan Poltawski added a comment - I've integrated into master and i'll open a backport request. (My point though, is I don't quite see how time helps here and if we're not sure, we shouldn't be doing it )
            Hide
            poltawski Dan Poltawski added a comment -

            Seems ok (although work continues with me using the performance analyser )

            Show
            poltawski Dan Poltawski added a comment - Seems ok (although work continues with me using the performance analyser )
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your contributions, this change is now upstream!

            “ If debugging is the process of removing software bugs, then programming must be the process of putting them in. ” - Edsger Dijkstra

            Show
            poltawski Dan Poltawski added a comment - Thanks for your contributions, this change is now upstream! “ If debugging is the process of removing software bugs, then programming must be the process of putting them in. ” - Edsger Dijkstra

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  12/May/14