Moodle
  1. Moodle
  2. MDL-33071

Reorganise quiz library code to take advantage of class auto-loading

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Won't Fix
    • Affects Version/s: 2.3, 2.5
    • Fix Version/s: None
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      You reall need to do a quick test of all the quiz functionality, to ensure that nothing is broken.

      There is a reason this is being done early in the 2.6 development cycle

      Show
      You reall need to do a quick test of all the quiz functionality, to ensure that nothing is broken. There is a reason this is being done early in the 2.6 development cycle
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      40285

      Description

      There are now lots of different classes defined in the quiz, and at the moment they are all mixed up in the mod/quiz folder with the front-end scripts like view.php and attempt.php.

      It would be better to move them into mod/quiz/classes folder, with one php file per class.

      At the same time, try to make it possible to use constants like quiz_attempt::FINISHED in lib.php without having to include all of locallib.php.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          The code moves almost all the quiz classes into the mod/quiz/classes folder to take advantage of auto-loading.

          In order to do this, some classes have had to be re-named, and these are listed in upgrade.txt. Nothing there strikes me as a very big deal.

          A more serious problem are the classes that are currently in attemptlib.php. All those class names start 'quiz_' and many of them will be refereed to in renderers, so renaming them will be a big deal. I have not yet decided if it is worth the pain of renaming and moving them now, or whether it is more important to maintain backwards compatibility.

          One possibility would be to make a nothing class definition like

          abstract class quiz_attempt {}
          class mod_quiz_attempt extends quiz_attempt {
              // Existing class definition goes here.
          }
          

          I think it needs ot be that way around to make code like

          public function review_page(quiz_attempt $attemptobj, ...)
          

          continue to work.

          On the other hand, the other existing uses are references to constants like quiz_attempt::FINISHED. The point is that theme designers may well have referenes to these classes, and it would be polite not to break their themes.

          I am looking for suggestions from the peer reviewer about this.

          Show
          Tim Hunt added a comment - The code moves almost all the quiz classes into the mod/quiz/classes folder to take advantage of auto-loading. In order to do this, some classes have had to be re-named, and these are listed in upgrade.txt. Nothing there strikes me as a very big deal. A more serious problem are the classes that are currently in attemptlib.php. All those class names start 'quiz_' and many of them will be refereed to in renderers, so renaming them will be a big deal. I have not yet decided if it is worth the pain of renaming and moving them now, or whether it is more important to maintain backwards compatibility. One possibility would be to make a nothing class definition like abstract class quiz_attempt {} class mod_quiz_attempt extends quiz_attempt { // Existing class definition goes here. } I think it needs ot be that way around to make code like public function review_page(quiz_attempt $attemptobj, ...) continue to work. On the other hand, the other existing uses are references to constants like quiz_attempt::FINISHED. The point is that theme designers may well have referenes to these classes, and it would be polite not to break their themes. I am looking for suggestions from the peer reviewer about this.
          Hide
          Mary Evans added a comment -

          Tim,
          I take it that you are talking about PHP classes rather that CSS one as these are very different.

          I don't know of any themes to date that have quiz specific modifications, other than CSS.

          Show
          Mary Evans added a comment - Tim, I take it that you are talking about PHP classes rather that CSS one as these are very different. I don't know of any themes to date that have quiz specific modifications, other than CSS.
          Hide
          Tim Hunt added a comment -

          Sorry, yes. I mean PHP classes. The OU themes override these renderers, so I am making pain for myself, but that is OK.

          Show
          Tim Hunt added a comment - Sorry, yes. I mean PHP classes. The OU themes override these renderers, so I am making pain for myself, but that is OK.
          Hide
          Mary Evans added a comment -

          Is Sam Marsall Peer Reviewing this? Otherwise I would if I know what I was doing...it's just that I don't have a clue as to what to look out for.

          Show
          Mary Evans added a comment - Is Sam Marsall Peer Reviewing this? Otherwise I would if I know what I was doing...it's just that I don't have a clue as to what to look out for.
          Hide
          Mary Evans added a comment -

          I found a typo in first line of your changes.

          probalby => probably

          Show
          Mary Evans added a comment - I found a typo in first line of your changes. probalby => probably
          Hide
          Mary Evans added a comment -

          Just reviewing your changes and wondered if this line is actually correct or not:

          
          -  throw new moodle_quiz_exception($attemptobj->get_quizobj(), 'notyourattempt');
          +  throw new mod_quiz_exception($attemptobj->get_quizobj(), 'notyourattempt'); 
          
          

          I would have thought that this should still stay the same at the beggining only addin mod_ before quiz like so...

          
          - throw new moodle_quiz_exception($attemptobj->get_quizobj(), 'notyourattempt');
          
          + throw new moodle_mod_quiz_exception($attemptobj->get_quizobj(), 'notyourattempt');
          
          
          Show
          Mary Evans added a comment - Just reviewing your changes and wondered if this line is actually correct or not: - throw new moodle_quiz_exception($attemptobj->get_quizobj(), 'notyourattempt'); + throw new mod_quiz_exception($attemptobj->get_quizobj(), 'notyourattempt'); I would have thought that this should still stay the same at the beggining only addin mod_ before quiz like so... - throw new moodle_quiz_exception($attemptobj->get_quizobj(), 'notyourattempt'); + throw new moodle_mod_quiz_exception($attemptobj->get_quizobj(), 'notyourattempt');
          Hide
          Mary Evans added a comment -

          Line 23 of mod/quiz/addrandomform.php → mod/quiz/classes/add_random_form.php
          Comment line begins with typo...

          * For for 
          Show
          Mary Evans added a comment - Line 23 of mod/quiz/addrandomform.php → mod/quiz/classes/add_random_form.php Comment line begins with typo... * For for
          Hide
          Dan Poltawski added a comment -

          Hi Tim,

          1. I don't like removal of the constants in the settings file QUIZ_SHOWIMAGE_*, that seems pretty ugly to me. How about a constants.php and include it from both (I know thats not really in the autoloading spirit)?
          2. This is unrelated to your change and I can't get php to complain about it, but playing with my new toy complained about the use of $this in mod_quiz_access_manager::validate_settings_form_fields. Changing it to $quizform like in the base class would avoid confusion.

          Otherwise it all looks good to go!

          Show
          Dan Poltawski added a comment - Hi Tim, I don't like removal of the constants in the settings file QUIZ_SHOWIMAGE_* , that seems pretty ugly to me. How about a constants.php and include it from both (I know thats not really in the autoloading spirit)? This is unrelated to your change and I can't get php to complain about it, but playing with my new toy complained about the use of $this in mod_quiz_access_manager::validate_settings_form_fields . Changing it to $quizform like in the base class would avoid confusion. Otherwise it all looks good to go!
          Hide
          Tim Hunt added a comment -

          Thanks Dan.

          So, you are not worried about the backwards-compatbility implications of renaming the classes? I think it is worth the pain, but then I am probably biassed.

          1. ... I am not sure about a separate contants.php file. That is an extra file to include, so better to move them all to lib.php. Except that, there is a sense in which anything in lib.php is implied to be part of the public API of the quiz. Are some of these constants implementation details? Well, things used in settings are part of the public API. Those values get into backup files, and people need to use the values when creating a quiz using web services. OK, I have convinced myself. -> lib.php.

          3. I should probably update the collatorlib references while I am at it. And get_plugin_list_with_file, since there are now more modern alternatives.

          Show
          Tim Hunt added a comment - Thanks Dan. So, you are not worried about the backwards-compatbility implications of renaming the classes? I think it is worth the pain, but then I am probably biassed. 1. ... I am not sure about a separate contants.php file. That is an extra file to include, so better to move them all to lib.php. Except that, there is a sense in which anything in lib.php is implied to be part of the public API of the quiz. Are some of these constants implementation details? Well, things used in settings are part of the public API. Those values get into backup files, and people need to use the values when creating a quiz using web services. OK, I have convinced myself. -> lib.php. 3. I should probably update the collatorlib references while I am at it. And get_plugin_list_with_file, since there are now more modern alternatives.
          Hide
          Dan Poltawski added a comment -

          > So, you are not worried about the backwards-compatbility implications of renaming the classes? I think it is worth the pain, but then I am probably biassed.

          Well - I saw you post about it and I didn't see many objections and really you know the impact better than me, so I didn't really have an opinion Although, surely people modifying quiz itself like this are limited. So it doesn't concern me that much.

          Show
          Dan Poltawski added a comment - > So, you are not worried about the backwards-compatbility implications of renaming the classes? I think it is worth the pain, but then I am probably biassed. Well - I saw you post about it and I didn't see many objections and really you know the impact better than me, so I didn't really have an opinion Although, surely people modifying quiz itself like this are limited. So it doesn't concern me that much.
          Hide
          Tim Hunt added a comment -

          I am going to wont-fix this. Rather than a big bang change, I will move classes naturally, when we are doing significant development in that area. E.g. MDL-40987.

          Show
          Tim Hunt added a comment - I am going to wont-fix this. Rather than a big bang change, I will move classes naturally, when we are doing significant development in that area. E.g. MDL-40987 .

            People

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

              Dates

              • Created:
                Updated:
                Resolved: