Moodle
  1. Moodle
  2. MDL-37797

Suspect coding error in lib/filelib.php, breaks images in HTML block

    Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide
      1. On front page, add HTML block with an image.
      2. Set the block to be display throughout the entire site
      3. Go to a course
      4. Move the position of the HTML block to different area (eg: from right to left)
      5. Go to Front page, and move the postion to different area.
      6. Refresh the page
      7. Copy the image url from the block and paste it to a new window

      Make sure there's no error occurs for the page.

      Show
      On front page, add HTML block with an image. Set the block to be display throughout the entire site Go to a course Move the position of the HTML block to different area (eg: from right to left) Go to Front page, and move the postion to different area. Refresh the page Copy the image url from the block and paste it to a new window Make sure there's no error occurs for the page.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      At around line 4280 (version 2.4.1), there is this database call..

      $bprecord = $DB->get_record('block_positions', array('blockinstanceid' => $context->instanceid), 'visible');

      The '$fields' parameter is just 'visible'. This is wrong as, AFAIK, there always needs to be a unique field first (usually 'id'). Whenever this line runs it will throw an error...

      Did you remember to make the first column something unique in your call to get_records? Duplicate value '1' found in column 'visible'.

      line 1032 of /lib/dml/mysqli_native_moodle_database.php: call to debugging()
      line 1382 of /lib/dml/moodle_database.php: call to mysqli_native_moodle_database->get_records_sql()
      line 1354 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
      line 1333 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
      line 4280 of /lib/filelib.php: call to moodle_database->get_record()
      line 38 of /pluginfile.php: call to file_pluginfile()

      I am not sure under exactly what circumstances it is called. I have not been able to recreate the problem myself but some of my clients see it. It is related to adding an image to an HTML block.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Howard Miller added a comment -

            Actually, it's even worse than that. An obvious 'bodge' is to change 'visible' to '*'. However, you then get a duplicate record error.

            Again, on a cursory inspection, the code is doing a singular get_record on the 'block_instanceid' which is a Multiple index. I'm not entirely sure what the table is for but it seems that duplicate instanceids are legitimate as it holds instances for different contexts... maybe.

            Show
            Howard Miller added a comment - Actually, it's even worse than that. An obvious 'bodge' is to change 'visible' to '*'. However, you then get a duplicate record error. Again, on a cursory inspection, the code is doing a singular get_record on the 'block_instanceid' which is a Multiple index. I'm not entirely sure what the table is for but it seems that duplicate instanceids are legitimate as it holds instances for different contexts... maybe.
            Hide
            Howard Miller added a comment -

            Some more work on this. It will be hard to reproduce. As far as I can see that table is only populated when a block is 'moved'. The multiple instanceid is critical. This will happen only when two blocks have the same instanceid but different contexts. This is perfectly possible but may only ever show up on a site with lots of blocks that have been moved about.

            Show
            Howard Miller added a comment - Some more work on this. It will be hard to reproduce. As far as I can see that table is only populated when a block is 'moved'. The multiple instanceid is critical. This will happen only when two blocks have the same instanceid but different contexts. This is perfectly possible but may only ever show up on a site with lots of blocks that have been moved about.
            Hide
            Howard Miller added a comment -

            aand... suggested fix...

            $bprecord = $DB->get_record('block_positions', array('contextid' => $context->id, 'blockinstanceid' => $context->instanceid), '*');

            adding the contextid makes the record unique. Confirmed on a site showing the problem.

            Show
            Howard Miller added a comment - aand... suggested fix... $bprecord = $DB->get_record('block_positions', array('contextid' => $context->id, 'blockinstanceid' => $context->instanceid), '*'); adding the contextid makes the record unique. Confirmed on a site showing the problem.
            Hide
            Michael de Raadt added a comment -

            Thanks for reporting this, Howard.

            It's good to see Rosie is already onto this.

            Show
            Michael de Raadt added a comment - Thanks for reporting this, Howard. It's good to see Rosie is already onto this.
            Hide
            Rossiani Wijaya added a comment -

            Thanks Raj for helping me to replicate the issue.

            This is up for peer-review.

            Show
            Rossiani Wijaya added a comment - Thanks Raj for helping me to replicate the issue. This is up for peer-review.
            Hide
            Andrew Davis added a comment -

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

            Looks fine. You are go for integration.

            Show
            Andrew Davis added a comment - [Y] Syntax [NA] Output [Y] Whitespace [NA] Language [NA] Databases [Y] Testing [NA] Security [NA] Documentation [Y] Git [Y] Sanity check Looks fine. You are go for integration.
            Hide
            Rossiani Wijaya added a comment -

            Thanks Andrew for reviewing.

            Submitting for integration review.

            Show
            Rossiani Wijaya added a comment - Thanks Andrew for reviewing. Submitting for integration review.
            Hide
            Andrew Davis added a comment -

            Raised MDL-37910 as this function doesn't seem to be covered by unit tests which should have caught a straight up coding error.

            Show
            Andrew Davis added a comment - Raised MDL-37910 as this function doesn't seem to be covered by unit tests which should have caught a straight up coding error.
            Hide
            Damyon Wiese 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.

            Cheers!

            Show
            Damyon Wiese 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. Cheers!
            Hide
            Dan Poltawski added a comment -

            Looks like this issue is a duplicate of MDL-37053

            Show
            Dan Poltawski added a comment - Looks like this issue is a duplicate of MDL-37053
            Hide
            Dan Poltawski added a comment -

            Thanks Howard/Rosie, i've integrated this now.

            Show
            Dan Poltawski added a comment - Thanks Howard/Rosie, i've integrated this now.
            Hide
            Adrian Greeve added a comment -

            Tested on the 2.3, 2.4 and master integration branches.
            No errors encountered while testing.
            Test passed.

            Show
            Adrian Greeve added a comment - Tested on the 2.3, 2.4 and master integration branches. No errors encountered while testing. Test passed.
            Hide
            Damyon Wiese added a comment -

            Congratulations this fix has been added to Moodle!

            You may want to dedicate this issue to someone special on this Valentines day.

            Thanks!

            Show
            Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: