ACL save may cause DB error

[JavaWoman]

martin_misuth on #wikka ran into this:

Query failed: INSERT INTO wikka_acls SET page_tag = 'HomePage', read_acl = '+' (Field 'write_acl' doesn't have a default value)

Checking the acls table as generated by 1.1.6.5, only the page_tag column seems to have a default value (empty string: appears as two single quotes), the read/write/comment columns do not.

Suggested solution: generate the table with empty string as default for every column.

[JavaWoman]

More info: 1. It appears that on a TEXT field, you cannot set a default value 2. it appears that some, but not all, MySQL installations (depending on version and/or global settings) will produce the above error when updating a column but not other columns that have no default

Possible solutions: 1. define our table without the NOT NULL qualifier on the read/write/comment columns 2. when updating a column or creating a new record, always write a whole record (instead of setting a single column), making sure the other columns have the original value or an empty string as default

[JavaWoman]

martin_misuth enquired in the #mysql channel, and got the reply that not being able to set a default for a TEXT field is by design. So, we must take this into account in our code.

Best solution is to make sure to explicitly set all columns when creating a record (updating then won't be a problem); default values should be represented by an empty string.

DotMG presented a preliminary fix in  http://pastebin.com/m24db1209, slightly cleaned up by me in  http://pastebin.com/m715e8809 . (Meanwhile I'm working on a total overhaul of ACLs handling which will make this code unnecessary.)

[DarTar]

good catch - but is there a reason why we use TEXT instead of VARCHAR ?

[JavaWoman]

Replying to DarTar:

good catch - but is there a reason why we use TEXT instead of VARCHAR ?

Lists can easily get long when summing up individual users and/or interspersing with comments, and until MySQL 5.0.3 a VARCHAR can hold only up to 255 characters (see  http://dev.mysql.com/doc/refman/5.0/en/char.html ) - we support much older versions. TEXT fields do not have such a low limitation (so we really need that), but this is where variations in installation and versions come in: see  http://dev.mysql.com/doc/refman/5.0/en/blob.html .

[DarTar]

Then let's apply the patch to 1.1.6.6 (ticket assigned to DotMG as author of the patch) and implement the overhauled ACL management code in 1.1.7 (pls open new ticket - 1.1.7 has changes in ACL management for comments anyway) so we don't delay the release of 1.1.6.6.

[DarTar]

 http://pastebin.com/m715e8809

	function SaveACL($tag, $privilege, $list) {
		// the $default will be put in the SET statement of the SQL for default values
		$default = "read_acl = '', write_acl = '', comment_acl = '', ";
		// we strip the privilege_acl from default, to avoid redundancy
		$default = str_replace($privilege."_acl = '',", '', $default);
		if ($this->LoadACL($tag, $privilege, 0)) $this->Query("UPDATE ".$this->config["table_prefix"]."acls SET ".mysql_real_escape_string($privilege)."_acl = '".mysql_real_escape_string(trim(str_replace("\r", "", $list)))."' WHERE page_tag = '".mysql_real_escape_string($tag)."' LIMIT 1");
		else $this->Query("INSERT INTO ".$this->config["table_prefix"]."acls SET $default page_tag = '".mysql_real_escape_string($tag)."', ".mysql_real_escape_string($privilege)."_acl = '".mysql_real_escape_string(trim(str_replace("\r", "", $list)))."'");
	}

[DotMG]

(In [1236]) refs #811

Applied code in #811#comment:6

[DarTar]

works fine for me

[DarTar]

Note: maybe we should adjust our workflow, since the change to be tested is already committed it doesn't make much sense to have a transition such as testing-> commit, what do you guys think?

[BrianKoontz]

Yes, we should probably just skip commit altogether and go directly from passed->resolve as...