Fix #123: Fix schema issues in report_customsql_categories #1
Conversation
db/upgrade.php
Outdated
| $table->add_field('name', XMLDB_TYPE_CHAR, '255', null, XMLDB_NOTNULL, null, null, null); | ||
|
|
||
| // If the 'name' field already exists, ensure its constraint is set to NOTNULL | ||
| if ($dbman->field_exists($table, $table->getField('name'))) { |
There was a problem hiding this comment.
Wasn't sure where to put this, this just makes sure that if the column exists already make sure it has the correct constraint defined on #L235.
There was a problem hiding this comment.
Ah, yes the field_exists check is good to have in the new upgrade block.
db/upgrade.php
Outdated
| if (!$DB->record_exists('report_customsql_categories', array('name' => $category->name))) { | ||
| $category->id = $DB->insert_record('report_customsql_categories', $category); | ||
| // Update the existing query category ids, to move them into this category. | ||
| $sql = 'UPDATE {report_customsql_queries} SET categoryid =' . $category->id; |
There was a problem hiding this comment.
Prior, this would be run unconditionally which would cause an error since #L123 would be skipped.
There was a problem hiding this comment.
I'm not sure I understand what you mean. Could you elaborate on this?
I can definitely see an issue if this was kept in the new upgrade block.
There was a problem hiding this comment.
My understanding is that sites that already have this plugin installed will have the report_customsql_categories record already, so if (!$DB->record_exists('report_customsql_categories', ... will be skipped, leaving $category->id undefined. This was causing $DB->execute($sql); to throw an error.
There was a problem hiding this comment.
Correct. As pointed out by the other comments, I believe only the updates needed should be in the new block, instead of trying to recreate the table and the surrounding logic, etc.
There was a problem hiding this comment.
Agreed, my bad there. I've left the original block alone and created a new one with what we need to resolve the failed schema check, see commit: bacdd6a
There was a problem hiding this comment.
There seems to be more changes here than expected.
I would expect this patch to address only the issue described in the linked issue (moodleou#123).
- to ensure the 2 missing columns exist,
- and to fix up the expected default value of
name, which according to the current state of this upgrade block, and the current install.xml file, should be not set (or NULL).
The reason for this, is (probably) at some point, the Miscellaneous category created, should not be a fixed English string, but instead a lang string that could be set in a different language. Thus, it doesn't make sense for it to necessarily exist as English only.
db/upgrade.php
Outdated
| $table->add_field('name', XMLDB_TYPE_CHAR, '255', null, XMLDB_NOTNULL, null, null, null); | ||
|
|
||
| // If the 'name' field already exists, ensure its constraint is set to NOTNULL | ||
| if ($dbman->field_exists($table, $table->getField('name'))) { |
There was a problem hiding this comment.
Ah, yes the field_exists check is good to have in the new upgrade block.
db/upgrade.php
Outdated
| if (!$DB->record_exists('report_customsql_categories', array('name' => $category->name))) { | ||
| $category->id = $DB->insert_record('report_customsql_categories', $category); | ||
| // Update the existing query category ids, to move them into this category. | ||
| $sql = 'UPDATE {report_customsql_queries} SET categoryid =' . $category->id; |
There was a problem hiding this comment.
I'm not sure I understand what you mean. Could you elaborate on this?
I can definitely see an issue if this was kept in the new upgrade block.
jnlar
force-pushed
the
issue123-categories-schema
branch
from
4c24779 to
882afea
Compare
Awesome 👍 commits have been squashed and can confirm upgrade + schema checks went okay locally |
Fixes issue moodleou#123
At some point the name columns constraint has been changed to NULL leading to this error.