Merge lp:~quam-plures-core/quam-plures/qp-bug_526380 into lp:quam-plures

[Yabs]

Implemented blueyeds fix for this bug

¥

[EdB (edb)]

Downloaded and will test, but does anyone have a plugin that triggers the bug ... or will the snippet in the bug report work as a pretend plugin? I recall the nature of this bug but I haven't migrated any plugins that make a database table.

[Yabs (yabs)]

IIRC the snippet in a pretend plugin should trigger it (if fix fails) ... been a smidge since I played with the plugin that "discovered" that bug though ... not sure if the plugin even exists since I tidied server :-S

Note : 100% untested, I just merged diffs, must go through a test install, but I don't expect problems from blueyed code ;)

¥

[EdB (edb)]

Yeah no problem with blueyed code but the idea and all that ya know? Anyway tested by modificating the 'test' plugin and identifying the problem in core-of-the-moment then seeing that the problem does not exist in this branch. To verificate the problem one simply has to install a plugin that might trigger the but, then disable the plugin, then enable it again.

So should I add the altered test plugin to this branch? I also removed the bit that stops the user settings from showing up, and fixed a comment in the code about translation stuff.

Will merge after knowing if test should be here or have it's own branch :)

review: Approve

[Yabs (yabs)]

Yeah, even blueyed's code needs review ... especially since it was diff'd in by a blonde :D

I'd need to see your test plugin code before I could answer that ;)

¥

[Tilman Blumenbach (tblue)]

I will try and merge blueyed's tests for that bug. That should make testing easier.

[EdB (edb)]

The test plugin will get it's own branch. Much cleaner that way :)

Tblue are you saying I should hold off on doing this merge?

[Tilman Blumenbach (tblue)]

> Tblue are you saying I should hold off on doing this merge?

Yes, please. But feel free to create a branch for your plugin.

[Bush League Critic (bushleaguecritic)]

Pulled this test branch and jury-rigged a plugin that creates a table
(GetDbLayout) based on the code snippet yabs provided in the original
bug report. I had no issues/errors installing the plugin and the table
was created exactly as requested.

Should that have been enough to trigger the original bug?

BLC

[EdB (edb)]

Hi BLC. The bug is triggered when you disable then enable a plugin that created a database that includes specific field types. If your DB has a float and/or a decimal field, then disable and enable will ... not show the bug in this branch. Apply the same plugin to the current core and you'll see the bug in all it's glory.

There is also now a test plugin branch that adds a table that triggers the bug if you want it.

[Tilman Blumenbach (tblue)]

OTOH, looks like the relevant part is exactly the same as blueyed's code, so perhaps adding tests is not necessary right now, but we should really start using tests at some later point!

[Tilman Blumenbach (tblue)]

Did the same as dmassay. Could reproduce behaviour described in bug report before merging in this branch, could not reproduce it afterwards.

So, looks fine.

review: Approve

=== modified file 'qp_inc/_core/model/db/_upgrade.funcs.php'
--- qp_inc/_core/model/db/_upgrade.funcs.php	2009-12-21 17:27:20 +0000
+++ qp_inc/_core/model/db/_upgrade.funcs.php	2010-07-27 14:17:42 +0000
@@ -142,7 +142,7 @@
 
 		if( preg_match( '|^(\s*CREATE TABLE\s+)(IF NOT EXISTS\s+)?([^\s(]+)(.*)$|is', $qry, $match) )
 		{
-			$tablename = db_delta_remove_backticks(preg_replace( $DB->dbaliases, $DB->dbreplaces, $match[3] ));
+			$tablename = db_delta_remove_quotes(preg_replace( $DB->dbaliases, $DB->dbreplaces, $match[3] ));
 			$qry = $match[1].( empty($match[2]) ? '' : $match[2] ).$tablename.$match[4];
 
 			$items[strtolower($tablename)][] = array(
@@ -159,7 +159,7 @@
 		}
 		elseif( preg_match( '|^(\s*INSERT INTO\s+)([\S]+)(.*)$|is', $qry, $match) )
 		{
-			$tablename = db_delta_remove_backticks(preg_replace( $DB->dbaliases, $DB->dbreplaces, $match[2] ));
+			$tablename = db_delta_remove_quotes(preg_replace( $DB->dbaliases, $DB->dbreplaces, $match[2] ));
 			$items[strtolower($tablename)][] = array(
 				'queries' => array($match[1].$tablename.$match[3]),
 				'note' => '',
@@ -167,7 +167,7 @@
 		}
 		elseif( preg_match( '|^(\s*UPDATE\s+)([\S]+)(.*)$|is', $qry, $match) )
 		{
-			$tablename = db_delta_remove_backticks(preg_replace( $DB->dbaliases, $DB->dbreplaces, $match[2] ));
+			$tablename = db_delta_remove_quotes(preg_replace( $DB->dbaliases, $DB->dbreplaces, $match[2] ));
 			$items[strtolower($tablename)][] = array(
 				'queries' => array($match[1].$tablename.$match[3]),
 				'note' => '',
@@ -308,7 +308,7 @@
 		{ // For every field line specified in the query
 			// Extract the field name
 			preg_match( '|^([^\s(]+)|', trim($create_definition), $match );
-			$fieldname = db_delta_remove_backticks($match[1]);
+			$fieldname = db_delta_remove_quotes($match[1]);
 			$fieldname_lowered = strtolower($fieldname);
 
 			$create_definition = trim($create_definition, ", \r\n\t");
@@ -319,17 +319,18 @@
 					'create_definition' => $create_definition,
 				);
 
-				if( ! preg_match( '~^(PRIMARY(?:\s+KEY)|UNIQUE(?:\s+(?:INDEX|KEY))?|KEY|INDEX) (?:\s+(\w+))? (\s+USING \w+)? \s* \((.*)\)$~ix', $create_definition, $match ) )
+				if( !  preg_match( '~^(PRIMARY(?:\s+KEY)|(?:FULLTEXT|UNIQUE)(?:\s+(?:INDEX|KEY))?|KEY|INDEX) (?:\s+()     (\w+)      )? (\s+USING\s+\w+)? \s* \((.*)\)$~ix', $create_definition, $match )
+					&& ! preg_match( '~^(PRIMARY(?:\s+KEY)|(?:FULLTEXT|UNIQUE)(?:\s+(?:INDEX|KEY))?|KEY|INDEX) (?:\s+([`"])([\w\s]+)\\2)? (\s+USING\s+\w+)? \s* \((.*)\)$~ix', $create_definition, $match ) )
 				{ // invalid type, should not happen
 					debug_die( 'Invalid type in $indices: '.$create_definition );
 				}
 				$add_index['keyword'] = $match[1];
-				$add_index['name'] = strtoupper($match[2]);
-				$add_index['type'] = $match[3]; // "USING [type_name]"
-				$add_index['col_names'] = explode( ',', $match[4] );
+				$add_index['name'] = strtoupper($match[3]);
+				$add_index['type'] = $match[4]; // "USING [type_name]"
+				$add_index['col_names'] = explode( ',', $match[5] );
 				foreach( $add_index['col_names'] as $k => $v )
 				{
-					$add_index['col_names'][$k] = strtolower(db_delta_remove_backticks(trim($v)));
+					$add_index['col_names'][$k] = strtolower(db_delta_remove_quotes(trim($v)));
 				}
 
 				if( $fieldname_lowered == 'primary' )
@@ -438,7 +439,7 @@
 			{
 				$index_pattern .= 'PRIMARY(\s+KEY)?';
 				// optional primary key name:
-				$index_pattern .= '(\s+\w+)?';
+				$index_pattern .= '(\s+[`"]?\w+[`"]?)?';
 			}
 			elseif( $index_data['unique'] )
 			{
@@ -446,11 +447,11 @@
 			}
 			else
 			{
-				$index_pattern .= '(INDEX|KEY)';
+				$index_pattern .= '(INDEX|(?:FULLTEXT\s+)?KEY)';
 			}
 			if( $index_name != 'PRIMARY' )
 			{
-				$index_pattern .= '(\s+`?'.$index_name.'`?)?'; // optionally in backticks (and index name is optionally itself)
+				$index_pattern .= '(\s+[`"]?'.$index_name.'[`"]?)?'; // optionally in backticks (and index name is optionally itself)
 			}
 
 			$index_columns = '';
@@ -462,7 +463,7 @@
 					$index_columns .= '\s*,\s*';
 				}
 				// Add the field to the column list string
-				$index_columns .= '`?'.$column_data['fieldname'].'`?'; // optionally in backticks
+				$index_columns .= '[`"]?'.$column_data['fieldname'].'[`"]?'; // optionally in backticks
 				if( ! empty($column_data['subpart']) )
 				{
 					$index_columns .= '\s*\(\s*'.$column_data['subpart'].'\s*\)\s*';
@@ -486,7 +487,7 @@
 				if( ! preg_match( '~^\w+\s+[^(]~', $index['create_definition'], $match ) )
 				{ // no key name given, make the name part optional, if it's the default one:
 					// (Default key name seems to be the first column, eventually with "_\d+"-suffix)
-					$auto_key = db_delta_remove_backticks(strtoupper($index['col_names'][0]));
+					$auto_key = db_delta_remove_quotes(strtoupper($index['col_names'][0]));
 					if( isset($used_auto_keys[$auto_key]) )
 					{
 						$used_auto_keys[$auto_key]++;
@@ -606,7 +607,7 @@
 			unset($type_matches); // have we detected the type as matching (for optional length param)
 			$fieldtype = '';
 
-			$pattern_field = '`?'.$tablefield->Field.'`?'; // optionally in backticks
+			$pattern_field = '[`"]?'.$tablefield->Field.'[`"]?'; // optionally in backticks
 
 			// Get the field type from the query
 			if( preg_match( '~^'.$pattern_field.'\s+ (TINYINT|SMALLINT|MEDIUMINT|INTEGER|INT|BIGINT|REAL|DOUBLE|FLOAT|DECIMAL|DEC|NUMERIC) ( \s* \([\d\s,]+\) )? (\s+ UNSIGNED)? (\s+ ZEROFILL)? (.*)$~ix', $column_definition, $match ) )
@@ -617,9 +618,18 @@
 				{ // synonym
 					$fieldtype = 'INT';
 				}
-				elseif( $fieldtype == 'DECIMAL' )
+				elseif( $fieldtype == 'DEC' )
 				{ // synonym
-					$fieldtype = 'DEC';
+					$fieldtype = 'DECIMAL';
+				}
+				// Build pattern to match type: Length is optional (for int types) or precision/scale (must match exactly):
+				if( substr($fieldtype, -3) == 'INT' )
+				{ // length is optional/not relevant
+					$matches_pattern = '~^'.preg_replace( '~\((\d+)\)~', '(\(\d+\))?', $tablefield->Type ).'$~i';
+				}
+				else
+				{
+					$matches_pattern = '~^'.preg_quote($tablefield->Type, '~').'$~i';
 				}
 
 				if( isset($match[2]) )
@@ -637,8 +647,6 @@
 
 				$field_to_parse = $match[5];
 
-				// The length param is optional:
-				$matches_pattern = '~^'.preg_replace( '~\((\d+)\)~', '(\(\d+\))?', $tablefield->Type ).'$~i';
 				$type_matches = preg_match( $matches_pattern, $fieldtype );
 			}
 			elseif( preg_match( '~^'.$pattern_field.'\s+(DATETIME|DATE|TIMESTAMP|TIME|YEAR|TINYBLOB|BLOB|MEDIUMBLOB|LONGBLOB|TINYTEXT|TEXT|MEDIUMTEXT|LONGTEXT) ( \s+ BINARY )? (.*)$~ix', $column_definition, $match ) )
@@ -838,6 +846,7 @@
 				}
 			}
 
+			// TODO: "COLLATE" and other attribute handling should happen here, based on $field_to_parse
 
 			if( ! isset($type_matches) )
 			{ // not tried to match before
@@ -859,7 +868,7 @@
 			}
 			else
 			{ // implicit default, see http://dev.mysql.com/doc/refman/4.1/en/data-type-defaults.html
-				if( preg_match( '~^(TINYINT|SMALLINT|MEDIUMINT|INTEGER|INT|BIGINT|REAL|DOUBLE|FLOAT|DECIMAL|DEC|NUMERIC)$~', $fieldtype ) )
+				if( preg_match( '~^(TINYINT|SMALLINT|MEDIUMINT|INTEGER|INT|BIGINT|REAL|DOUBLE|FLOAT|DECIMAL|DEC|NUMERIC)~', $fieldtype ) )
 				{ // numeric
 					$update_default = '0';
 					$update_default_set = '0';
@@ -1052,7 +1061,8 @@
 		array_shift( $items[$table_lowered] );
 
 		// Add the remaining indices (which are not "inline" with a column definition and therefor already handled):
-		foreach( $indices as $index )
+		$add_index_queries = array();
+		foreach( $indices as $k => $index )
 		{
 			if( empty($index['create_definition']) )
 			{ // skip "inline"
@@ -1064,14 +1074,35 @@
 				$query .= ' DROP PRIMARY KEY,';
 				unset( $obsolete_indices['PRIMARY'] );
 			}
-			// Push a query line into $items that adds the index to that table
-			$items[$table_lowered][] = array(
+
+			// Create a query that adds the index to the table
+			$query = array(
 				'queries' => array($query.' ADD '.$index['create_definition']),
 				'note' => 'Added index <strong>'.$index['create_definition'].'</strong>',
 				'type' => 'add_index' );
+
+			// Check if the index creation has to get appended after any DROPs (required for indices with the same name)
+			$append_after_drops = false;
+			foreach( $obsolete_indices as $obsolete_index )
+			{
+				if( strtolower($obsolete_index['name']) == strtolower($index['name']) )
+				{
+					$append_after_drops = true;
+					break;
+				}
+			}
+			if( $append_after_drops )
+			{ // do this after any DROPs (i.e. KEY name changes)
+				$add_index_queries[] = $query;
+			}
+			else
+			{ // this needs to get done before any other DROPs
+				// to prevent e.g. "Incorrect table definition; there can be only one auto column and it must be defined as a key(Errno=1075)"
+				$items[$table_lowered][] = $query;
+			}
 		}
 
-
+		// Now add queries to drop any (maybe changed!) indices
 		foreach( $obsolete_indices as $index_info )
 		{
 			// Push a query line into $items that drops the index from the table
@@ -1080,6 +1111,9 @@
 				'note' => 'Dropped index <strong>'.$index_info['name'].'</strong>',
 				'type' => 'drop_index' );
 		}
+
+		// Add queries to (re)create (maybe changed indices) to the end
+		$items[$table_lowered] = array_merge($items[$table_lowered], $add_index_queries);
 	}
 
 
@@ -1134,18 +1168,24 @@
 
 
 /**
- * Remove backticks around a field/table name.
- *
- * "backtick" means "single backquote" (`)
+ * Remove quotes/backticks around a field/table name.
  *
  * @param string Field name
+ * @param string List of quote chars to remove
  * @return string
  */
-function db_delta_remove_backticks($fieldname)
+function db_delta_remove_quotes($fieldname, $quotes = '`"')
 {
-	if( substr($fieldname, 0, 1) == '`' && substr($fieldname, -1) == '`' )
-	{ // backticks:
-		$fieldname = substr($fieldname, 1, -1);
+	$quotes_len = strlen( $quotes );
+
+	for( $i = 0; $i < $quotes_len; $i++ )
+	{
+		$char = $quotes[$i];
+		if( substr($fieldname, 0, 1) == $char && substr($fieldname, -1) == $char )
+		{ // found quotes:
+			$fieldname = substr($fieldname, 1, -1);
+			return $fieldname;
+		}
 	}
 	return $fieldname;
 }
@@ -1154,7 +1194,7 @@
 /**
  * Alter the DB schema to match the current expected one ({@link $schema_queries}).
  *
- * @todo if used by install only, then put it into the install folde!!!
+ * @todo if used by install only, then put it into the install folder!!!
  *
  * @param boolean Display what we've done?
  */
@@ -1162,9 +1202,11 @@
 {
 	global $schema_queries, $DB, $debug;
 
+	// Go through all tables:
 	foreach( $schema_queries as $table => $query_info )
 	{
-		$items_need_update = db_delta( $query_info[1], array('drop_column', 'drop_index') );
+		// Look for differences between terrain & map:
+		$items_need_update = db_delta( $query_info[1], array('drop_column', 'drop_index'), false );
 
 		if( empty($items_need_update) )
 		{
@@ -1185,7 +1227,7 @@
 			}
 		}
 		else
-		{ // the same, but with output
+		{ // execute & output
 			foreach( $items_need_update as $table => $itemlist )
 			{
 				if( count($itemlist) == 1 && $itemlist[0]['type'] == 'create_table' )