Moodle
  1. Moodle
  2. MDL-30562

questionid_column_name and extra_answer_fields should be public to allow access from backup and restore classes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.3, 2.2
    • Fix Version/s: 2.2.1, 2.3
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      I really coudn't imagine what could go wrong from making previously protected method public. Thought you must be sure it is public in all subclasses (i.e. shortanswer in core). In my experience having public/protected mismatch leads to empty screen on previewing questions (or fatal error depending on you error-reporting level).

      So just run preview of one question of each core types in Moodle if you want to be extra sure. If a question is able to display itself, all is OK.

      Show
      I really coudn't imagine what could go wrong from making previously protected method public. Thought you must be sure it is public in all subclasses (i.e. shortanswer in core). In my experience having public/protected mismatch leads to empty screen on previewing questions (or fatal error depending on you error-reporting level). So just run preview of one question of each core types in Moodle if you want to be extra sure. If a question is able to display itself, all is OK.
    • Workaround:
      Hide

      No one possible without hacking PHP, so better have this fixed in all 2.x

      Show
      No one possible without hacking PHP, so better have this fixed in all 2.x
    • Affected Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      33342

      Description

      In Moodle 1.x backup/restore of question was handled by question type class, so having questionid_column_name() and extra_answer_fields() protected had a sense and don't interefere with anything.

      Now in Moodle 2.x backup/restore is handled by separate classes that nees access to extra_question_fields(), questionid_column_name() and extra_answer_fields() functions. So these methods really should be public now (if possible - starting from 2.0 or at least 2.1).

      Tim, if you want, we could provide a git branch with the change, but it's really rather trivial so you may prefer doing it by youself.

        Issue Links

          Activity

          Hide
          Oleg Sychev added a comment -

          Linking to the issues it would help fix.

          These are much more complex changes and should be left for Moodle 2.3 probably. But this one is absolutely necessary. And easy.

          Show
          Oleg Sychev added a comment - Linking to the issues it would help fix. These are much more complex changes and should be left for Moodle 2.3 probably. But this one is absolutely necessary. And easy.
          Hide
          Tim Hunt added a comment -

          Yes, indeed, this one is necessary and easy. Please submit a patch.

          Show
          Tim Hunt added a comment - Yes, indeed, this one is necessary and easy. Please submit a patch.
          Hide
          Oleg Sychev added a comment -

          Changes branches available.

          It seems the bug was created in Moodle 2.1, so no changes in 2.0 is necessary.

          Show
          Oleg Sychev added a comment - Changes branches available. It seems the bug was created in Moodle 2.1, so no changes in 2.0 is necessary.
          Hide
          Oleg Sychev added a comment -

          Hmm, Tim, does that really takes so long to review this?

          I'm holding back release of Preg 2.1 until integration of this to have compatible question type and so backup/restore...

          Or there is no hope to get it integrated soon?

          Show
          Oleg Sychev added a comment - Hmm, Tim, does that really takes so long to review this? I'm holding back release of Preg 2.1 until integration of this to have compatible question type and so backup/restore... Or there is no hope to get it integrated soon?
          Hide
          Tim Hunt added a comment -

          Well, if you submit a branch including merges, then it takes a lot longer that expected to review.

          Also, as you point out, changing protected -> public in the base class will break any subclasses that override this method. Therefore, we need to note the API change in the upgrade.txt file.

          I am making a new git branch to sort out the mess. Once that is done, I will submit for integration.

          Show
          Tim Hunt added a comment - Well, if you submit a branch including merges, then it takes a lot longer that expected to review. Also, as you point out, changing protected -> public in the base class will break any subclasses that override this method. Therefore, we need to note the API change in the upgrade.txt file. I am making a new git branch to sort out the mess. Once that is done, I will submit for integration.
          Hide
          Tim Hunt added a comment -

          Oh, and I also need to edit your commit comment. Please read http://docs.moodle.org/dev/Commit_cheat_sheet

          Show
          Tim Hunt added a comment - Oh, and I also need to edit your commit comment. Please read http://docs.moodle.org/dev/Commit_cheat_sheet
          Hide
          Oleg Sychev added a comment -

          Added Moodle 2.2 branch

          Show
          Oleg Sychev added a comment - Added Moodle 2.2 branch
          Hide
          Tim Hunt added a comment -

          TO INTEGRATORS:

          I think we should cheat, and apply this API change in 2.2 stable as well as in master, because it will bring a log of benefits, and I don't think anyone will have started updating their plugins yet.

          Show
          Tim Hunt added a comment - TO INTEGRATORS: I think we should cheat, and apply this API change in 2.2 stable as well as in master, because it will bring a log of benefits, and I don't think anyone will have started updating their plugins yet.
          Hide
          Oleg Sychev added a comment -

          Umm, doesn't understand a bit about merges for this issue (not MDL-25617 with rather complex branch).

          Either the branches for this issue seems to contains only one commit with 3 changed strings, or I miss something due to been new to Git/GitHub. Where I supposed to look on GitHub to see it?

          Show
          Oleg Sychev added a comment - Umm, doesn't understand a bit about merges for this issue (not MDL-25617 with rather complex branch). Either the branches for this issue seems to contains only one commit with 3 changed strings, or I miss something due to been new to Git/GitHub. Where I supposed to look on GitHub to see it?
          Show
          Tim Hunt added a comment - https://github.com/vostreltsov/moodle/compare/master...MDL-30562-master
          Hide
          Oleg Sychev added a comment -

          Oh, my. Accidental merging (using pull instead of fetch) while getting accustomed to Git. I'm sorry.

          Git is somewhat tricky for newbies. It would be good to have some TortoiseGit instructions on http://docs.moodle.org/dev/Git_for_developers...

          Show
          Oleg Sychev added a comment - Oh, my. Accidental merging (using pull instead of fetch) while getting accustomed to Git. I'm sorry. Git is somewhat tricky for newbies. It would be good to have some TortoiseGit instructions on http://docs.moodle.org/dev/Git_for_developers ...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Aparup Banerjee added a comment -

          Thanks guys,
          I've integrated that into master and 2.2.x as far as i can't see that change breaking anything 2.2 even though its an api related change.

          Show
          Aparup Banerjee added a comment - Thanks guys, I've integrated that into master and 2.2.x as far as i can't see that change breaking anything 2.2 even though its an api related change.
          Hide
          Ankit Agarwal added a comment -

          Tested on a bunch of random questions types on both branches. All questions were displayed correctly.
          Passing
          Thanks

          Show
          Ankit Agarwal added a comment - Tested on a bunch of random questions types on both branches. All questions were displayed correctly. Passing Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

          Now... disconnect, relax and enjoy the next days, yay!

          Closing...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks! Now... disconnect, relax and enjoy the next days, yay! Closing...ciao
          Hide
          Joseph Rézeau added a comment -

          Hi all,
          Just noticed that my REGEXP question type throws a similar error when installed on current moodle 2.2 and 2.3 DEV versions (no such problem on 2.1).
          ERROR=

          Fatal error: Access level to qtype_regexp::questionid_column_name() must be public (as in class question_type) in Unknown on line 0

          Is this related to the problem at hand?
          I seem to remember having reported this problem either on the quiz forum or in the bug tracker some time before last Xmas, but cannot find a trace of it.
          I'm afraid I do not quite understand what I should change in my code to fix this. Any help welcome.
          Joseph

          Show
          Joseph Rézeau added a comment - Hi all, Just noticed that my REGEXP question type throws a similar error when installed on current moodle 2.2 and 2.3 DEV versions (no such problem on 2.1). ERROR= Fatal error: Access level to qtype_regexp::questionid_column_name() must be public (as in class question_type) in Unknown on line 0 Is this related to the problem at hand? I seem to remember having reported this problem either on the quiz forum or in the bug tracker some time before last Xmas, but cannot find a trace of it. I'm afraid I do not quite understand what I should change in my code to fix this. Any help welcome. Joseph
          Hide
          Tim Hunt added a comment -

          Weird. This should be fixed. The "in Unknown on line 0" is a particularly unhelpful error message.

          Show
          Tim Hunt added a comment - Weird. This should be fixed. The "in Unknown on line 0" is a particularly unhelpful error message.
          Hide
          Joseph Rézeau added a comment -

          Got it!
          Explained in moodle 2.3 moodle/question/type/upgrade.txt:

          If you are using the facilities provided by overriding the extra_answer_fields
          or questionid_column_name methods, then you must change these to be public
          methods. (This is required so that backup and restore can be made to work
          automatically. MDL-24408, MDL-25617, MDL-30562)

          In my regexp/questiontype.php file I had to change

              protected function questionid_column_name() {
                  return 'question';
              }

          to

              public function questionid_column_name() {
                  return 'question';
              }

          However, a more helpful error message would be welcome. What is weird is that I find no trace in the moodle 2.3 lang strings of the error message I quoted in my previous post. Where does it come from?

          Show
          Joseph Rézeau added a comment - Got it! Explained in moodle 2.3 moodle/question/type/upgrade.txt: If you are using the facilities provided by overriding the extra_answer_fields or questionid_column_name methods, then you must change these to be public methods. (This is required so that backup and restore can be made to work automatically. MDL-24408 , MDL-25617 , MDL-30562 ) In my regexp/questiontype.php file I had to change protected function questionid_column_name() { return 'question'; } to public function questionid_column_name() { return 'question'; } However, a more helpful error message would be welcome. What is weird is that I find no trace in the moodle 2.3 lang strings of the error message I quoted in my previous post. Where does it come from?
          Hide
          Tim Hunt added a comment -

          I'm afraid that the unhelpful error message comes from PHP, and there is nothing we can do to make it better.

          I'm glad you found the upgrade.txt file, and that it helped.

          Show
          Tim Hunt added a comment - I'm afraid that the unhelpful error message comes from PHP, and there is nothing we can do to make it better. I'm glad you found the upgrade.txt file, and that it helped.
          Hide
          Joseph Rézeau added a comment - - edited

          @Tim,
          Since I would like to have only ONE version of my REGEXP question type for moodle versions 2.1, 2.2, 2.3 etc.
          can I use "public function" instead of "protected function" in my REGEXP version for 2.1 as well as in further versions? Or should I keep "protected"?
          Joseph
          EDIT.- Actually, it seems I do not need that function questionid_column_name() at all in my question type!

          Show
          Joseph Rézeau added a comment - - edited @Tim, Since I would like to have only ONE version of my REGEXP question type for moodle versions 2.1, 2.2, 2.3 etc. can I use "public function" instead of "protected function" in my REGEXP version for 2.1 as well as in further versions? Or should I keep "protected"? Joseph EDIT.- Actually, it seems I do not need that function questionid_column_name() at all in my question type!
          Hide
          Tim Hunt added a comment -

          You are right that you don't need to override that method in your qtype.

          public function would have worked in all versions, if it had been necessary.

          Show
          Tim Hunt added a comment - You are right that you don't need to override that method in your qtype. public function would have worked in all versions, if it had been necessary.
          Hide
          Joseph Rézeau added a comment -

          @Tim,

          Hum, now I realize I do need that questionid_column_name() function, cf our discussion here: http://moodle.org/mod/forum/discuss.php?d=187125&parent=821864.

          But only because - on the model of existing question types - I had named my questionid_column_name "question" rather than "questionid"...

          Which leads to further questioning...

          1.- In order to commit my question type to the new plugins repository, I had to rename its table to qtype_regex.
          2.- I see that Oleg did the same for his preg question type (table qtype_preg), and so did Jorge Villalon for his Concept map question (table qtype_conceptmap_options).
          3.- At the moment, the only "standard" question type that follows this new naming scheme is the Essay question type, with a table named "qtype_essay_options".
          4.- In those tables following new naming scheme, the column name is sometimes called "question", sometimes "questionid".

          In order to prepare for your switching to the new scheme for all standard questions, would it not be a good idea for both standard and 3rd-party questions to standardize names, maybe as follows (taking for instance the essay question type):

          question type table name = qtype_essay_options
          questionid_column_name = questionid

          In fact, if ALL question types agreed to call their questionid_column_name questionid, then we would no longer need to override that name in our questions, would we?

          Joseph

          Show
          Joseph Rézeau added a comment - @Tim, Hum, now I realize I do need that questionid_column_name() function, cf our discussion here: http://moodle.org/mod/forum/discuss.php?d=187125&parent=821864 . But only because - on the model of existing question types - I had named my questionid_column_name "question" rather than "questionid"... Which leads to further questioning... 1.- In order to commit my question type to the new plugins repository, I had to rename its table to qtype_regex. 2.- I see that Oleg did the same for his preg question type (table qtype_preg), and so did Jorge Villalon for his Concept map question (table qtype_conceptmap_options). 3.- At the moment, the only "standard" question type that follows this new naming scheme is the Essay question type, with a table named "qtype_essay_options". 4.- In those tables following new naming scheme, the column name is sometimes called "question", sometimes "questionid". In order to prepare for your switching to the new scheme for all standard questions, would it not be a good idea for both standard and 3rd-party questions to standardize names, maybe as follows (taking for instance the essay question type): question type table name = qtype_essay_ options questionid_column_name = questionid In fact, if ALL question types agreed to call their questionid_column_name questionid , then we would no longer need to override that name in our questions, would we? Joseph
          Hide
          Tim Hunt added a comment -

          My plan is to fix all the table and column names to be as consistent as possible, and consistent with the Moodle coding guidelines, some time before Moodle 2.3.

          And yes, once everything is more consistent, we can eliminate some of the code that supports the old way of doing things.

          Show
          Tim Hunt added a comment - My plan is to fix all the table and column names to be as consistent as possible, and consistent with the Moodle coding guidelines, some time before Moodle 2.3. And yes, once everything is more consistent, we can eliminate some of the code that supports the old way of doing things.
          Hide
          Oleg Sychev added a comment -

          @Tim and Joseph

          We should find a convention for the main question type table names. http://docs.moodle.org/dev/Database has no special string for question types but urges following convetions:
          1) The main table containing instances of each module must have the same name as the module (eg widget)
          2) Tables associated with one block must follow this convention with their names: $CFG->prefix + "block_" + name_of_the_block + anything_else. For example, assuming that $CFG->prefix is 'mdl_', all the tables for the block "rss_client" must start by 'mdl_block_rss_client'

          It seems that frankenstyle prefer to have main table for plugin as just qtype_xxx without "options". I may see some wisdom in following it and having tables qtype_shortanswer, qtype_preg etc (but qtype_numeric_answers for the tolerance handling!). I may see some wisdom in having "options" suffix thought.

          Tim, would do you think on this matter?

          Show
          Oleg Sychev added a comment - @Tim and Joseph We should find a convention for the main question type table names. http://docs.moodle.org/dev/Database has no special string for question types but urges following convetions: 1) The main table containing instances of each module must have the same name as the module (eg widget) 2) Tables associated with one block must follow this convention with their names: $CFG->prefix + "block_" + name_of_the_block + anything_else. For example, assuming that $CFG->prefix is 'mdl_', all the tables for the block "rss_client" must start by 'mdl_block_rss_client' It seems that frankenstyle prefer to have main table for plugin as just qtype_xxx without "options". I may see some wisdom in following it and having tables qtype_shortanswer, qtype_preg etc (but qtype_numeric_answers for the tolerance handling!). I may see some wisdom in having "options" suffix thought. Tim, would do you think on this matter?
          Hide
          Tim Hunt added a comment -

          I have not thought beyond the fact that all the table names should start with the frankenstyle plugin name. I probably won't have a clear idea what I think is 'right' until after I have fixed all the core question types.

          Show
          Tim Hunt added a comment - I have not thought beyond the fact that all the table names should start with the frankenstyle plugin name. I probably won't have a clear idea what I think is 'right' until after I have fixed all the core question types.
          Hide
          Oleg Sychev added a comment -

          IMHO it has sense to have a distinct name for the main plugin table, i.e, "qtype_xxx" without "options".

          Also, if we want to apply "options" systematically, then numeric (and similar qtypes) should have something like "qtype_numeric_answer_options" instead of "qtype_numeric_answers".

          Show
          Oleg Sychev added a comment - IMHO it has sense to have a distinct name for the main plugin table, i.e, "qtype_xxx" without "options". Also, if we want to apply "options" systematically, then numeric (and similar qtypes) should have something like "qtype_numeric_answer_options" instead of "qtype_numeric_answers".

            People

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

              Dates

              • Created:
                Updated:
                Resolved: