Details

    • Testing Instructions:
      Hide
      1. Find or create a mod plugin with:
        1. pluginname_supports(FEATURE_MOD_ARCHETYPE) returning MOD_ARCHETYPE_RESOURCE
        2. pluginname_supports(FEATURE_MOD_INTRO) returning false
      2. Add an instance of the resource to a class
      3. Add the activities block to the class
      4. Click "Resources" on the activities block
      5. You should see no error message
      Show
      Find or create a mod plugin with: pluginname_supports(FEATURE_MOD_ARCHETYPE) returning MOD_ARCHETYPE_RESOURCE pluginname_supports(FEATURE_MOD_INTRO) returning false Add an instance of the resource to a class Add the activities block to the class Click "Resources" on the activities block You should see no error message
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
    • Pull Master Branch:
      MDL-36482_master
    • Rank:
      45938

      Description

      Viewing the page /course/resources.php when you have a module which doesn't support FEATURE_MOD_INTRO on the course causes the following database error:

      Error reading from database
      
      More information about this error
      Debug info: Unknown column 'intro' in 'field list'
      SELECT id,name,intro,introformat,timemodified FROM mdl_printable WHERE id IN (?,?,?,?) ORDER BY id
      [array (
      0 => '392',
      1 => '391',
      2 => '390',
      3 => '393',
      )]
      Error code: dmlreadexception
      Stack trace:
      
          line 407 of /lib/dml/moodle_database.php: dml_read_exception thrown
          line 945 of /lib/dml/mysqli_native_moodle_database.php: call to moodle_database->query_end()
          line 1175 of /lib/dml/moodle_database.php: call to mysqli_native_moodle_database->get_records_sql()
          line 1148 of /lib/dml/moodle_database.php: call to moodle_database->get_records_select()
          line 90 of /course/resources.php: call to moodle_database->get_records_list()
      

      To reproduce this behaviour:

      1. Find or create a mod plugin with:
        1. pluginname_supports(FEATURE_MOD_ARCHETYPE) returning MOD_ARCHETYPE_RESOURCE
        2. pluginname_supports(FEATURE_MOD_INTRO) returning false
      2. Add an instance of the resource to a class
      3. Add the activities block to the class
      4. Click "Resources" on the activities block
      5. You should see the error message

        Activity

        Hide
        Dan Poltawski added a comment -

        Hi Michael,

        I don't suppose you have an example of a module which is affecting by this?

        Show
        Dan Poltawski added a comment - Hi Michael, I don't suppose you have an example of a module which is affecting by this?
        Hide
        Michael Aherne added a comment -

        I do, but it's not my code so I'm not sure if I can post it. I'll check it out and, if not, create a test module with the same behaviour. Cheers, Michael.

        Show
        Michael Aherne added a comment - I do, but it's not my code so I'm not sure if I can post it. I'll check it out and, if not, create a test module with the same behaviour. Cheers, Michael.
        Hide
        Michael Aherne added a comment -

        OK, that's an example mod uploaded.

        Show
        Michael Aherne added a comment - OK, that's an example mod uploaded.
        Hide
        Dan Poltawski added a comment -

        Thanks Michael, thats helpful.

        Show
        Dan Poltawski added a comment - Thanks Michael, thats helpful.
        Hide
        Michael Aherne added a comment - - edited

        I've added a patch which fixes this bug. I don't like the fact that I had to remove the restriction on which fields are returned by the get_records_list call, but I can't see what else can be done.

        The main issue isn't actually the support for FEATURE_MOD_INTRO - we could construct a different call depending on whether this is supported and deal with the results accordingly - but the code assumes that all module tables contain a timemodified column, which is not necessarily the case (unless this is a requirement for plugins, but I can't find it documented anywhere!).

        Any comments on the patch would be very welcome.

        Show
        Michael Aherne added a comment - - edited I've added a patch which fixes this bug. I don't like the fact that I had to remove the restriction on which fields are returned by the get_records_list call, but I can't see what else can be done. The main issue isn't actually the support for FEATURE_MOD_INTRO - we could construct a different call depending on whether this is supported and deal with the results accordingly - but the code assumes that all module tables contain a timemodified column, which is not necessarily the case (unless this is a requirement for plugins, but I can't find it documented anywhere!). Any comments on the patch would be very welcome.
        Hide
        Frédéric Massart added a comment - - edited

        Hi Michael,

        Thanks for providing a patch for this issue. Here are my comments:

        [Y] Syntax
        [Y] Output
        [Y] Whitespace
        [-] Language
        [Y] Databases
        [Y] Testing
        [-] Security
        [-] Documentation
        [N] Git
        [Y] Sanity check

        Though your patch will work, I am not entirely convinced that this is the right way of proceeding. As you mentioned timemodified is not a required field (http://docs.moodle.org/dev/Database) even if it's recommended, and therefore I would entirely drop its use here. As far as I know we do not display the information on the course page, and on the other pages (at least some) linked from the 'Activity block', this information is missing too. Also, I don't seen any reason why we would only display the 'timemodified' when the course format does not have sections...

        What do you think about constructing the fields list 'id, name' with intro and introformat only if the plugin supports FEATURE_MOD_INTRO, and use the same check to output the information?

        On a more trivial note, the Git commit could use the component, which woulb be core I guess. http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages

        Many thanks!
        Fred

        Show
        Frédéric Massart added a comment - - edited Hi Michael, Thanks for providing a patch for this issue. Here are my comments: [Y] Syntax [Y] Output [Y] Whitespace [-] Language [Y] Databases [Y] Testing [-] Security [-] Documentation [N] Git [Y] Sanity check Though your patch will work, I am not entirely convinced that this is the right way of proceeding. As you mentioned timemodified is not a required field ( http://docs.moodle.org/dev/Database ) even if it's recommended, and therefore I would entirely drop its use here. As far as I know we do not display the information on the course page, and on the other pages (at least some) linked from the 'Activity block', this information is missing too. Also, I don't seen any reason why we would only display the 'timemodified' when the course format does not have sections... What do you think about constructing the fields list 'id, name' with intro and introformat only if the plugin supports FEATURE_MOD_INTRO, and use the same check to output the information? On a more trivial note, the Git commit could use the component, which woulb be core I guess. http://docs.moodle.org/dev/Commit_cheat_sheet#Provide_clear_commit_messages Many thanks! Fred
        Hide
        Russell Smith added a comment -

        Based on Fred's comments, I reworked Michael's patch to see if it is more along the expected lines. I don't want to push in and overwrite all the pull items, so I'm just going to attach one diff link to this comment. If Michael is happen, I can post like for all other versions and re-request peer review.

        https://github.com/mr-russ/moodle/compare/master...MDL-36482_master

        Show
        Russell Smith added a comment - Based on Fred's comments, I reworked Michael's patch to see if it is more along the expected lines. I don't want to push in and overwrite all the pull items, so I'm just going to attach one diff link to this comment. If Michael is happen, I can post like for all other versions and re-request peer review. https://github.com/mr-russ/moodle/compare/master...MDL-36482_master
        Hide
        Michael Aherne added a comment -

        Hi Russell, I'm very happy for you to take over this issue so I've reassigned it to you. I'm afraid I've not had time to work on this issue recently so it would be great if you were able to fix it. I was never very happy with the patch I put in for it anyway!

        Show
        Michael Aherne added a comment - Hi Russell, I'm very happy for you to take over this issue so I've reassigned it to you. I'm afraid I've not had time to work on this issue recently so it would be great if you were able to fix it. I was never very happy with the patch I put in for it anyway!
        Hide
        Russell Smith added a comment -

        I've reworked the patch on the basis of the comments Frederic made. Michael has agreed for me to take this issue over. Please re-review.

        Thanks

        Show
        Russell Smith added a comment - I've reworked the patch on the basis of the comments Frederic made. Michael has agreed for me to take this issue over. Please re-review. Thanks
        Hide
        Frédéric Massart added a comment -

        Thanks guys! Pushing this for integration after second peer review. Cheers, Fred.

        Show
        Frédéric Massart added a comment - Thanks guys! Pushing this for integration after second peer review. Cheers, Fred.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (23, 24, 25 & master), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (23, 24, 25 & master), thanks!
        Hide
        Michael Aherne added a comment -

        Many thanks to Russell for taking this on and fixing it!

        Show
        Michael Aherne added a comment - Many thanks to Russell for taking this on and fixing it!
        Hide
        Petr Škoda added a comment -

        there is no intro in $cm, it is in $resource, this change prevents displaying of all descriptions, it seems that developer did not test this, failing...

        Solution:

        replace

        if (isset($cm->intro) && isset($cm->introformat)) {
        

        with

        if (isset($resource->intro) && isset($resource->introformat)) {
        
        Show
        Petr Škoda added a comment - there is no intro in $cm, it is in $resource, this change prevents displaying of all descriptions, it seems that developer did not test this, failing... Solution: replace if (isset($cm->intro) && isset($cm->introformat)) { with if (isset($resource->intro) && isset($resource->introformat)) {
        Hide
        Russell Smith added a comment -

        I have no excuse. Most of our modules don't have descriptions and I didn't pick it up and was silly enough not to think how important that was to check. A bad fail.

        I've reworked the patch with Petr's correct fix and push it up to github.

        I'm not sure what to do with a Problem in testing status, so I'm not going to alter that until I understand when to do next in the process.

        Show
        Russell Smith added a comment - I have no excuse. Most of our modules don't have descriptions and I didn't pick it up and was silly enough not to think how important that was to check. A bad fail. I've reworked the patch with Petr's correct fix and push it up to github. I'm not sure what to do with a Problem in testing status, so I'm not going to alter that until I understand when to do next in the process.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Oh, my, I'm blind!

        Show
        Eloy Lafuente (stronk7) added a comment - Oh, my, I'm blind!
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Hi Russell, you've replaced your original commit while, for work in progress like this is always better to add a new commit with the changes, FYI.

        I'm going to do that now. Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Hi Russell, you've replaced your original commit while, for work in progress like this is always better to add a new commit with the changes, FYI. I'm going to do that now. Ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        All branches amended, sending back to testing, thanks Petr!

        Show
        Eloy Lafuente (stronk7) added a comment - All branches amended, sending back to testing, thanks Petr!
        Hide
        Petr Škoda added a comment -

        works fine now, thanks

        Show
        Petr Škoda added a comment - works fine now, thanks
        Hide
        Damyon Wiese added a comment -

        Thanks for your contribution! This issue has been reviewed, integrated, tested and now released to everyone.

        Closing as Fixed!

        Show
        Damyon Wiese added a comment - Thanks for your contribution! This issue has been reviewed, integrated, tested and now released to everyone. Closing as Fixed!
        Hide
        David Monllaó added a comment -

        Removing acceptance_test_required label as there are no modules in core with MOD_ARCHETYPE_RESOURCE and FEATURE_MOD_INTRO = false. Please add qa_test_require label if you think is necessary, I haven't added it as I think is too specific IMO.

        Show
        David Monllaó added a comment - Removing acceptance_test_required label as there are no modules in core with MOD_ARCHETYPE_RESOURCE and FEATURE_MOD_INTRO = false. Please add qa_test_require label if you think is necessary, I haven't added it as I think is too specific IMO.
        Hide
        Elizabeth Dalton added a comment -

        I wanted to note, in case anyone else hits this problem, that on our system it was triggered by including Subpages in a course. (Moodle 2.4.4) We consider Subpages to offer critical functionality for our completely online courses, so we don't want to disable that plugin. Hopefully at some point in the future Subpages will be included in Core and so problems like this will turn up earlier in testing.

        Show
        Elizabeth Dalton added a comment - I wanted to note, in case anyone else hits this problem, that on our system it was triggered by including Subpages in a course. (Moodle 2.4.4) We consider Subpages to offer critical functionality for our completely online courses, so we don't want to disable that plugin. Hopefully at some point in the future Subpages will be included in Core and so problems like this will turn up earlier in testing.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: