update_record() doesn't allow for a column listed in the 'where' object to be updated

[Robert Lyon]

If I have a where object like:

stdClass Object (
     [localusr] => 11
     [authinstance] => 2
)

And data object like:

 stdClass Object (
     [remoteusername] => '<email address hidden>'
     [authinstance] => 4
     [localusr] => 11
 )

It will only update the remoteusername and not the authinstance as well?

The reason for this is that inside update_record() is a foreach loop to remove any data fields if they match where fields

But we probably don't need to do that.

[Robert Lyon (robertl-9)]

Actually, with postgres it will do a db write even if the values been written are the same as already existing. (unlike mysql)

[Aaron Wells (u-aaronw)]

Had a chat with Robert about this today. Moodle gets around this problem by forcing their update_record() method to use the "id" column to locate the record. We can't do that because we have some tables without an "id". And even then, sometimes it can be useful to run "update_record" without an ID even in cases where the table *does* have an ID. For instance, if we know some set of unique identifying information about the record, but not its ID, then this using update_record without ID can save us from having to do an extra SELECT query just to find the ID.

As Robert alluded to, the only downside to updating the WHERE columns as well, is that Postgres will do a "write" operation even if the value that you're setting via your UPDATE query is exactly the same as the old value. This can potentially cause a trigger to run, or a record to have to get re-indexed, which is an unnecessary expense, but not the end of the world.

So, I think what we should do is a slight shift in the API for update_record(), which will probably bring it more in line with how developers would expect it to function anyway.

This function takes three arguments: update_record($table, $dataobject, $where=null). $table is the table to update, $dataobject is the data to insert, and $where is an optional parameter that indicates which records should be updated. If $where is ommitted, then the function assumes that $dataobject contains an "id" field, and it tries to use that to identify the record to update.

The $where parameter is multi-purpose. It can be:

1. A string, in which case it should hold the name of a column in $dataobject, and that column should be used to identify the record to update.

2. An array of strings, in which case it identifies multiple columns in $dataobject to use, to identify the record in the database

3. A hash of key => value pairs, in which case it represents pretty much just a normal "where" clause, where each key is a column name, and each value is the value it should match.

The problem behavior here is caused, because any columns you identify in $where, via any of its three forms, are removed from $dataobject and not updated in the DB.

My proposed fix is that in case #3, we don't remove the columns from $dataobject. Sure, in cases #1 and #2 it makes sense to remove them, because you're using the values in $dataobject to identify the database record. But in case #3, the value for each column is already supplied in $where, so if there is also a value supplied in $dataobject, then it indicates that you want to change that value.

I've taken a quick look through the code, and I believe this approach will not cause any problems in existing code. So I'll go ahead and implement that.

[Aaron Wells (u-aaronw)]

In fact, my proposal for how to change update_record(), is pretty much already hinted at by the description at the top of the function:

 * @param mixed $where defines the WHERE part of the upgrade. Can be string (key) or array (keys) or hash (keys/values).
 * If the first two, values are expected to be in $dataobject.

[Mahara Bot (dev-mahara)]

Patch for "master" branch: https://reviews.mahara.org/5848

[Mahara Bot (dev-mahara)]

Reviewed: https://reviews.mahara.org/5848
Committed: https://git.mahara.org/mahara/mahara/commit/e78bbce37119c979346500cd995648441dc39ef2
Submitter: Son Nguyen (<email address hidden>)
Branch: master

commit e78bbce37119c979346500cd995648441dc39ef2
Author: Aaron Wells <email address hidden>
Date: Mon Dec 14 18:45:51 2015 +1300

update_record: Let a column be a data column & a where column

Bug 1525736: Also doing a general cleanup of this very old
and very messy function.

behatnotneeded: Covered by existing tests

Change-Id: I4b2feba22764fd290a69dc4b6ab1d734abd08a1c