Non-core contributed modules

While looking to sort out a proper solution, I coded a work-around for this problem and then encountered another related one: (I had to put an error_reporting(E_ALL) into the code to get the actual error message)

--------------------------------------------------------------------------------
(postgres7): ALTER TABLE mdl25b_questionnaire_response_single_id_seq RENAME TO mdl25b_questionnaire_resp_single_id_seq
--------------------------------------------------------------------------------
Success
--------------------------------------------------------------------------------
(postgres7): SELECT COUNT FROM mdl25b_questionnaire_survey
--------------------------------------------------------------------------------
Notice: Undefined variable: field in /var/www/html/gcm77/moodle25/mod/questionnaire/db/upgrade.php on line 179
Fatal error: Call to a member function setAttributes() on a non-object in /var/www/html/gcm77/moodle25/mod/questionnaire/db/upgrade.php on line 179

The problem seems to be these lines of code beginning at line 101 of upgrade.php - basically, it only creates the $table and $field objects within the conditional code, but it then attempts to use them after the end of that condition. This probably works if there are records in the 'questionnaire_survey' table, but I was doing an upgrade of a newly-installed environment and there weren't any.

/// Upgrade the questionnaire_survey table to use integer timestamps.
if (($numrecs = count_records('questionnaire_survey')) > 0) {
$table = new XMLDBTable('questionnaire_survey');
$field = new XMLDBField('changed');
$field->setAttributes(XMLDB_TYPE_CHAR, '20');
$status &= change_field_type($table, $field);
$status &= change_field_precision($table, $field);
$recstart = 0;
$recstoget = 100;
while ($recstart < $numrecs) {
if ($records = get_records('questionnaire_survey', '', '', '', '*', $recstart, $recstoget)) {
foreach ($records as $record) { $tstampparts = explode(' ', $record->changed); $dateparts = explode('-', $tstampparts[0]); $timeparts = explode(':', $tstampparts[1]); $time = mktime($timeparts[0], $timeparts[1], $timeparts[2], $dateparts[1], $dateparts[2], $dateparts[0]); $status &= set_field('questionnaire_survey', 'changed', $time, 'id', $record->id); }
}
$recstart += $recstoget;
}
}
$field->setAttributes(XMLDB_TYPE_INTEGER, '10');
$status &= change_field_type($table, $field);
$status &= change_field_precision($table, $field);

/// Upgrade the questionnaire_question_type table to use typeid.
unset($table);
unset($field);

What I've tried is moving the following 5 lines from just inside the IF statement to instead be just before it, and this seems to allow the upgrade to work:

$table = new XMLDBTable('questionnaire_survey');
$field = new XMLDBField('changed');
$field->setAttributes(XMLDB_TYPE_CHAR, '20');
$status &= change_field_type($table, $field);
$status &= change_field_precision($table, $field);

I have also encountered a problem when doing a clean install:

ERROR: column "changed" is of type timestamp without time zone but default expression is of type integer HINT: You will need to rewrite or cast the expression.

ADOConnection._Execute(CREATE TABLE mdl22_questionnaire_survey (
id BIGSERIAL,
name VARCHAR(64) NOT NULL DEFAULT '',
owner VARCHAR(16) NOT ..., false) % line 891, file: adodb.inc.php
ADOConnection.Execute(CREATE TABLE mdl22_questionnaire_survey (
id BIGSERIAL,
name VARCHAR(64) NOT NULL DEFAULT '',
owner VARCHAR(16) NOT ...) % line 89, file: dmllib.php
execute_sql(CREATE TABLE mdl22_questionnaire_survey (
id BIGSERIAL,
name VARCHAR(64) NOT NULL DEFAULT '',
owner VARCHAR(16) NOT ..., true) % line 2232, file: dmllib.php
execute_sql_arr(Array[41]) % line 635, file: ddllib.php
install_from_xmldb_file(/var/www/html/gcm77/moodle22/mod/questionnaire/db/install.xml) % line 387, file: adminlib.php

The problem here appears to be line 45 of mod/questionnaire/install.xml:

<FIELD NAME="changed" TYPE="datetime" NOTNULL="false" DEFAULT="0" SEQUENCE="false" ENUM="false" PREVIOUS="thank_body"/>

Mike, please correct me if I'm wrong, but I think the TYPE="datetime" should actually be TYPE="int" because this would fit with the default value of 0, and because this appears to be what is done in the upgrade.php script.

I attach a patch to change this: patch_install_xml.txt

Sorry, I've been trying to develop a rough-and-ready fix for the upgrade script, mod/questionnaire/db/upgrade.php to make it work in Postgres environments so that I could put a patch on this bug, but have failed and have had to alter the database table manually to get it to work.

Basically, there are three problems :

1. conversion of the field "changed" in the questionnaire_survey table from a timestamp into an integer via the intermediate format of a character(20) field - this needs to be something like a char. 30 to allow for high-precision timestamps. This is on line 103, $field->setAttributes(XMLDB_TYPE_CHAR, '20');

2. Objects are defined inside a conditional expression at line 100, and then used outside it. The five lines of code beginning with line 101 need to be moved in front of the IF statement, i.e.

$table = new XMLDBTable('questionnaire_survey');
$field = new XMLDBField('changed');
$field->setAttributes(XMLDB_TYPE_CHAR, '20');
$status &= change_field_type($table, $field);
$status &= change_field_precision($table, $field);
if (($numrecs = count_records('questionnaire_survey')) > 0) {

3. Seven fields which had values of Y or N now need to have values of y or n.
There is code to put a new enumeration constraint on the field, but for postgres there also needs to be a command to take the old constraint off, dml commands to change the uppercase values to lowercase in the existing records, a command to set a new default value as lowercase, and then the command to set a new constraint:

The seven fields are:
Table questionnaire_question, fields 'required', 'deleted' and 'public'.
Table questionnaire_question_type, field 'has_choices'
Table questionnaire_response, field 'complete'
Table questionnaire_response_bool, field 'choice_id'
Table questionnaire_survey, field 'public'

so, for example, where it currently says:

— Begin code —

$table = new XMLDBTable('questionnaire_question');

$field = new XMLDBField('required');
$field->setAttributes('char', '1', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, XMLDB_ENUM, array('y', 'n'), '\'n\'');
$result &= execute_sql_arr($table->getModifyEnumSQL($CFG->dbtype, $CFG->prefix, $field, false), false);
unset($field);

— End code —

I have been changing it to this:

— Begin code —

$table = new XMLDBTable('questionnaire_question');

//Take the old constraint off
$field = new XMLDBField('required');
$field->setAttributes('char', '1', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, false, null, '\'n\'');
$result &= execute_sql_arr($table->getModifyEnumSQL($CFG->dbtype, $CFG->prefix, $field, false), false);
unset($field);

//Change the current values
set_field( 'questionnaire_question', 'required', 'y', 'required', 'Y' );
set_field( 'questionnaire_question', 'required', 'n', 'required', 'N' );

$field = new XMLDBField('required');
$field->setAttributes('char', '1', XMLDB_UNSIGNED, XMLDB_NOTNULL, null, XMLDB_ENUM, array('y', 'n'), '\'n\'');
$result &= execute_sql_arr($table->getModifyEnumSQL($CFG->dbtype, $CFG->prefix, $field, false), false);

//Now set the new default
$result &= execute_sql_arr($table->getModifyDefaultSQL($CFG->dbtype, $CFG->prefix, $field, false), false);
unset($field);

— End code —

What is defeating me is the call to getModifyDefaultSQL, because it is returning invalid SQL like this, without a type for the field:

ALTER TABLE mdl28_questionnaire_question ADD deleted_alter_column_tmp

I think that Eloy may be the best person to comment on why that's happening, and may also want to extend the getModifyEnumSQL method when running in Postgres so that it does all the changes to the constraints in one function. That would be more elegant than my approach.

Hi Gareth,

some comments about your nice research in the questionnaire module:

1) Yep, it seems that char(20) isn't enough to store such info. So more chars are needed, for sure. And the field must end being one BIGINT(10), for sure. So the install.xml file must declare it as BIGINT(10) too, for sure.

2) Your appreciation about code within the conditional shouldn't be there at all. 100% agree. In fact, the general rule should be to perform ALL the DDL statements (calls to dmllib.php) unconditionally. And then, process records (DML) conditionally, if desired. But DDL changes must not be inside those conditions. Not at all.

3) About changes in check constraints (formerly "enums"). I really think that the correct path to perform those changes is:

a- Drop the check constraint.
b- Update contents to transform Y->y and N->n
c- Create the check constraint.

IMO it's impossible (not always computable) to make that in only one change_field_enum() call, because this case is about a trivial uppercase to lowercase transformation but this cannot be assumed always at all. For example, see, in "admin/xmldb/actions/test/test.class.php" tests number 36, 37 and 38. There one enum is created, then dropped and then recreated. That's the way, IMO. (note that those tests are special testing code and you never need to access $table->getModifyEnumSQL() at all. It's there for testing checks, only!. Supress that line from your upgrade code. See next paragraph).

Also, I've seen that code in questionnaire/db/upgrade.php does some INCORRECT uses of XMLDB objects, trying to get SQL code directly from XMLDB internals. That's really a bad practice and upgrade.php should use EXCLUSIVELY ddllib.php functions, that have some extra checks no present with direct access.

I hope this helps you fixing this. Sorry by the delay. Apologises by my lack of time these days.... And thanks again for your work!

Ciao

In CONTRIB-323 I recommended that the default value be removed for the changed field in mdl_questionnaire_survey in the install.xml - this seems to resolve the issue on a fresh install although I admit that I have not read the replies to this issue but thought it might be helpful. Peace - Anthony

I've changed the priority of this code to blocker since it prevents the module from being installed or upgraded in Moodle 1.9. Mike when do you think you might have a chance to look at this? Let me know how I can be helpful in fixing this. Peace - Anthony

Changed upgrade code to change enum case in several steps so that it will work with PostGres.

Great! Thanks!

Eloy -

I see you changed the 'fix' version from 1.9 and 2.0 to 1.9.1.

For my reference in the future, why did you decide this?

I figure since this is a 'contrib' module, it makes sense to give it a generic release version, since its not tied to a specific 1.9 release, no? Also, I updated the HEAD version which I believe is 2.0.

Hi Mike,

good question. Since some weeks ago I use to set the "fix" version to the next unreleased version that is going to address the problem. If the unreleased version is HEAD (2.0) then only 2.0 is selected. If the unreleased version is 19_STABLE (1.9.x) then only 1.9.x is selected (we always assume that the fix is merged to HEAD). If the unreleased version are 18_STABLE and 19_STABLE (1.8.x and 1.9.x) then both 1.8.x and 1.9.x are selected (but not HEAD).

So summarizing, HEAD only is the "fix" version when something is EXCLUSIVELY fixed in HEAD. In the other side, when something is fixed in one or more branches, the next unreleased version in those branches are the "fix" version.

That way, the list of bugs/improvements for each release will be more realistic. Else, the HEAD release will show some sort of "excess" of things fixed (when they were fixed months ago for a previous release).

That's the approach I follow since some weeks ago. Before then I was doing the same than you, always adding HEAD. I sincerely think it's better the "new" way.

Finally, all this has a lot of sense with core and perhaps it's more abstract in contrib. But really, I think we can (must) the same "fix for" schema, following closely the releases (in fact we could publish the list of bug fixed performed under contrib for each release, exactly equivalent to the core list:

http://tracker.moodle.org/secure/ReleaseNote.jspa?projectId=10033&styleName=Html&version=10262

And that's all, ciao

I look at the 'fixed' versions as the minimum one I would have to download in order to incorporate the fix. With this contrib fix, the fix can be used for any version of 1.9.

So in the case of a contrib module, if I fix 1.9 (for example), that module can be used in any released version of 1.9, and any un-released version (assuming there are no new bugs). Since its not tied exclusively to a Moodle core release, I don't think using a Moodle release number is appropriate.

I'm worried if we set a version like 1.9.1, then people may think the fix will not work with 1.9.0 (or 1.9+).

Thoughts?

Hi Mike,

I see your point.

But I really think it's the same under core. If we put something as fixed for 1.9.1 it means that 1.9.1 release will include it for sure. But also 1.9+ dailies since the day of the fix will.

And exactly the same happens for contrib modules. I just think it's a matter of think that the contrib module is part of core, then it has perfect sense to think the same way than in core.

Also you'll offer daily downloads per branch:

• Download Questionnaire for Moodle 1.8
• Download Questionnaire for Moodle 1.9
• ...

no matter of when one bug was fixed (same than core, once again). And with the extra info in tracker pointing when each bug was exactly fixed.

I really think it's better to have one unique schema about how to fulfil versions in both projects. And I think we can use the core one into contrib. I see it clearer than adding existing releases in the "fix for" field. If something has been released, it's impossible to include one fix for it. It has been released!

Imagine the module part of core, with real releases (not only dailies). I think that's the key. That way the core "fix for" schema has perfect sense applied to contrib.

Just my personal opinion, of course. Ciao

Mike - Thanks for raising a good question that was in need of clarification for CONTRIB. After reading your questions, comments and Eloy's replies, I would like for what we do in CONTRIB to be as consistent with what happens in CORE. This makes life easier for the core developers since there is one set of rules but it also allows us to use CONTRIB to train tomorrow's developers in the practices they will need. It makes sense to have good release documentation and gradually folks will understand what it means to say it was fixed in 1.9.1. Peace - Anthony

Just to confirm, my postgres upgrade worked first time this time. Yay. Thanks for all the hard work on this every-one.

Now I can get on testing the roles and permissions stuff.