Moodle

This came from some discussion in the Moodle HQ chat where I was getting a bit concerned about the overall db API for developers.

Currently in HEAD upgrade.php files we have stuff like this (comments, $result etc removed)

$dbman = $DB->get_manager();

$table = new XMLDBTable('grade_letters');
$index = new XMLDBIndex('contextid-lowerboundary');
$index->setAttributes(XMLDB_INDEX_NOTUNIQUE, array('contextid', 'lowerboundary'));

$dbman->drop_index($table, $index);

A cleaner-looking alternative (from Penny's suggestion, extended by me) could PERHAPS be:

$dbmanager = $DB->get_manager();

$table = $dbmanager->get_table_object('grade_letters'); // internally instantiates XMLDB objects
$index = $dbmanager->get_index_object('contextid-lowerboundary'', XMLDB_INDEX_NOTUNIQUE, array('contextid', 'lowerboundary'));

$dbmanager->drop_index($table, $index);

Something like this could easily be a layer on top of the existing tables (retaining backward compatibility). The get_xxx_object calls just instantiate the XMLDB functions internally, and also call setAttribute as required.

Hi,

apart from the upper/lower thing (I agree we must change to xmldb_table and so on). Some comments:

1) XMLDB objects have that name because of one simple reason. They are JUST some objects, usually read from XML files (install.xml), able to represent one neutral DB structure (tables, fields, indexes...). And they only have a bunch of methods, mainly setters and getters, readers and writers, to be able to represent that structure from php files (upgrade.php). And they don't know anything about DBs nor managers nor generators. Because of that, I really don't understand why we want a new way to instantiate them (from the manager).

2) The example above... keeping XMLDB objects separated (and not mixed with manager) would be:

2A)
$dbman = $DB->get_manager();

$table = new xmldb_table('grade_letters'); // explicity instantiates XMLDB objects
$index = new xmldb_index('contextid-lowerboundary');
$index->set_attributes(XMLDB_INDEX_NOTUNIQUE, array('contextid', 'lowerboundary'));

$dbman->drop_index($table, $index);

oki, I know it's one more line, lets add some more parameters to the index constructor (badly PHP doesn't support multiple constructors), so we can make:

2B)
$dbman = $DB->get_manager();

$table = new xmldb_table('grade_letters'); // explicity instantiates XMLDB objects
$index = new xmldb_index('contextid-lowerboundary', XMLDB_INDEX_NOTUNIQUE, array('contextid', 'lowerboundary'));

$dbman->drop_index($table, $index);

IMO this code is pretty equivalent (in complexity and readability) to the one proposed above. Plus, we are keeping the xmldb stuff definition out from the manager (point 1 above).

Uhm... is really so important the difference between 2A and 2B? IMO complexity of both are 100% the same (the developer has to remember exactly the same things no matter if he/she uses 2A, 2B or the prev. comment proposal.

Also, note that this "reduced" syntax only can will save one line when used with individual fields, indexes (and keys), but haven't too much sense when fields, indexes (and keys) are added to one table (to create it), we'll continue having the same number of lines here (one for each field/index/key to be added). How would you implement this in the above suggestion? You'll need to mix manager and xmldb methods to do so, for example, adding one table, with your alternative:

$dbmanager = $DB->get_manager();

$table = $dbmanager->get_table_object('grade_letters'); // internally instantiates XMLDB objects

$field = dbmanager->get_table_object('id', REST, OF, FIELD, ATTRIBUTES, TO SPECIFY, HERE);
$table->add_field($field);

$field2 = dbmanager->get_table_object('id', REST, OF, FIELD, ATTRIBUTES, TO SPECIFY, HERE);
$table->add_field($field2);

$index1 = $dbmanager->get_index_object('contextid-lowerboundary'', XMLDB_INDEX_NOTUNIQUE, array('contextid', 'lowerboundary'));
$table->add_index($index1);

$dbmanager->crete_table($table);

(looks really horrible, requiring 2 lines per element and mixing manager and xmldb code. -1 here. The "pure" xmldb alternative is only 1 line per element.)

Just IMO... ciao

I wouldn't be unhappy with 2B, but hmm, that last code doesn't look right... why would we get an object for every field? Couldn't it be just the same number of lines as it is now?

$dbmanager = $DB->get_manager();

$table = $dbmanager->get_table_object('grade_letters'); // internally instantiates XMLDB objects

$table->add_comment( 'Some info about the table');
$table->add_field('id', XMLDB_TYPE_INTEGER, '10', false, XMLDB_NOTNULL, XMLDB_SEQUENCE, null, null, null);
$table->add_field('other', XMLDB_TYPE_INTEGER, '10', false, XMLDB_NOTNULL, XMLDB_SEQUENCE, null, null, null);
$table->add_field('primary', XMLDB_KEY_PRIMARY, array('id'));

$dbmanager->create_table($table);

But if you feel strongly about keeping xmldb_xxxx functions I would be OK with:

$dbmanager = $DB->get_manager();

$table = new xmldb_table('grade_letters'); // explicity instantiates XMLDB objects

$table->add_comment( 'Some info about the table');
$table->add_field('id', XMLDB_TYPE_INTEGER, '10', false, XMLDB_NOTNULL, XMLDB_SEQUENCE, null, null, null);
$table->add_field('other', XMLDB_TYPE_INTEGER, '10', false, XMLDB_NOTNULL, XMLDB_SEQUENCE, null, null, null);
$table->add_field('primary', XMLDB_KEY_PRIMARY, array('id'));

$dbmanager->create_table($table);

> I wouldn't be unhappy with 2B, but hmm, that last code doesn't look right... why would we get an object for every field? Couldn't it be just the same number of lines as it is now?

yup, absolutely!

What I was trying to remark is that...IMO, this methods, as commented in the suggestion:

$table = $dbmanager->get_table_object()
$field = $dbmanager->get_field_object()
$index = $dbmanager->get_index_object()

haven't too much sense, because if we implement them, then they should be always used and that will produce code like the one in my horrible example above. Of course, using:

$table->add_field()
$table->add_index()

is the correct approach. But then, we'll be using sometimes:

$index = $dbmanager->get_index_object('contextid-lowerboundary'', XMLDB_INDEX_NOTUNIQUE, array('contextid', 'lowerboundary'));

(to create one individual index, in the original suggestion)

and sometimes:

$table>add_index('contextid-lowerboundary'', XMLDB_INDEX_NOTUNIQUE, array('contextid', 'lowerboundary'));

(when creating indexes together with table creation)

That's the cause I think it's better to keep the manager - and all those get_xxxx_object() - out from the API.

I'd try to follow your latest example (in previous comment) for complex object creation + 2B for individual object creation.

How does that sound? Ciao

OK, that sounds like a good plan to me, thank you.

I guess the "old" object names, calls etc can easily be kept as deprecated for backward compatibility?

1st iteration committed: lowercase XMLDB constants, object, file and structure (no changes in public API yet)

2nd iteration committed: lowercase XMLDB table, field, index, key and statement. Also added wrapper class with old name to keep BC compatibility (no changes in public API yet).

Well,

this is the list of method names I'm going to rename/add to the "public" API:

$table->addFieldInfo(field_specs) ==> $table->add_field(field_specs)
$table->addKeyInfo(field_specs) ==> $table->add_key(key_specs)
$table->addIndexInfo(field_specs) ==> $table->add_index(index_specs)

$field->setAttributes ==> $field->set_attributes(field_specs) AND __construct()
$key->setAttributes ==> $key->set_attributes(key_specs) AND __construct()
$index->setAttributes ==> $key->set_attributes(key_specs) AND __construct()

With these objectives:

• The old API (left) will continue working in 2.0 (will be out in 2.1)
• The old API will show one debugging(DEBUG_DEVELOPER) message about being deprecated.
• XMLDB Editor will suggest new API.

That's all. I think we don't need anything more. Ciao

3rd iteration committed: xmldb_table "public" methods changed + debugging of deprecated ones until Moodle 2.1

$table->addFieldInfo(field_specs) ==> $table->add_field(field_specs)
$table->addKeyInfo(field_specs) ==> $table->add_key(key_specs)
$table->addIndexInfo(field_specs) ==> $table->add_index(index_specs)

Ciao

P.S: I've kept functional tests using old methods to check the "deprecate" message is working ok. One simple search/replace will to the job there, hopefully tomorrow (Nico or me)

4th iteration committed: xmldb_field, xmldb_index and xmldb_key "public" methods changed + debugging of deprecated ones until Moodle 2.1

$field->setAttributes ==> $field->set_attributes(field_specs) AND __construct()
$key->setAttributes ==> $key->set_attributes(key_specs) AND __construct()
$index->setAttributes ==> $key->set_attributes(key_specs) AND __construct()

Also, XMLDB Editor is suggesting new (shorter and lowercase) code, following the 2B) above and add_xxx() functions as suggested.

Resolving this as fixed.

Note: DML/DDL functional tests haven't been transformed to new API just to check that debugging is working oki and that deprecated functions perform well. Nico has been notified and will transforms tests API calls soon.