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

          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