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 2.4 Branch:
      MDL-37797_m24
    • Pull Master Branch:
    • Rank:
      47534

      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.

        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: