General developer forum

Well,

if waste of lines is the key then, perhaps we could have something like:

list($c, $d, $p, $r, $cm, $ctx) = forum_retrofit_as_much_as_possible($forum, $reply); // obviously, joking


just one line and same functionality and will work in all the forum pages!

Seriously, IMO, that's "polluting" the database abstraction layer with functions not doing "standard" database tasks.

I understand and agree 100% that, if possible, code can / must be reduced / centralised. 100%. But the database layer cannot be the place for all that extra functionality.

Some months ago we agreed that all the get_xxx() (read) functions will be returning false/empty if no data is found, while all the write operations will be throwing exception if something wrong happens.

I think it's a simple / correct behaviour and any exception to that simple principle must be avoided (alternatively, that behaviour can be re-analysed / changed, of course, but always as a whole).

I can imagine ppl wanting to throw exceptions in other get_records/recordset() functions, why not? The justification is exactly the same than the one exposed above for the get_record() function. Will we create then all those get_existing_records/existing_recordset() new functions? Just to provide Moodle with the ability to throw exceptions in all the read database functions? Sincerely I think no.

Or imagine you want to perform something different from throwing an exception in a lot of insert_record() uses over the whole Moodle, say, silly example I know, send one email. Will we create one insert_record_send_email() method in the DB abstraction layer just for that? Sincerely I think no. I know the example is ridiculous (so don't try to fix it ), but the underlying question is the same. And the answer should be the same too: No.

Also, the name is not clear IMO: get_existing_record(), obviously the DB only is going to give you existing records, until now, I don't know DBs able to "imagine" records on the fly.

Finally, I know we are talking only about adding one simple method to $DB (a few lines performing one get_record + throw exception), in order to save some lines here and there (lazy developers!) and, from that perspective, it doesn't hurt me too much.

Instead, my concerns are about how that tiny change affects to the general behaviour / status-quo existing until now. And if that can be used, in the future, to justify abusing the DB layer by adding more and more, said with irony, "cool" features continuously.

One DB must do whatever one DB does. Our $DB too, of course. Nothing else. -1 here (although I guess that the vast majority of you, fellow and lazy - like me- developers, will vote the opposite)

Ciao

Yes please!

Rather than a new method, I would kind of prefer if get_record always does this (anybody requiring the current behaviour should do get_records and look for count 0). However failing that change, the suggested one is a big improvement.

Where are the 1,000 Eloys that are supposed to be expressing their opinion against this? I only see one.

--sam

I completely agree that encapsulating the
if (!($thing = $DB->get_record(...)) {
throw new moodle_exception(...);
}
in a function would be a good thing.

I am normally pretty picky about APIs, and keeping them clean, but I do understand why Eloy thinks this should not be on $DB. It seems like a sensible part of the DB API to me.

I don't like Petr's idea of adding $errorcode='', $module='' arguments. If the specific error message is important, we should leave the if and the throw in the calling code.

However, for the many situations where you are getting a record really should exist (for example $DB->get_record('course_categories', array('id' => $course->id)); - a place where you are following a foreign key link) then the error checking in the calling script is unnecessary clutter.

About re-purposing the $ignoremultiple=false parameter. We could rename that to $strictness, then create some constants

IGNORE_MISSING = 0
IGNORE_MULTIPLE = 1
MUST_EXIST = 2

that is backwards compatible, and the constant names hopefully would make the code more readable.

This sounds like a good idea in that it makes the code shorter (and hopefully faster to write!) without losing readability.

Perhaps $DB->must_get_record() works better as a name (or family of names if this is going to be replicated consistently for other $DB->get functions).

I do see Eloy's point about the purity of the class, but I think if we see these as "add-ons" to DML then it's OK with me. The point of an API is to be as useful as possible after all.