Uploaded image for project: '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
    • Status: Closed
    • Priority: 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:

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            oa_sychev 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
            oa_sychev 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
            timhunt Tim Hunt added a comment -

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

            Show
            timhunt Tim Hunt added a comment - Yes, indeed, this one is necessary and easy. Please submit a patch.
            Hide
            oa_sychev 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
            oa_sychev 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
            oa_sychev 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
            oa_sychev 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
            timhunt 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
            timhunt 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
            timhunt 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
            timhunt 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
            oa_sychev Oleg Sychev added a comment -

            Added Moodle 2.2 branch

            Show
            oa_sychev Oleg Sychev added a comment - Added Moodle 2.2 branch
            Hide
            timhunt 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
            timhunt 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
            oa_sychev 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
            oa_sychev 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
            timhunt Tim Hunt added a comment - https://github.com/vostreltsov/moodle/compare/master...MDL-30562-master
            Hide
            oa_sychev 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
            oa_sychev 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
            stronk7 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
            stronk7 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
            nebgor 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
            nebgor 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_frenz 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_frenz Ankit Agarwal added a comment - Tested on a bunch of random questions types on both branches. All questions were displayed correctly. Passing Thanks
            Hide
            stronk7 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
            stronk7 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
            rezeau 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
            rezeau 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
            timhunt Tim Hunt added a comment -

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

            Show
            timhunt Tim Hunt added a comment - Weird. This should be fixed. The "in Unknown on line 0" is a particularly unhelpful error message.
            Hide
            rezeau 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
            rezeau 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
            timhunt 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
            timhunt 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
            rezeau 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
            rezeau 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
            timhunt 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
            timhunt 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
            rezeau 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
            rezeau 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
            timhunt 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
            timhunt 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
            oa_sychev 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
            oa_sychev 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
            timhunt 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
            timhunt 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
            oa_sychev 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
            oa_sychev 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:
                  Fix Release Date:
                  9/Jan/12