Uploaded image for project: '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

      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.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              ankit_frenz Ankit Agarwal added a comment -

              raising priority

              Show
              ankit_frenz Ankit Agarwal added a comment - raising priority
              Hide
              tsala 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
              tsala 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
              dougiamas 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
              dougiamas 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
              abgreeve 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
              abgreeve 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
              tsala 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
              tsala 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
              dougiamas Martin Dougiamas added a comment -

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

              Show
              dougiamas Martin Dougiamas added a comment - That looks pretty likely, thanks Adrian. Someone should try it though.
              Hide
              stronk7 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
              stronk7 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
              stronk7 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
              stronk7 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
              rajeshtaneja 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
              rajeshtaneja 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.gomez.caballero 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.gomez.caballero 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
              abgreeve Adrian Greeve added a comment -

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

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

              Thanks Adrian,

              Looks good

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks Adrian, Looks good
              Hide
              poltawski 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
              poltawski 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
              poltawski 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
              poltawski 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_frenz Ankit Agarwal added a comment -

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

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

              Thanks Ankit.

              Show
              poltawski Dan Poltawski added a comment - Thanks Ankit.
              Hide
              stronk7 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
              stronk7 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
              cfollin 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
              cfollin 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
              salvetore Michael de Raadt added a comment -

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

              Show
              salvetore Michael de Raadt added a comment - Thanks for sharing that patch, Chris. It will undoubtedly be useful to some.
              Hide
              fox 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
              fox 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
              pholden 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
              pholden 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
              ppollet Patrick Pollet added a comment -

              same here

              Good job Chris

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

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

              Show
              fox Séverin Terrier added a comment - Would be useful to have Chris patch integrated in next 1.9.19+ (or 1.9.20 ?)!
              Hide
              fern 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
              fern 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
              fox 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
              fox 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
              mchurch 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
              mchurch 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
              salvetore 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
              salvetore 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
              fox 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
              fox 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
              poltawski 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
              poltawski 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
              mchurch 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
              mchurch 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
              danmarsden 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
              danmarsden 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:
                    Fix Release Date:
                    9/Jul/12