Moodle
  1. Moodle
  2. MDL-33603

Database entries are always linked to wrong entry

    Details

    • Testing Instructions:
      Hide
      1. Add a few entries to a database module
      2. Make sure to include ##more## in your template so that you can see the more icon.
      3. Click on "view as list"
      4. Change the "Sort by" column and make sure that the first entry is different to the default. (clicking "Descending" should work)
      5. Click on any of the "more" icons

      [Test]
      It takes you to the correct page with the entry details.

      Show
      Add a few entries to a database module Make sure to include ##more## in your template so that you can see the more icon. Click on "view as list" Change the "Sort by" column and make sure that the first entry is different to the default. (clicking "Descending" should work) Click on any of the "more" icons [Test] It takes you to the correct page with the entry details.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-33603-master-v2
    • Rank:
      41543

      Description

      Replication steps:-

      1. Add a few entries to a database module
      2. Make sure to include ##more## in your template so that you can see the more icon.
      3. Click on "view as list"
      4. Change the "Sort by" column and make sure that the first entry is different to the default. (clicking "Descending" should work)
      5. Click on any of the "more" icons

      Expected result:-
      It takes you to the correct page with the entry details

      Actual result:-
      It takes to a page with details of some random entry.

      Because of this issue all the databases on moodle.org are completely useless, such as Moodle Jobs, Moodle Buzz etc

      For example http://moodle.org/mod/data/view.php?d=50 , try accessing any package from that url.

        Issue Links

          Activity

          Hide
          Ankit Agarwal added a comment -

          raising priority

          Show
          Ankit Agarwal added a comment - raising priority
          Hide
          Helen Foster added a comment -

          Ankit, thanks for your report.

          I notice all the front page databases on moodle.org e.g. http://moodle.org/mod/data/view.php?d=55 are affected. Adding 2.2.3 as affected version (which moodle.org is using). The bug seems to have occurred recently i.e. in the past week or two. I notice that the first entry in a database is linked to the last entry.

          Show
          Helen Foster added a comment - Ankit, thanks for your report. I notice all the front page databases on moodle.org e.g. http://moodle.org/mod/data/view.php?d=55 are affected. Adding 2.2.3 as affected version (which moodle.org is using). The bug seems to have occurred recently i.e. in the past week or two. I notice that the first entry in a database is linked to the last entry.
          Hide
          Martin Dougiamas added a comment -

          I'm bumping this as it's quite crucial, and has very high visibility.

          I suspect it's a regression from recent work like MDL-32126

          Show
          Martin Dougiamas added a comment - I'm bumping this as it's quite crucial, and has very high visibility. I suspect it's a regression from recent work like MDL-32126
          Hide
          Adrian Greeve added a comment -

          After looking around I found that the following line was causing the trouble

          $page = (int)array_search($record->id, $recordids);
          

          $recordids doesn't have a sort applied to it so the array that is searched for producing a single entry is something random.
          What I have done is changed the line back to the original code. Ideally we don't want to do another query on the database, but this piece of code worked in the original version.
          Another solution (which I propose creating an MDL for) is to change some of the functions that I created while fixing mdl-17327. This will be time consuming and require alterations to the unit tests as well, hence doing it at another date. This will allow everything to work at the moment and proper testing be done on the future solution.

          Show
          Adrian Greeve added a comment - After looking around I found that the following line was causing the trouble $page = ( int )array_search($record->id, $recordids); $recordids doesn't have a sort applied to it so the array that is searched for producing a single entry is something random. What I have done is changed the line back to the original code. Ideally we don't want to do another query on the database, but this piece of code worked in the original version. Another solution (which I propose creating an MDL for) is to change some of the functions that I created while fixing mdl-17327. This will be time consuming and require alterations to the unit tests as well, hence doing it at another date. This will allow everything to work at the moment and proper testing be done on the future solution.
          Hide
          Helen Foster added a comment -

          Hoping this fix will also fix the database activity RSS feed, which is also churning out incorrect links (as used in https://twitter.com/#!/moodlejobs )

          Show
          Helen Foster added a comment - Hoping this fix will also fix the database activity RSS feed, which is also churning out incorrect links (as used in https://twitter.com/#!/moodlejobs )
          Hide
          Martin Dougiamas added a comment -

          That looks pretty likely, thanks Adrian. Someone should try it though.

          Show
          Martin Dougiamas added a comment - That looks pretty likely, thanks Adrian. Someone should try it though.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well,

          that page is really spaghetti code, lol.

          Your fix looks correct (will lead to the correct page being displayed), but, for sure, instead of "hacking" $nowperpage and doubling the query and friends I would make the $newrecordids variable (around line 594) to only contain the id being requested and done.

          That should avoid the duplicate query and the array_seach() while using the same code to fetch the individual record.

          Not tested, just talking after reading a bit the code context of the proposed change.

          So +0 from me. For your consideration. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, that page is really spaghetti code, lol. Your fix looks correct (will lead to the correct page being displayed), but, for sure, instead of "hacking" $nowperpage and doubling the query and friends I would make the $newrecordids variable (around line 594) to only contain the id being requested and done. That should avoid the duplicate query and the array_seach() while using the same code to fetch the individual record. Not tested, just talking after reading a bit the code context of the proposed change. So +0 from me. For your consideration. Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Re myself: Ah, I see, with my proposal we lose the "pager" in single mode. I did not remember it's shown there at all (bloody persistent filtering).

          So perhaps your solution is the only one valid for now, in order to keep that "pager" working in the single template.

          (BTW, those "pagers" should be part of the templates, it's the only part of the page the designer has not control over).

          So changing my previous +0 to +0.5 (I don't like, but I cannot imagine an easy alternative).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Re myself: Ah, I see, with my proposal we lose the "pager" in single mode. I did not remember it's shown there at all (bloody persistent filtering). So perhaps your solution is the only one valid for now, in order to keep that "pager" working in the single template. (BTW, those "pagers" should be part of the templates, it's the only part of the page the designer has not control over). So changing my previous +0 to +0.5 (I don't like, but I cannot imagine an easy alternative). Ciao
          Hide
          Rajesh Taneja added a comment -

          I am not an expert here, but I don't see any easy way out.
          Seems Adrian's solution is simplest.

          There is one way we can reduce double query and pass that sorting to php.

          1. Replace $DB->get_records_sql($sqlselect, $allparams, $page * $nowperpage, $nowperpage) with $DB->get_records_sql($sqlselect, $allparams) and move it before https://github.com/abgreeve/moodle/blob/fa74decd3ab029a62429f4190c998f65fa614ef6/mod/data/view.php#L646
          2. Write a loop to create an array of record id's and search in that.
          3. Take a slice of array created in point 1 and use it at https://github.com/abgreeve/moodle/blob/fa74decd3ab029a62429f4190c998f65fa614ef6/mod/data/view.php#L632
          Show
          Rajesh Taneja added a comment - I am not an expert here, but I don't see any easy way out. Seems Adrian's solution is simplest. There is one way we can reduce double query and pass that sorting to php. Replace $DB->get_records_sql($sqlselect, $allparams, $page * $nowperpage, $nowperpage) with $DB->get_records_sql($sqlselect, $allparams) and move it before https://github.com/abgreeve/moodle/blob/fa74decd3ab029a62429f4190c998f65fa614ef6/mod/data/view.php#L646 Write a loop to create an array of record id's and search in that. Take a slice of array created in point 1 and use it at https://github.com/abgreeve/moodle/blob/fa74decd3ab029a62429f4190c998f65fa614ef6/mod/data/view.php#L632
          Hide
          Luis Gómez Caballero added a comment - - edited

          I've solved this issue editing the URL address line: I've changed order=DESC to order=ASC and also sort=0 to sort=1.

          I hope this would help.

          Thank you!

          Show
          Luis Gómez Caballero added a comment - - edited I've solved this issue editing the URL address line: I've changed order=DESC to order=ASC and also sort=0 to sort=1. I hope this would help. Thank you!
          Hide
          Adrian Greeve added a comment -

          Thanks to Raj for providing the concept of this fix. This based on the comment above.

          Show
          Adrian Greeve added a comment - Thanks to Raj for providing the concept of this fix. This based on the comment above.
          Hide
          Rajesh Taneja added a comment -

          Thanks Adrian,

          Looks good

          Show
          Rajesh Taneja added a comment - Thanks Adrian, Looks good
          Hide
          Dan Poltawski added a comment -

          Hi,

          I've integrated this to master, 22 and 21 only.

          The branch for 1.9 was wrong, $DB didn't exist then, nor params. If people want this backporting to 20 and 1.9 then I think we need to do that in a new issue.

          Show
          Dan Poltawski added a comment - Hi, I've integrated this to master, 22 and 21 only. The branch for 1.9 was wrong, $DB didn't exist then, nor params. If people want this backporting to 20 and 1.9 then I think we need to do that in a new issue.
          Hide
          Dan Poltawski added a comment -

          I tested this on master sucessfully.

          Ankit, could you do me a favour and test in 2.2 and 2.1?

          I expect its fine and since I don't want to hold up beta release i'm passing this isuse.

          Show
          Dan Poltawski added a comment - I tested this on master sucessfully. Ankit, could you do me a favour and test in 2.2 and 2.1? I expect its fine and since I don't want to hold up beta release i'm passing this isuse.
          Hide
          Ankit Agarwal added a comment -

          Tested this on 2.2 and 2.1.
          seems to be working fine.
          Thanks

          Show
          Ankit Agarwal added a comment - Tested this on 2.2 and 2.1. seems to be working fine. Thanks
          Hide
          Dan Poltawski added a comment -

          Thanks Ankit.

          Show
          Dan Poltawski added a comment - Thanks Ankit.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been spread to every git and cvs repository out there, just in time to roll Moodle 2.3beta!

          Thanks! Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been spread to every git and cvs repository out there, just in time to roll Moodle 2.3beta! Thanks! Closing, ciao
          Hide
          Chris Follin added a comment -

          Attaching patch for 1.9 in case anyone needs it. It's very similar to the 2.1 version but doesn't use $DB.

          Show
          Chris Follin added a comment - Attaching patch for 1.9 in case anyone needs it. It's very similar to the 2.1 version but doesn't use $DB.
          Hide
          Michael de Raadt added a comment -

          Thanks for sharing that patch, Chris. It will undoubtedly be useful to some.

          Show
          Michael de Raadt added a comment - Thanks for sharing that patch, Chris. It will undoubtedly be useful to some.
          Hide
          Séverin Terrier added a comment -

          Thanks Chris for the patch. Having updated (one of my Moodle) from 1.9.17+ to 1.9.19+ recently, this problem has been put on what should be the more 1.9 STABLE (and recommanded) version
          I feel bad to know that the solution was known several months ago, but not integrated...

          Show
          Séverin Terrier added a comment - Thanks Chris for the patch. Having updated (one of my Moodle) from 1.9.17+ to 1.9.19+ recently, this problem has been put on what should be the more 1.9 STABLE (and recommanded) version I feel bad to know that the solution was known several months ago, but not integrated...
          Hide
          Paul Holden added a comment -

          Just wanted to add my thanks to Chris for the patch too - we had been suffering from the same problem since upgrading to 1.9.19+

          Show
          Paul Holden added a comment - Just wanted to add my thanks to Chris for the patch too - we had been suffering from the same problem since upgrading to 1.9.19+
          Hide
          Patrick Pollet added a comment -

          same here

          Good job Chris

          Show
          Patrick Pollet added a comment - same here Good job Chris
          Hide
          Séverin Terrier added a comment -

          Would be useful to have Chris patch integrated in next 1.9.19+ (or 1.9.20 ?)!

          Show
          Séverin Terrier added a comment - Would be useful to have Chris patch integrated in next 1.9.19+ (or 1.9.20 ?)!
          Hide
          Fernando Oliveira added a comment -

          Has anybody had any luck applying this fix to Moodle 1.9.19? When I apply this on our site and click on the "read more" link, I get a blank page. Any suggestions? Thx.

          Show
          Fernando Oliveira added a comment - Has anybody had any luck applying this fix to Moodle 1.9.19? When I apply this on our site and click on the "read more" link, I get a blank page. Any suggestions? Thx.
          Hide
          Séverin Terrier added a comment -

          @Fernando : i've used Chris patch (see attached mdl33603_19.patch) with success (NOT the 1.9 DIFF).

          Show
          Séverin Terrier added a comment - @Fernando : i've used Chris patch (see attached mdl33603_19.patch) with success (NOT the 1.9 DIFF).
          Hide
          Mike Churchward added a comment -

          Michael de Raadt Since this feature was broken in 1.9 from a commit made on April 20, 2012, could we not apply the fix to the current release of 1.9? Also, wouldn't this bug constitute a security issue, since it returns the wrong data?

          Show
          Mike Churchward added a comment - Michael de Raadt Since this feature was broken in 1.9 from a commit made on April 20, 2012, could we not apply the fix to the current release of 1.9? Also, wouldn't this bug constitute a security issue, since it returns the wrong data?
          Hide
          Michael de Raadt added a comment -

          Hi, Mike.

          We won't integrate this fix into 1.9 here at Moodle HQ. If it were considered a security issue, it could be handled by Dan Marsden, who is overseeing the integration of serious security issues into 1.9. In my opinion this is not a security issue, and certainly not a serious one.

          Show
          Michael de Raadt added a comment - Hi, Mike. We won't integrate this fix into 1.9 here at Moodle HQ. If it were considered a security issue, it could be handled by Dan Marsden, who is overseeing the integration of serious security issues into 1.9. In my opinion this is not a security issue, and certainly not a serious one.
          Hide
          Séverin Terrier added a comment -

          What i don't understand is the following :

          • it's generally recommended to update to most recent version (or minor version), to FIX existing bugs (and have a MORE stable version)
          • this problem didn't exist in 1.9.17, and has been introduced in later version, so by upgrading you have a LESS stable version (the opposite of what is expected) !
          • this bug touches ALL Moodle upgraded to latest 1.9.19+, using database module ! That can be lot of people, and make them have a bad opinion of Moodle
          • even if it's just pointing to the wrong information, people may be tempted to "repair" it doing wrong things (deleting activity, manipulating directly in Moodle internal database...), and that can generate data loss (or corruption)
          • a patch has been made by Chris Follin since august 2012, and several people have reported it works OK
          • i think it would take less time to integrate this patch (that could correct the problem for ALL people making an upgrade) than to discuss about this inclusion...

          In short : i don't understand why Moodle HQ don't want to correct a bug that didn't exist in previous version, where a solution exist, and prefer let (lot of) people suffer this bug, using latest "MOST STABLE" 1.9 version...

          Show
          Séverin Terrier added a comment - What i don't understand is the following : it's generally recommended to update to most recent version (or minor version), to FIX existing bugs (and have a MORE stable version) this problem didn't exist in 1.9.17, and has been introduced in later version, so by upgrading you have a LESS stable version (the opposite of what is expected) ! this bug touches ALL Moodle upgraded to latest 1.9.19+, using database module ! That can be lot of people, and make them have a bad opinion of Moodle even if it's just pointing to the wrong information, people may be tempted to "repair" it doing wrong things (deleting activity, manipulating directly in Moodle internal database...), and that can generate data loss (or corruption) a patch has been made by Chris Follin since august 2012, and several people have reported it works OK i think it would take less time to integrate this patch (that could correct the problem for ALL people making an upgrade) than to discuss about this inclusion... In short : i don't understand why Moodle HQ don't want to correct a bug that didn't exist in previous version, where a solution exist, and prefer let (lot of) people suffer this bug, using latest "MOST STABLE" 1.9 version...
          Hide
          Dan Poltawski added a comment - - edited

          I'm in agreement, Séverin. I think we should be correcting regressions we made in security issues with the same process as security issues (that is, if we are still accepting changes on that branch, we should integrate the fix).

          Show
          Dan Poltawski added a comment - - edited I'm in agreement, Séverin. I think we should be correcting regressions we made in security issues with the same process as security issues (that is, if we are still accepting changes on that branch, we should integrate the fix).
          Hide
          Mike Churchward added a comment -

          Michael de Raadt Please review Séverin's comment. What you are saying is that Moodle HQ will break code in 1.9 but not fix it. That does not seem correct.

          Show
          Mike Churchward added a comment - Michael de Raadt Please review Séverin's comment. What you are saying is that Moodle HQ will break code in 1.9 but not fix it. That does not seem correct.
          Hide
          Dan Marsden added a comment -

          yeah - I agree we should backport this - esp as it's a regression caused by a sec fix. - have created MDL-38225 for this.

          I don't have time to do this at the moment but if another person volunteers to create a fresh 1.9 git branch, tests it and adds the detail to MDL-38225 I'll have a closer look.

          Show
          Dan Marsden added a comment - yeah - I agree we should backport this - esp as it's a regression caused by a sec fix. - have created MDL-38225 for this. I don't have time to do this at the moment but if another person volunteers to create a fresh 1.9 git branch, tests it and adds the detail to MDL-38225 I'll have a closer look.

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              8 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: