Very unhappy about insert_record() behaviour

Very unhappy about insert_record() behaviour

by Martín Langhoff -
Number of replies: 2

The insert_record() we have now is -- in some specific cases -- corrupting data, due to a "bugfix" that we did a while ago that I don't think is good at all.

The bugfix was discussed plenty here http://moodle.org/mod/forum/discuss.php?d=38228 and here http://moodle.org/mod/forum/discuss.php?d=38183 . I reviewed the code, so I share the blame - but don't think I was conscious of how badly this could hit.

The problem I am referring to the unsetting of the PK field that happens unconditionally. There is no good reason for it, and will corrupt an insert where the caller knows the ID field beforehand. It is horribly bad practice to delete the value and keep going, without a warnign.

This messes up any relations you are trying to preserve, because the ID is getting overwritten.

For a good example of how this can hit you - Francois and Jonathan are working with a script that migrates a Moodle DB to another Moodle DB, using 2 DB handles and dmllib. This way we can migrate from PostgreSQL to Oracle and viceversa, with dmllib doing the conversion (and applying DIRTY ORACLE HACKs and any other tricks that may be required).

Similarly, any "upgrade" script that we may have now or in the future that moves data from one table to another, and tries to preserve any relations will not work.

I'm going to open a bug and draft a patch for this -- actually, I won't for a few days as I'm pretty tied up. I sure hope Jonathan and Francois will.

Average of ratings: -
In reply to Martín Langhoff

Re: Very unhappy about insert_record() behaviour

by sam marshall -
Picture of Core developers Picture of Peer reviewers Picture of Plugin developers
How about adding an extra parameter to insert_record that indicates you really want to use a specific id field, $preserveid or somesuch.

Then also add developer debugging code that causes a warning if you do have an id field in the record, and you didn't set the parameter to request it actually be used (in that case it still clears the id).

If done like that it shouldn't break anything (new)? I think.

--sam


In reply to sam marshall

Re: Very unhappy about insert_record() behaviour

by Martín Langhoff -
After thinking about it a bit, I came to that same conclusion - will post a patch soon wink

Luckily Francois, who has been working on the DB migration script, had already worked around the problem, so it only slowed things down. But in the debugging session I came across this, and got rather worried.