Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.5
    • Fix Version/s: None
    • Component/s: Quiz
    • Labels:
      None
    • Environment:
      All
    • Database:
      Any
    • Affected Branches:
      MOODLE_15_STABLE
    • Rank:
      10730

      Description

      Perhaps when generating the dropdown menu for import/export formats, it would be a good idea to check whether the relevant methods exist in the subclass?

      At the moment, trying to import or export from/into a format that doesn't support it gives output like:

      1. I have trouble comprehending what I've read.

      This quiz format has not yet been completed!

      2. I often leave tasks unfinished.

      This quiz format has not yet been completed!

      ... and so on

      I could add this if people agree it's reasonable.

        Activity

        Hide
        Martin Dougiamas added a comment -

        From (penny at catalyst.net.nz) Tuesday, 19 April 2005, 08:57 AM:

        Also, export should use whatever moodle function is normally used to recursively create directories as it's currently using mkdir which will fail if there isn't already a directory for the given course.

        (Ie, if you create a course and then straight away make a quiz and do an export, the course id directory will not exist already)

        From Gustav Delius (gwd2 at york.ac.uk) Tuesday, 19 April 2005, 08:13 PM:

        Yes, it would be great if you could do that. I am assigning this bug to Howard who is also trying to do something about the import and export.

        From Howard Miller (howard.miller at udcf.gla.ac.uk) Tuesday, 19 April 2005, 08:16 PM:

        Hi, I am working on both these problems right now!! I'm doing a general tidy up and bug fix in the import export area. Watch this space!

        From Howard Miller (howard.miller at udcf.gla.ac.uk) Tuesday, 19 April 2005, 08:20 PM:

        The problem with the second point - the un-implemented functionality - has always been that you don't know if the method is implemented until the subclass has actually been loaded. Only the subclass for the selected type is included, so when the menu is created one doesn't know. I guess I will just need so add a 'lookup' somewhere obvious, although any inspired ideas are appreciated!

        From Howard Miller (howard.miller at udcf.gla.ac.uk) Tuesday, 19 April 2005, 08:28 PM:

        ...and that's the file area for the course doesn't exist problem fixed and in CVS.

        From Howard Miller (howard.miller at udcf.gla.ac.uk) Tuesday, 19 April 2005, 11:35 PM:

        Both problems fixed in CVS. List of supported formats is now hard-coded - shame, but it looks better!

        From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 06:44 AM:

        I have backported both fixes from HEAD to STABLE, but not sure if the list of supported formats for export/import is the same - can someone in the know confirm?

        From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 06:48 AM:

        One last comment - the reason I found this problem with the import/export stuff is that I added the new quiz format from Hotpot - and having this hard coded list makes it harder for people to add on new formats.

        Copying the file around is one thing but asking people to add to an array of accepted formats is perhaps a little harder.

        Not sure what the solution is here though - we could be including each format.php and doing a method_exists but I agree, it's expensive. Although, moodle typically does exactly that with the modules. Maybe there is an easier way for a quiz format to register itself with the API as having that functionality than to pull in each format.php?

        From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 07:14 AM:

        In fact, the more I think about it the more I like the approach of asking each format whether they support it or not. I will write this & see how it works and post again...

        From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 07:52 AM:

        (adding MD to cc list)

        Serious problem – I hadn't realised that the quiz formats all had the same class name. This means that we can never include more than one without getting

        PHP Fatal error: Cannot redeclare class quiz_file_format

        I propose that we change the class names to quiz_(dirname)_format. This will prevent any collisions as well as be able to do what I proposed previously – checking which provide import and export. We can't use method_exists as it wants an instantiated object but we can use is_callable(array('classname','methodname'));

        Changing the classnames isn't too much work although it would affect those who have developed third party quiz formats (for example hotpot which we're currently evaluating and can therefore send a patch to it's developers).

        IMHO this is a really big problem and should be fixed asap - MD - what are your thoughts? Fix now or after branch?

        From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 08:41 AM:

        Attaching a patch that changes all the classnames to be distinct and for the dropdown, polls each format to find out whether it does import/export.

        I wanted to use the existing functions and check with is_callable, but since some formats override readquestion and some override readquestions it's not going to work. Also both functions are in the parent class anyway, so they're always callable. So I added two new functions, (dirname)_offer_import and (dirname)_offer_export.

        Seems to work fine.

        One last comment - multianswer does not get included in the hard coded list for import but it does actually override readqu estions, so I included it in my list.

        From Howard Miller (howard.miller at udcf.gla.ac.uk) Wednesday, 20 April 2005, 01:13 PM:

        A couple of things - how many people are really modifying the import/export capabilities? I don't think I've seen that many here's a new import export class postings and most of those have got added to the code. Is it really a big problem? Why has your hotpot class not been included in the main branch at some point?

        The other thing is that we kinda new this was getting out of hand and plan to revisit/refactor the import/export code in the near future. For the moment I'm not keen to increase the complexity, but equally well I see the problem and am prepared to go down the majority route.

        From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 01:18 PM:

        > Is it really a big problem?

        Don't know - it could be that there are lots of people out there running customised quiz formats?

        > Why has your hotpot class not been included in the main branch at some point?

        Hotpot isn't mine - I just started playing with this all yesterday

        I believe MD wants to include it in the main distribution once people have confirmed that it upgrades smoothly.

        From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 01:19 PM:

        bah, weird logout problem lost me half my comment. I was also going to say:

        > The other thing is that we kinda new this was getting out of hand and plan to

        > revisit/refactor the import/export code in the near future. For the moment I'm not

        > keen to increase the complexity, but equally well I see the problem and am

        > prepared to go down the majority route.

        That's ok - I just wanted to sound my piece and provide a patch - I'm by no means hugely familiar with the quiz code so you guys who know it better are far more qualified to comment than I am. If now is not the time, well then, use the patch after we branch if you like

        From Howard Miller (howard.miller at udcf.gla.ac.uk) Wednesday, 20 April 2005, 01:54 PM:

        OK My inclination is to go with the 'bodge' until after 1.5 branches, and then have a proper think about the whole thing.

        From Howard Miller (howard.miller at udcf.gla.ac.uk) Wednesday, 20 April 2005, 05:08 PM:

        ok - you talked me into it Automated version now in CVS.

        From Martin Langhoff (martin at catalyst.net.nz) Wednesday, 20 April 2005, 05:15 PM:

        howard – not sure how you've fixed it. We discussed some better ideas than registering a function as was implemented in Penny's patch. Registering a method removes the need for having the name of the class in the function/method name, and is more elegant.

        Alternatively, and to avoid instantiating the object just to find out, defining a constant is a lot cheaper than registering and calling a function.

        cheers~

        From Howard Miller (howard.miller at udcf.gla.ac.uk) Wednesday, 20 April 2005, 05:22 PM:

        Hi! Not entirely with you here. I didn't implement Penny's patch, but I think I did something very like it. Each format class now has it's own name (eg, quiz_format_gift, quiz_format_xml). There are two new default methods; provide_import() and provide_export() default is to return false, so you have to override these (and return true) to declare that the class provides that functionality. There is now a function that simply loads all the classes and checks the return value of the appropriate function - that's all it is! Does this help?

        From Howard Miller (howard.miller at udcf.gla.ac.uk) Wednesday, 20 April 2005, 05:28 PM:

        Oh - I meant to say that while it does mean instantiating all 11 classes just to get a drop down menu, I think its ok. It's a teacher level function that doesn't happen very often anyway. I get 5.8M of memory usage - I'm not sure how accurate that is, but it seems about average.

        Show
        Martin Dougiamas added a comment - From (penny at catalyst.net.nz) Tuesday, 19 April 2005, 08:57 AM: Also, export should use whatever moodle function is normally used to recursively create directories as it's currently using mkdir which will fail if there isn't already a directory for the given course. (Ie, if you create a course and then straight away make a quiz and do an export, the course id directory will not exist already) From Gustav Delius (gwd2 at york.ac.uk) Tuesday, 19 April 2005, 08:13 PM: Yes, it would be great if you could do that. I am assigning this bug to Howard who is also trying to do something about the import and export. From Howard Miller (howard.miller at udcf.gla.ac.uk) Tuesday, 19 April 2005, 08:16 PM: Hi, I am working on both these problems right now!! I'm doing a general tidy up and bug fix in the import export area. Watch this space! From Howard Miller (howard.miller at udcf.gla.ac.uk) Tuesday, 19 April 2005, 08:20 PM: The problem with the second point - the un-implemented functionality - has always been that you don't know if the method is implemented until the subclass has actually been loaded. Only the subclass for the selected type is included, so when the menu is created one doesn't know. I guess I will just need so add a 'lookup' somewhere obvious, although any inspired ideas are appreciated! From Howard Miller (howard.miller at udcf.gla.ac.uk) Tuesday, 19 April 2005, 08:28 PM: ...and that's the file area for the course doesn't exist problem fixed and in CVS. From Howard Miller (howard.miller at udcf.gla.ac.uk) Tuesday, 19 April 2005, 11:35 PM: Both problems fixed in CVS. List of supported formats is now hard-coded - shame, but it looks better! From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 06:44 AM: I have backported both fixes from HEAD to STABLE, but not sure if the list of supported formats for export/import is the same - can someone in the know confirm? From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 06:48 AM: One last comment - the reason I found this problem with the import/export stuff is that I added the new quiz format from Hotpot - and having this hard coded list makes it harder for people to add on new formats. Copying the file around is one thing but asking people to add to an array of accepted formats is perhaps a little harder. Not sure what the solution is here though - we could be including each format.php and doing a method_exists but I agree, it's expensive. Although, moodle typically does exactly that with the modules. Maybe there is an easier way for a quiz format to register itself with the API as having that functionality than to pull in each format.php? From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 07:14 AM: In fact, the more I think about it the more I like the approach of asking each format whether they support it or not. I will write this & see how it works and post again... From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 07:52 AM: (adding MD to cc list) Serious problem – I hadn't realised that the quiz formats all had the same class name. This means that we can never include more than one without getting PHP Fatal error: Cannot redeclare class quiz_file_format I propose that we change the class names to quiz_(dirname)_format. This will prevent any collisions as well as be able to do what I proposed previously – checking which provide import and export. We can't use method_exists as it wants an instantiated object but we can use is_callable(array('classname','methodname')); Changing the classnames isn't too much work although it would affect those who have developed third party quiz formats (for example hotpot which we're currently evaluating and can therefore send a patch to it's developers). IMHO this is a really big problem and should be fixed asap - MD - what are your thoughts? Fix now or after branch? From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 08:41 AM: Attaching a patch that changes all the classnames to be distinct and for the dropdown, polls each format to find out whether it does import/export. I wanted to use the existing functions and check with is_callable, but since some formats override readquestion and some override readquestions it's not going to work. Also both functions are in the parent class anyway, so they're always callable. So I added two new functions, (dirname)_offer_import and (dirname)_offer_export. Seems to work fine. One last comment - multianswer does not get included in the hard coded list for import but it does actually override readqu estions, so I included it in my list. From Howard Miller (howard.miller at udcf.gla.ac.uk) Wednesday, 20 April 2005, 01:13 PM: A couple of things - how many people are really modifying the import/export capabilities? I don't think I've seen that many here's a new import export class postings and most of those have got added to the code. Is it really a big problem? Why has your hotpot class not been included in the main branch at some point? The other thing is that we kinda new this was getting out of hand and plan to revisit/refactor the import/export code in the near future. For the moment I'm not keen to increase the complexity, but equally well I see the problem and am prepared to go down the majority route. From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 01:18 PM: > Is it really a big problem? Don't know - it could be that there are lots of people out there running customised quiz formats? > Why has your hotpot class not been included in the main branch at some point? Hotpot isn't mine - I just started playing with this all yesterday I believe MD wants to include it in the main distribution once people have confirmed that it upgrades smoothly. From (penny at catalyst.net.nz) Wednesday, 20 April 2005, 01:19 PM: bah, weird logout problem lost me half my comment. I was also going to say: > The other thing is that we kinda new this was getting out of hand and plan to > revisit/refactor the import/export code in the near future. For the moment I'm not > keen to increase the complexity, but equally well I see the problem and am > prepared to go down the majority route. That's ok - I just wanted to sound my piece and provide a patch - I'm by no means hugely familiar with the quiz code so you guys who know it better are far more qualified to comment than I am. If now is not the time, well then, use the patch after we branch if you like From Howard Miller (howard.miller at udcf.gla.ac.uk) Wednesday, 20 April 2005, 01:54 PM: OK My inclination is to go with the 'bodge' until after 1.5 branches, and then have a proper think about the whole thing. From Howard Miller (howard.miller at udcf.gla.ac.uk) Wednesday, 20 April 2005, 05:08 PM: ok - you talked me into it Automated version now in CVS. From Martin Langhoff (martin at catalyst.net.nz) Wednesday, 20 April 2005, 05:15 PM: howard – not sure how you've fixed it. We discussed some better ideas than registering a function as was implemented in Penny's patch. Registering a method removes the need for having the name of the class in the function/method name, and is more elegant. Alternatively, and to avoid instantiating the object just to find out, defining a constant is a lot cheaper than registering and calling a function. cheers~ From Howard Miller (howard.miller at udcf.gla.ac.uk) Wednesday, 20 April 2005, 05:22 PM: Hi! Not entirely with you here. I didn't implement Penny's patch, but I think I did something very like it. Each format class now has it's own name (eg, quiz_format_gift, quiz_format_xml). There are two new default methods; provide_import() and provide_export() default is to return false, so you have to override these (and return true) to declare that the class provides that functionality. There is now a function that simply loads all the classes and checks the return value of the appropriate function - that's all it is! Does this help? From Howard Miller (howard.miller at udcf.gla.ac.uk) Wednesday, 20 April 2005, 05:28 PM: Oh - I meant to say that while it does mean instantiating all 11 classes just to get a drop down menu, I think its ok. It's a teacher level function that doesn't happen very often anyway. I get 5.8M of memory usage - I'm not sure how accurate that is, but it seems about average.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: