Skip to content
This repository

CUBRID Database 9.0 Support #1893

Open
wants to merge 3 commits into from

7 participants

Esen Sagynov Alexander Makarov tom-- acutexyz resurtm Carsten Brandt Maurizio Domba Cerin
Esen Sagynov

CUBRID Database 9.0 patch for #1892.

It's been over two years since users have been requesting CUBRID Database support.

I would like to contribute the working code with:

  • CUBRID 9.0+ support through PDO_CUBRID-9.0.0.0001+
  • CUBRID DB Schemas
  • *cubrid.sql data files for demo projects and tests
  • Two test files
  • Updates to YiiBase.php and yiilite.php.

I have tested with blog demo app as well as ran those 2 test files which are functionally identical to those of MySQL. Tests all pass.

Currently the code is hosted at my repo https://github.com/kadishmal/yii/tree/cubrid-database-support. The actual code commit is c0b90c73b.

Let me know if you have any comments.

framework/YiiBase.php
@@ -699,7 +699,11 @@ public static function registerAutoloader($callback, $append=false)
699 699
 		'CDbExpression' => '/db/schema/CDbExpression.php',
700 700
 		'CDbSchema' => '/db/schema/CDbSchema.php',
701 701
 		'CDbTableSchema' => '/db/schema/CDbTableSchema.php',
702  
-		'CMssqlColumnSchema' => '/db/schema/mssql/CMssqlColumnSchema.php',
  702
+        'CCubridColumnSchema' => '/db/schema/cubrid/CCubridColumnSchema.php',
  703
+        'CCubridCommandBuilder' => '/db/schema/cubrid/CCubridCommandBuilder.php',
  704
+        'CCubridSchema' => '/db/schema/cubrid/CCubridSchema.php',
  705
+        'CCubridTableSchema' => '/db/schema/cubrid/CCubridTableSchema.php',
  706
+        'CMssqlColumnSchema' => '/db/schema/mssql/CMssqlColumnSchema.php',
2
Carsten Brandt Collaborator
cebe added a note December 31, 2012

Make sure to use tabs instead of spaces in all the code.

Sure, I will fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Alexander Makarov
Collaborator

@kadishmal are you using it daily and going to take care of it?

Esen Sagynov

Sure, I will take care of any CUBRID related issue.

Alexander Makarov
Collaborator

@qiangxue what do you think?

tom--

i am using it daily and will do whatever i can.

and my experience is that @kadishmal really does take care of issues remarkably well.

acutexyz

What's important - CUBRID is a very alive product, and in short period they did a lot of work. I'm sure it will be very good to have Yii and CUBRID together, since both of the sides would benefit.

tom--

@acutexyz is right. while CUBRID already has really impressive stuff to offer, it is moving forwards rapidly and the team pays close attention to user's requests.

tom--

I am not sure where to put this feature request other than here.

Use of CUBRID collection data types is inconvenient. For example, in the current PR, if a column has type SET OF VARCHAR(32) it is used in AR as follows:

$model->attr = new CDbExpression("{'man', 'fred', 'man'}");
$model->save();
$model->refresh();
var_dump($model->attr);
>> string(11)"{fred, man}"

Applications will usually need to convert from array to the specially formatted expression and and from the returned string to array. I think it is the framework's job to do the conversions.

CCubridColumnSchema::extractType() and CCubridColumnSchema::typecast() appear to be the place to do it on save. I'm not so sure about the fetch.

Esen Sagynov

The support for CUBRID Collection Data Types can be added. I haven't implemented it in this version as I was not very sure about whether Yii supports this kind of conversion of out of the box. If not, we should add this routine directly to CCubridColumnSchema perhaps. I should look into this more to have a clear design.

Esen Sagynov

Another nice feature of CUBRID we can also think of supporting is the Click Counter. I may release this update along with the Collection Types support.

tom--

Iiuc, the click counter is easy to use with a CDbExpression, analogous to new CDbExpression('NOW()'), and I don't mind doing it that way.

But collections are harder. I implemented helper methods to convert from PHP array to SQL collection and from string to PHP array. It is not so simple because it must be aware of the column type as well as the data type of the items in the collection.

Btw, I find the documentation of CUBRID collection data types a little unclear regarding what types the items are allowed to be. I assume INT, CHAR and VARCHAR are allowed. Any others?

Esen Sagynov

You're right, this detail is missing. In CUBRID, any data type except LOB data types (BLOB/CLOB) is allowed to be an element of collection data types.

I've reported this issue to the team. We'll improve the related manual pages.

Just in case you want to know more about Data Types and Domains in CUBRID, refer to this article. It was written by one of the CUBRID Architects.

If you have other requests/issues, please report directly to http://jira.cubrid.org/. I'll assign the right person to your issues. This way you can keep track of when the issues are resolved.

tom--
tom-- commented March 25, 2013

How can I help to advance the decision about CUBRID support in Yii Framework?

Alexander Makarov
Collaborator

What do you think about distributing is as an extension? You'll have a full control of the code i.e. will be able to update it w/o waiting for us to check it and we'll add a link to it into the definitive guide to Yii.

resurtm
Collaborator

Same decision was made in #2179. Documentation could be changed and would refer to a new third party extension.

Esen Sagynov

Let's support ENUM, meaning only CUBRID 9.1. I would personally choose 9.1 over 8.4.3 for new projects. So, let's go with it.

Regarding the extension, I would personally prefer Yii with native CUBRID database support. This would greatly improve user experience which I always care about in any project.

Alexander Makarov
Collaborator

@kadishmal well, noone from Yii core team uses CUBRID or has any experience with it so we just can't maintain it efficiently. That's why I'm suggesting a recommended extension instead.

Esen Sagynov

I'm here for that. Moreover, there are more and more users who, I'm sure, would be glad to provide fixes and improvements to CUBRID dialect over time. I strongly believe it will be a representative asset for Yii framework.

Carsten Brandt
Collaborator
cebe commented March 26, 2013

@samdark I am looking forward to use CUBRID in my projects. Just had no time to start with it yet.

Alexander Makarov
Collaborator

@cebe so you're going to handle it? What about Informix #2179 ?

Carsten Brandt
Collaborator
cebe commented March 26, 2013

@samdark yes, I'm assigned to this ticket ;)
Have worked with informix in one project, can take a look at it, but CUBRID will have higher priority.

tom--
tom-- commented March 26, 2013

@samdark you said "noone from Yii core team uses CUBRID or has any experience with it". You really ought to make it a priority to fix that :)

But seriously: I am not a freelancer or any kind of professional web developer. My (webapp) software runs my business which has been serving thousands of users for more than 10 years with continuous expansion of the aggregate data in MySQL.

I am currently rewriting this app (paying off the technology debt on the PHP code more than anything else) and I have to consider what will change in the next 10 years. I think this perspective is different from a lot of Yii programmers'.

MySQL does not have a future that I can take a chance on. Even if Oracle were out of the picture, I don't see how anyone can sort out the awful mess it has become. I cannot risk using it. Drizzle never gained traction. Maria doesn't sort out its technical problems, even if it sorts out the Oracle one. So I am forced to look at something else altogether.

In my positon, choosing an open source RDBMS with no MySQL BC requirement or other such constraint, there's only once choice. Fortunately it is also a very good product.

People have been reconsidering the A in LAMP. The M has to go too. Sooner or later.

Maurizio Domba Cerin
Collaborator

A bit OT but you have PostgreSql, too :D

tom--
tom-- commented March 26, 2013

@mdomba I know it has a following, but when I said "or other such constraint" I was thinking of skills, experience, preference, deployed infrastructure, etc. Absent all that, looking at PostgreSql's features, current status and history, it's not really any competition to CUBRID.

Moreover, since my last 10 years has taught me to minimize server-specific SQL as a priority, all of PostgreSql's fancy stuff is off limits to me.

Carsten Brandt cebe commented on the diff March 26, 2013
framework/web/auth/schema-cubrid.sql
((5 lines not shown))
  5
+ * @link http://www.yiiframework.com/
  6
+ * @copyright Copyright © 2008 Yii Software LLC
  7
+ * @license http://www.yiiframework.com/license/
  8
+ * @since 1.1.12
  9
+ */
  10
+
  11
+drop table if exists `AuthAssignment`;
  12
+drop table if exists `AuthItemChild`;
  13
+drop table if exists `AuthItem`;
  14
+
  15
+create table `AuthItem`
  16
+(
  17
+   `name`                 varchar(64) not null,
  18
+   `type`                 integer not null,
  19
+   `description`          varchar(65535),
  20
+   `bizrule`              varchar(65535),
5
Carsten Brandt Collaborator
cebe added a note March 26, 2013

Why are you using varchar instead of string here?

Esen Sagynov
kadishmal added a note March 26, 2013

If bizrule, description can be longer than 65535 characters, then we need to change it to STRING. Let me know.

Carsten Brandt Collaborator
cebe added a note March 27, 2013

for mysql schema it is text so I think we should use STRING here to have similar behavior

tom--
tom-- added a note March 27, 2013

@cebe In general I would prefer to see the most portable SQL generated in Yii. Correct me if I am wrong but I think STRING is a CUBRID special.

CUBRID's VARCHAR ought to suffice here, either without a (n) suffix or with a large one:

Variable-length character strings are represented as VARCHAR (n), where n represents the number of characters. If n is not specified, the value is specified as 1,073,741,823, the maximum length. http://www.cubrid.org/manual/91/en/sql/datatype.html#varchar-n-char-varying-n

Any argument along the lines of "we do it like this in MySQL, so..." is going to make me nervous.

Esen Sagynov
kadishmal added a note March 27, 2013

Good suggestion. Perhaps VARCHAR is a more portable solution. Briefly VARCHAR wihout size argument is the same as STRING.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Carsten Brandt cebe commented on the diff March 26, 2013
framework/web/auth/schema-cubrid.sql
((16 lines not shown))
  16
+(
  17
+   `name`                 varchar(64) not null,
  18
+   `type`                 integer not null,
  19
+   `description`          varchar(65535),
  20
+   `bizrule`              varchar(65535),
  21
+   `data`                 string,
  22
+   primary key (`name`)
  23
+);
  24
+
  25
+create table `AuthItemChild`
  26
+(
  27
+   `parent`               varchar(64) not null,
  28
+   `child`                varchar(64) not null,
  29
+   primary key (`parent`,`child`),
  30
+   foreign key (`parent`) references `AuthItem` (`name`) on delete cascade on update restrict,
  31
+   foreign key (`child`) references `AuthItem` (`name`) on delete cascade on update restrict
3
Carsten Brandt Collaborator
cebe added a note March 26, 2013

mysql schema has on update cascade here which makes sense imo. any reason you are using on update restrict here and in AuthAssignment table below?

Esen Sagynov
kadishmal added a note March 26, 2013

CUBRID doesn't support ON UPDATE CASCADE.

Carsten Brandt Collaborator
cebe added a note March 27, 2013

okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Carsten Brandt cebe commented on the diff March 26, 2013
framework/db/schema/cubrid/CCubridColumnSchema.php
((45 lines not shown))
  45
+	{
  46
+		if($this->dbType==='TIMESTAMP' && $defaultValue==='CURRENT_TIMESTAMP')
  47
+			$this->defaultValue=null;
  48
+		else
  49
+			parent::extractDefault($defaultValue);
  50
+	}
  51
+	
  52
+	/**
  53
+	 * Extracts size, precision and scale information from column's DB type.
  54
+	 * @param string $dbType the column's DB type
  55
+	 */
  56
+	protected function extractLimit($dbType)
  57
+	{
  58
+		parent::extractLimit($dbType);
  59
+		// CUBRID does not set limits to numeric data types.
  60
+		// So, we need to set the defalt limit size for INTEGER equal to 11.
4
Carsten Brandt Collaborator
cebe added a note March 26, 2013

Why do we need to set this to 11?

Esen Sagynov
kadishmal added a note March 26, 2013

In Yii tests the default behavior was to compare the precision of INT columns with 11, so I set the default precision to 11.

Another option is to remove this line, and set null in tests where they compare INT's precision.

Carsten Brandt Collaborator
cebe added a note March 27, 2013

Imo, when there is no precision in CUBRID we should not virtually add one. Better set it to null and adjust tests to it.

Esen Sagynov
kadishmal added a note March 27, 2013

Agree. Let's change the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Carsten Brandt cebe commented on the diff March 26, 2013
framework/db/schema/cubrid/CCubridSchema.php
((10 lines not shown))
  10
+
  11
+/**
  12
+ * CCubridSchema is the class for retrieving metadata information from a CUBRID database (version 8.4.0 and later).
  13
+ *
  14
+ * @author Esen Sagynov <kadismal@gmail.com>
  15
+ * @version $Id: CCubridSchema.php
  16
+ * @package system.db.schema.cubrid
  17
+ * @since 1.1.12
  18
+ */
  19
+class CCubridSchema extends CDbSchema
  20
+{
  21
+	public $columnTypes=array(
  22
+		'pk' => 'INTEGER NOT NULL AUTO_INCREMENT PRIMARY KEY',
  23
+		// same as STRING or CHARACTER VARYING
  24
+		'string' => 'VARCHAR(255)',
  25
+		'text' => 'VARCHAR(65535)',
7
Carsten Brandt Collaborator
cebe added a note March 26, 2013

according to http://www.cubrid.org/manual/843/en/STRING STRING is not the same as VARCHAR(65535) so I'd prefer string here...

Esen Sagynov
kadishmal added a note March 26, 2013

Sure. STRING in CUBRID is the longest of string types. It may better replace the TEXT data type.

tom--
tom-- added a note March 27, 2013

as previous comment: isn't VARCHAR(large number) more portable than STRING ?

Carsten Brandt Collaborator
cebe added a note March 27, 2013

Why do you think it should be portable somehow? This file is CCubridSchema so it only works with cubrid. The PHP type is still simple string, we are just choosing the type that fits best for storage in the db...
Can you please explain what you mean with "portable"?

tom--
tom-- added a note March 27, 2013

Schema manipulation queries generate SQL that might becomes, more or less, part of a Yii user's app. If one day a developer has to port an app from one RDBMS to another, that developer might be sad or happy depending whether Yii used a lot of server-specific SQL or used instead "more standard SQL".

So, if using type spec clause X versus Y makes no functional difference from our point of views (considering Yii Framework design), the choice less likely to make the future porting developer sad is the better one.

I wish I could be more precise and say something like: use standard SQL as far as possible. But that begs the question, what standard? And who cares about standards like ANSI SQL when important possible target RDBMSs reject various ANSI SQL clauses? That's why I said "more portable". I am not an SQL expert so this is the best I can manage for now.

I could put this another way and say: I have learned from being sad and I won't use an ENUM again. It's great that CUBRID added it to help with porting MySQL schemas but for new schemas or schema parts, there's no need for it. I'm not going to use CUBRID's collection types for the same reason, not because I don't like them and what they can do for my app.

Carsten Brandt Collaborator
cebe added a note March 27, 2013

Okay, need to think about this for a while...
Any input on this, @yiisoft/core-developers ?

Esen Sagynov
kadishmal added a note March 27, 2013

I would go for VARCHAR in this very case just for future portability. Anyway this doesn't affect any functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Carsten Brandt cebe commented on the diff March 26, 2013
framework/db/schema/cubrid/CCubridSchema.php
((13 lines not shown))
  13
+ *
  14
+ * @author Esen Sagynov <kadismal@gmail.com>
  15
+ * @version $Id: CCubridSchema.php
  16
+ * @package system.db.schema.cubrid
  17
+ * @since 1.1.12
  18
+ */
  19
+class CCubridSchema extends CDbSchema
  20
+{
  21
+	public $columnTypes=array(
  22
+		'pk' => 'INTEGER NOT NULL AUTO_INCREMENT PRIMARY KEY',
  23
+		// same as STRING or CHARACTER VARYING
  24
+		'string' => 'VARCHAR(255)',
  25
+		'text' => 'VARCHAR(65535)',
  26
+		'integer' => 'INTEGER',
  27
+		'float' => 'NUMERIC',
  28
+		'real' => 'NUMERIC',
7
Carsten Brandt Collaborator
cebe added a note March 26, 2013

There are FLOAT and REAL, why not use them here? http://www.cubrid.org/manual/843/en/FLOAT|REAL

Esen Sagynov
kadishmal added a note March 26, 2013

MySQL and CUBRID have different number of arguments in FLOAT and REAL. I didn't want to cause the failure. CUBRID's NUMERIC accepts two arguments like MySQL's FLOAT.

If you're sure that having single argument or two doesn't matter in Yii's core, then I'm fine changing them to FLOAT and REAL respectively. What do you say?

Carsten Brandt Collaborator
cebe added a note March 27, 2013

We should not use MySQL as a reference. When there is native support for a data type, we should use it.

tom--
tom-- added a note March 27, 2013

I am a very repetitive person:

Any argument along the lines of "we do it like this in MySQL, so..." is going to make me nervous.

What is the more portable SQL?

Carsten Brandt Collaborator
cebe added a note March 27, 2013

I just wrote "We should not use MySQL as a reference." What's your point?

tom--
tom-- added a note March 27, 2013

My 1st sentence supports your 1st sentence.

I could be wrong but your 2nd sentence could be read as advocating a CUBRID-specific declaration if it more closely approximate's PHP (in this case, float -> FLOAT, real -> REAL). Idk if those are supported on other mainstream RDBMSs. Hence my question: what is more portable? I explained above what that might mean and why I think it's worth considering.

Esen Sagynov
kadishmal added a note March 27, 2013

If it doesn't hurt, let's leave it as it is, i.e. NUMERIC, unless you have strong arguments for using native data types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Carsten Brandt cebe commented on the diff March 26, 2013
framework/db/schema/cubrid/CCubridSchema.php
((22 lines not shown))
  22
+		'pk' => 'INTEGER NOT NULL AUTO_INCREMENT PRIMARY KEY',
  23
+		// same as STRING or CHARACTER VARYING
  24
+		'string' => 'VARCHAR(255)',
  25
+		'text' => 'VARCHAR(65535)',
  26
+		'integer' => 'INTEGER',
  27
+		'float' => 'NUMERIC',
  28
+		'real' => 'NUMERIC',
  29
+		'decimal' => 'NUMERIC',
  30
+		'datetime' => 'DATETIME',
  31
+		'timestamp' => 'TIMESTAMP',
  32
+		'time' => 'TIME',
  33
+		'date' => 'DATE',
  34
+		'binary' => 'BIT VARYING',
  35
+		'bool' => 'SHORT',
  36
+		'boolean' => 'SHORT',
  37
+		'money' => 'NUMERIC(19,4)',
5
Esen Sagynov
kadishmal added a note March 26, 2013

I'm really happy you've pointed these issues. Now we can discuss. The reason why NUMERIC(19,4) (or DECIMAL(19,4)) is because MySQL has DECIMAL(19,4), and they are compatible. I didn't want to break anything.

CUBRID's MONETARY may be suitable but it has slightly different syntax. It is the same as NUMERIC(19,2), i.e. scale=4 vs. scale=2. I didn't know if somewhere this was badly important. So didn't want to risk.

Let me know if we can change this.

Carsten Brandt Collaborator
cebe added a note March 27, 2013

Same as above

We should not use MySQL as a reference. When there is native support for a data type, we should use it.

tom--
tom-- added a note March 27, 2013

@cebe: The first sentence, yes. The second, no. If CUBRID has more than one way to support the feature that are functionally equivalent in PHP, I say choose not the native data type but the more portable SQL.

Esen Sagynov
kadishmal added a note March 27, 2013

Agree with @tom--. Let's ensure future portability wherever possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Carsten Brandt
Collaborator
cebe commented March 26, 2013

Installed CUBRID and reviewed PR partially. Btw: no need to change anything right now. I have my local branch where I already adjusted some typos and version annotation. Please discuss here, I will do the needed changes then.

Carsten Brandt
Collaborator
cebe commented March 27, 2013

Thanks for your comments, will check it and do the necessary changes soon.

Alexander Makarov
Collaborator

After discussing with @qiangxue decided to include it into core framework. So now it's only about ensuring that code is OK.

Alexander Makarov samdark commented on the diff March 27, 2013
framework/db/schema/cubrid/CCubridColumnSchema.php
... ...
@@ -0,0 +1,66 @@
  1
+<?php
  2
+/**
  3
+ * CCubridColumnSchema class file.
  4
+ *
  5
+ * @author Esen Sagynov <kadismal@gmail.com>
  6
+ * @link http://www.yiiframework.com/
  7
+ * @copyright Copyright &copy; 2008-2011 Yii Software LLC
  8
+ * @license http://www.yiiframework.com/license/
  9
+ */
  10
+
  11
+/**
  12
+ * CCubridColumnSchema class describes the column meta data of a CUBRID table.
  13
+  *
  14
+ * @author Esen Sagynov <kadismal@gmail.com>
  15
+ * @version $Id: CCubridColumnSchema.php
3
Alexander Makarov Collaborator
samdark added a note March 27, 2013

Not needed.

Esen Sagynov
kadishmal added a note March 27, 2013

Which part is not needed?

Carsten Brandt Collaborator
cebe added a note March 28, 2013

@version $Id: CCubridColumnSchema.php was needed on svn. Now on git we don't need it. already fixed that locally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Alexander Makarov samdark commented on the diff March 27, 2013
framework/db/schema/cubrid/CCubridColumnSchema.php
((2 lines not shown))
  2
+/**
  3
+ * CCubridColumnSchema class file.
  4
+ *
  5
+ * @author Esen Sagynov <kadismal@gmail.com>
  6
+ * @link http://www.yiiframework.com/
  7
+ * @copyright Copyright &copy; 2008-2011 Yii Software LLC
  8
+ * @license http://www.yiiframework.com/license/
  9
+ */
  10
+
  11
+/**
  12
+ * CCubridColumnSchema class describes the column meta data of a CUBRID table.
  13
+  *
  14
+ * @author Esen Sagynov <kadismal@gmail.com>
  15
+ * @version $Id: CCubridColumnSchema.php
  16
+ * @package system.db.schema.cubrid
  17
+ * @since 1.1.8
2
Alexander Makarov Collaborator
samdark added a note March 27, 2013

Please update version here and in similar phpdocs.

Carsten Brandt Collaborator
cebe added a note March 27, 2013

I already fixed doc and whitespace locally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Alexander Makarov samdark commented on the diff March 27, 2013
framework/db/schema/cubrid/CCubridColumnSchema.php
((16 lines not shown))
  16
+ * @package system.db.schema.cubrid
  17
+ * @since 1.1.8
  18
+ */
  19
+class CCubridColumnSchema extends CDbColumnSchema
  20
+{
  21
+	/**
  22
+	 * Extracts the PHP type from DB type.
  23
+	 * @param string $dbType DB type
  24
+	 */
  25
+	protected function extractType($dbType)
  26
+	{
  27
+		if(preg_match('/(FLO|REA|DOU|NUM|DEC)/',$dbType))
  28
+			$this->type='double';
  29
+		// The following "bool" and 'boolean" are for future compatibility.
  30
+		// As of CUBRID 9.0, they are not supported.
  31
+		else if(strpos($dbType,'BOOL')!==false)
2
Alexander Makarov Collaborator
samdark added a note March 27, 2013

according to code style should be elseif

Carsten Brandt Collaborator
cebe added a note March 27, 2013

fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Alexander Makarov samdark commented on the diff March 27, 2013
framework/db/schema/cubrid/CCubridCommandBuilder.php
((17 lines not shown))
  17
+ * @since 1.1.12
  18
+ */
  19
+class CCubridCommandBuilder extends CDbCommandBuilder
  20
+{
  21
+	// CUBRID operator special characters
  22
+	private $operatorSpecialChars = array('+', '-', '*', '/', '%', '|', '!', '<', '>', '=', '^', '&' , '~');
  23
+	/**
  24
+	 * Creates a SELECT command for a single table.
  25
+	 * @param CDbTableSchema $table the table metadata
  26
+	 * @param CDbCriteria $criteria the query criteria
  27
+	 * @param string $alias the alias name of the primary table. Defaults to 't'.
  28
+	 * @return CDbCommand query command.
  29
+	 */
  30
+	public function createFindCommand($table,$criteria,$alias='t')
  31
+	{
  32
+		$columns = $table->getColumnNames();
1
Alexander Makarov Collaborator
samdark added a note March 27, 2013

don't need spaces here around =

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Alexander Makarov samdark commented on the diff March 27, 2013
tests/framework/db/schema/CCubridTest.php
((1 lines not shown))
  1
+<?php
  2
+
  3
+Yii::import('system.db.CDbConnection');
  4
+Yii::import('system.db.schema.cubrid.CCubridSchema');
  5
+
  6
+
  7
+class CCubridTest extends CTestCase
  8
+{
  9
+	private $db;
  10
+
  11
+	public function setUp()
  12
+	{
  13
+		if(!extension_loaded('pdo') || !extension_loaded('pdo_cubrid'))
  14
+			$this->markTestSkipped('PDO and CUBRID extensions are required.');
  15
+
  16
+		$this->db=new CDbConnection('cubrid:host=192.168.5.99;port=33000;dbname=yii','test','test');
2
Alexander Makarov Collaborator
samdark added a note March 27, 2013

IP here is very specific. Is it common for CUBRID setups?

Carsten Brandt Collaborator
cebe added a note March 27, 2013

nope, should be localhost. already fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Carsten Brandt cebe commented on the diff March 27, 2013
framework/db/schema/cubrid/CCubridCommandBuilder.php
((107 lines not shown))
  107
+	 * @return CDbCommand the created command
  108
+	 * @throws CException if no counter is specified
  109
+	 */
  110
+	public function createUpdateCounterCommand($table,$counters,$criteria)
  111
+	{
  112
+		$columns = $table->getColumnNames();
  113
+
  114
+		$criteria->condition = $this->quoteColumnNames($columns, $criteria->condition);
  115
+		$criteria->order = $this->quoteColumnNames($columns, $criteria->order);
  116
+		$criteria->group = $this->quoteColumnNames($columns, $criteria->group);
  117
+		$criteria->having = $this->quoteColumnNames($columns, $criteria->having);
  118
+
  119
+		return parent::createUpdateCounterCommand($table,$counters,$criteria);
  120
+	}
  121
+
  122
+	private function quoteColumnNames($columns, $expression)
4
Carsten Brandt Collaborator
cebe added a note March 27, 2013

What do we need this quoting for? As far as I see you can write SELECT hi, test, col2 FROM testtable; without any problems.
When user wants to use a reserved word he can add quotes himself. Quoting isn't done in all the other dbms as well...

Esen Sagynov
kadishmal added a note March 27, 2013

You statement implies a user has to know about all the reserved words in CUBRID. This is not practical.

As a developer I would prefer the framework to do that job for me which is what that quoting does.

Think otherwise?

Carsten Brandt Collaborator
cebe added a note March 28, 2013

I think we can expect from a developer who sets up a CUBRID database that he knows about the reserved words in CUBRID. This is the case for all other DBMS already.
Escaping all the fields in all querys adds an overhead that is not needed imo.
Other opinions on this, @yiisoft/core-developers ?

tom--
tom-- added a note March 28, 2013

@cebe made three arguments:

  1. Users are competent and don't need this help from the framework.
  2. Consistency with what other CDbCommandBuilders do reduces the chance of surprising a user accustomed to that behavior (and hence the rate of bug reports pointing out the inconsistency)
  3. Performance.

On 1. I can say with authority that I, for one, am incompetent. I have never kept a list of a DB's reserved words to hand when I compose queries or SQL. I have often stumbled into trouble with reserved words. These days I escape everything, like SHOW CREATE TABLE, does because a) it easy and b) eliminates this whole class of defect.

On 2. I don't feel competent to judge. Consistency and convention have their merits. Otoh, I'm not keen on this particular convention.

On 3. Again, idk, since idk the typical use cases for CDbCommandBuilder and thus how to weight performance against reduced defect rate.

I could be way off base here but I had been under the impression that CDbCommandBuilder was intended for less senior users. I don't use it because I prefer CActiveRecord and am an experienced SQL author (except that I can't remember reserved words;). If my impression is right then CDbCommandBuilder should maybe put being helpful first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tom--
tom-- commented April 25, 2013

CDbConnection::initConnection() uses SET NAMES only for mysql and pgsql at present. Might it be worth extending this to CUBRID?

Esen Sagynov

SET NAMES can be a good idea as it is supported in CUBRID 9.0+.

Esen Sagynov

@cebe, are you working on this PR? Is there anything I can help with?

Carsten Brandt
Collaborator
cebe commented June 26, 2013

Yes, it is on my list, but I had no time to actually work on it yet.

The biggest question here is whether to do the parsing and quoting of all the values since it is inconsistent to other dbms. In general good to have but may have some problems. Need to test if it works with CDbExpression.
I'd like to have opinions on this from other @yiisoft/core-developers .

The rest is fine I think. Will need another review because it is already 3 month since my first one but should be okay.

Carsten Brandt
Collaborator
cebe commented June 26, 2013

After discussion with @qiangxue we decided to not introduce the auto quoting thing for cubrid only.

  1. for inconsitency to other dbms
  2. because it is really complex thing and we are not sure if it is possible at all to implement such a method correctly for all cases.

Will remove it from the code and clean up the PR soon.

tom--
tom-- commented June 26, 2013

I am pleased we can progress this. I would rather have CUBRID supported in Yii than get stuck on the question.

Esen Sagynov
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.