Database entries are always linked to wrong entry

raising priority

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.

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

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.

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 )

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

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

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

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

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!

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

Thanks Adrian,

Looks good

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.

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.

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

Thanks Ankit.

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

Thanks! Closing, ciao

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

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

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...

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+

same here

Good job Chris

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

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.

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

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?

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.

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...

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).

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.

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.