Addshore has submitted this change and it was merged.
Change subject: Added some comments and todos.
......................................................................
Added some comments and todos.
Change-Id: I89db453aa4587f5ec83d3e86d7fdf7c8f774294e
---
M src/MySQL/MySQLFieldSqlBuilder.php
M src/MySQL/MySQLIndexSqlBuilder.php
M src/MySQL/MySQLTableSqlBuilder.php
M src/SQLite/SQLiteFieldSqlBuilder.php
M src/SQLite/SQLiteUnEscaper.php
M src/Schema/Definitions/FieldDefinition.php
6 files changed, 20 insertions(+), 8 deletions(-)
Approvals:
Addshore: Looks good to me, approved
jenkins-bot: Verified
diff --git a/src/MySQL/MySQLFieldSqlBuilder.php
b/src/MySQL/MySQLFieldSqlBuilder.php
index 3adbc40..b663105 100644
--- a/src/MySQL/MySQLFieldSqlBuilder.php
+++ b/src/MySQL/MySQLFieldSqlBuilder.php
@@ -68,13 +68,14 @@
*/
protected function getFieldType( $fieldType ) {
switch ( $fieldType ) {
+ // No datatype for short strings, i.e. VARCHAR? TEXT or
BLOB fields should not be used for that.
case FieldDefinition::TYPE_INTEGER:
return 'INT';
case FieldDefinition::TYPE_FLOAT:
- return 'FLOAT';
+ return 'FLOAT'; // No support for Decimal?
Won't we need that?
//todo define max length of text fields?
case FieldDefinition::TYPE_TEXT:
- return 'BLOB';
+ return 'BLOB'; // This is 64k max. And: MySQL
also has a TEXT type.
case FieldDefinition::TYPE_BOOLEAN:
return 'TINYINT';
default:
diff --git a/src/MySQL/MySQLIndexSqlBuilder.php
b/src/MySQL/MySQLIndexSqlBuilder.php
index d340693..578782c 100644
--- a/src/MySQL/MySQLIndexSqlBuilder.php
+++ b/src/MySQL/MySQLIndexSqlBuilder.php
@@ -29,6 +29,7 @@
* @param TableNameFormatter $tableNameFormatter
*/
public function __construct( Escaper $escaper, TableNameFormatter
$tableNameFormatter ) {
+ // The only TableNameFormatter implementation I could find is
based on the MediaWiki DB class. Is that OK?
$this->escaper = $escaper;
$this->tableNameFormatter = $tableNameFormatter;
}
diff --git a/src/MySQL/MySQLTableSqlBuilder.php
b/src/MySQL/MySQLTableSqlBuilder.php
index 0dd3fc9..0dc3543 100644
--- a/src/MySQL/MySQLTableSqlBuilder.php
+++ b/src/MySQL/MySQLTableSqlBuilder.php
@@ -64,6 +64,7 @@
}
protected function getCreateNameSql() {
+ // $this->dbName should perhaps be escaped as an identifier.
return 'CREATE TABLE `' . $this->dbName . '`.'
. $this->getPreparedTableName( $this->table->getName()
);
}
@@ -91,6 +92,12 @@
protected function getTableOptionSql() {
return 'ENGINE=InnoDB, DEFAULT CHARSET=binary';
}
+ // Should the engine really be hardcoded? InnoDB is performant
for concurrent modification,
+ // but MyISAM may be preferable for raw query speed and bulk
updates. Also, InnDB doesn't
+ // support full text indexes.
+ // The charset also should be configurable: using binary is
safe, but using utf8 gives
+ // better collation (sort order). Note however that when using
utf8, comparison
+ // of varchar/char/text is case insensitive per default.
protected function getQueryEnd() {
return ';';
diff --git a/src/SQLite/SQLiteFieldSqlBuilder.php
b/src/SQLite/SQLiteFieldSqlBuilder.php
index 36b560d..6aa1dce 100644
--- a/src/SQLite/SQLiteFieldSqlBuilder.php
+++ b/src/SQLite/SQLiteFieldSqlBuilder.php
@@ -77,9 +77,9 @@
case FieldDefinition::TYPE_INTEGER:
return 'INTEGER';
case FieldDefinition::TYPE_FLOAT:
- return 'FLOAT';
+ return 'FLOAT'; // SQLite uses REAL, not FLOAT
case FieldDefinition::TYPE_TEXT:
- return 'BLOB';
+ return 'BLOB'; // could/should be TEXT?
case FieldDefinition::TYPE_BOOLEAN:
return 'TINYINT';
default:
diff --git a/src/SQLite/SQLiteUnEscaper.php b/src/SQLite/SQLiteUnEscaper.php
index ba03257..c9812d1 100644
--- a/src/SQLite/SQLiteUnEscaper.php
+++ b/src/SQLite/SQLiteUnEscaper.php
@@ -16,7 +16,8 @@
* @param string $identifier
* @return string
*/
- public function getUnEscapedIdentifier( $identifier ){
+ public function getUnEscapedIdentifier( $identifier ) {
+ // perhaps assert that the string is actually quoted, to avoid
obscure breakage.
return str_replace( '""', '"', substr( $identifier, 1, -1 ) );
}
diff --git a/src/Schema/Definitions/FieldDefinition.php
b/src/Schema/Definitions/FieldDefinition.php
index 18c5bdb..dbe8ba9 100644
--- a/src/Schema/Definitions/FieldDefinition.php
+++ b/src/Schema/Definitions/FieldDefinition.php
@@ -56,9 +56,9 @@
private $autoIncrement;
const TYPE_BOOLEAN = 'bool';
- const TYPE_TEXT = 'str';
+ const TYPE_TEXT = 'str'; // need at least short sting vs text vs blob
const TYPE_INTEGER = 'int';
- const TYPE_FLOAT = 'float';
+ const TYPE_FLOAT = 'float'; // need decimal vs. float
const NOT_NULL = false;
const NULL = true;
@@ -172,15 +172,17 @@
if ( !is_string( $name ) ) {
throw new InvalidArgumentException( 'The field $name
needs to be a string' );
}
+ //TODO: fail on crazy names (containing e.g. spaces) even if
the DB supports that.
}
private function assertIsValidType( $type ) {
if ( !is_string( $type ) ) {
throw new InvalidArgumentException( 'The field $type
needs to be a string' );
}
+ //TODO: check against known types
}
- private function assertIsValudNull( $null ) {
+ private function assertIsValudNull( $null ) { //FIXME: "Valud"
if ( !is_bool( $null ) ) {
throw new InvalidArgumentException( 'The $null
parameter needs to be a boolean' );
}
--
To view, visit https://gerrit.wikimedia.org/r/90106
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I89db453aa4587f5ec83d3e86d7fdf7c8f774294e
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/extensions/WikibaseDatabase
Gerrit-Branch: master
Gerrit-Owner: Daniel Kinzler <[email protected]>
Gerrit-Reviewer: Addshore <[email protected]>
Gerrit-Reviewer: Jeroen De Dauw <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
