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

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

    Details

      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).

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            salvetore Michael de Raadt added a comment -

            Well spotted, Ankit.

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

            well it was Sam who pointed me in this direction

            Show
            ankit_frenz Ankit Agarwal added a comment - well it was Sam who pointed me in this direction
            Hide
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - thanks for the report, I was planning to do that for years
            Hide
            poltawski 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
            poltawski 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
            salvetore 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
            salvetore 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
            poltawski 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
            poltawski 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
            poltawski 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
            poltawski 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
            salvetore 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
            salvetore 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_frenz 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_frenz Ankit Agarwal added a comment - I did functional testing in a lot of places affected by this patch and all looks good. Thanks
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  25/Jun/12