Moodle
  1. Moodle
  2. MDL-31607

Review all instances of require_login() to use course object instead of course id

    Details

    • Rank:
      38173

      Description

      In many places we already have course object fetched but still pass $course->id as argument to require_login().
      This leads to an extra un-necessary call to DB. All uses to require_login() should be reviewed and course object should be used as argument whenever possible.

      attaching a basic grep output of such cases(should not be considered conclusive).

        Activity

        Hide
        Michael de Raadt added a comment -

        Well spotted, Ankit.

        Show
        Michael de Raadt added a comment - Well spotted, Ankit.
        Hide
        Ankit Agarwal added a comment -

        well it was Sam who pointed me in this direction

        Show
        Ankit Agarwal added a comment - well it was Sam who pointed me in this direction
        Hide
        Petr Škoda added a comment -

        thanks for the report, I was planning to do that for years

        Show
        Petr Škoda added a comment - thanks for the report, I was planning to do that for years
        Hide
        Dan Poltawski added a comment -

        Thanks, this has been integrated.

        Note I have reviewed a good sample of these changed lines and looks good. But I have not reviewed every single instance.

        I would like the tester to do the (dull) and thorough task of checking each file individually.

        Show
        Dan Poltawski added a comment - Thanks, this has been integrated. Note I have reviewed a good sample of these changed lines and looks good. But I have not reviewed every single instance. I would like the tester to do the (dull) and thorough task of checking each file individually.
        Hide
        Michael de Raadt added a comment -

        This sort of checking is not the role of a tester. Functional testing is a check of the user interface, not a peer-review.

        I'm disappointed that this change has made it into integration if we know that there is a need for peer review but we skipped it.

        Show
        Michael de Raadt added a comment - This sort of checking is not the role of a tester. Functional testing is a check of the user interface, not a peer-review. I'm disappointed that this change has made it into integration if we know that there is a need for peer review but we skipped it.
        Hide
        Dan Poltawski added a comment -

        Hi, actually I misread this issue and thought that Ankit had peer reviwed this. Sorry

        However I thought that final code review was a valid testing step for something which has no functional change.

        Show
        Dan Poltawski added a comment - Hi, actually I misread this issue and thought that Ankit had peer reviwed this. Sorry However I thought that final code review was a valid testing step for something which has no functional change.
        Hide
        Dan Poltawski added a comment -

        To be clear: I reviewed the diff in full. What I did not review is looking into every single file affected and ensuring that the course object was retrieved.

        Show
        Dan Poltawski added a comment - To be clear: I reviewed the diff in full. What I did not review is looking into every single file affected and ensuring that the course object was retrieved.
        Hide
        Michael de Raadt added a comment -

        As well as looking for unfound instances of the double DB check, I would like to see a good sample of the pages affected by this change functionally tested.

        Show
        Michael de Raadt added a comment - As well as looking for unfound instances of the double DB check, I would like to see a good sample of the pages affected by this change functionally tested.
        Hide
        Ankit Agarwal added a comment -

        I did functional testing in a lot of places affected by this patch and all looks good.
        Thanks

        Show
        Ankit Agarwal added a comment - I did functional testing in a lot of places affected by this patch and all looks good. Thanks
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This has been near becoming rejected, because it's not the best code you are able to produce.

        But, luckily, at the end, it has landed and has been spread to all repos out there.

        Many thanks and, don't forget it, keep improving your skills, you can!

        Closing, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - This has been near becoming rejected, because it's not the best code you are able to produce. But, luckily, at the end, it has landed and has been spread to all repos out there. Many thanks and, don't forget it, keep improving your skills, you can! Closing, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: