rework of pull request #354 on issue #124 #525

Merged
merged 6 commits into from Aug 28, 2012

Conversation

Projects
None yet
3 participants
@cebe
Member

cebe commented Mar 19, 2012

cleanup of @DaSourcerer's pull request #354 on issue #124

the new CMysqlCommandBuilder overides the complete method but the only difference to the original implementation is

line 61-63 and 100-103 each:

-    $sql="UPDATE {$table->rawName} SET ".implode(', ',$fields);
+    $sql="UPDATE {$table->rawName}";
     $sql=$this->applyJoin($sql,$criteria->join);
+    $sql.=" SET ".implode(', ',$fields);

and lines 68 + 106:

-    $command=$this->_connection->createCommand($sql);
+    $command=$this->getDbConnection()->createCommand($sql);

Tested this with my application and a unit test is also attached so I think it is ready for merge now.

DaSourcerer and others added some commits Feb 17, 2012

Add CMysqlCommandBuilder to handle joins on update
Fixes issue #124
CDbCommandBuilder produced faulty mysql update queries
when joins are involved. JOIN has to come before SET in mysql.
added some unit tests mysql
testing mysql update commands with new CMysqlCommandBuilder
issue #124

@cebe cebe referenced this pull request Mar 19, 2012

Closed

Issue #124 #354

@qiangxue

This comment has been minimized.

Show comment
Hide comment
@qiangxue

qiangxue Mar 21, 2012

Member

That's quite some duplicated code. Could you please check if other DBMS supports similar syntax? If so, we should modify CDbCommandBuilder.php directly. Even if it is not, we should still try to refactor the code to avoid copy large chunk of code.

Member

qiangxue commented Mar 21, 2012

That's quite some duplicated code. Could you please check if other DBMS supports similar syntax? If so, we should modify CDbCommandBuilder.php directly. Even if it is not, we should still try to refactor the code to avoid copy large chunk of code.

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Mar 21, 2012

Member

Okay, will check it.

Member

cebe commented Mar 21, 2012

Okay, will check it.

@DaSourcerer

This comment has been minimized.

Show comment
Hide comment
@DaSourcerer

DaSourcerer Mar 21, 2012

Contributor

A little support: The command builder seems to be fine with pgsql. Oracle will need a subquery for the same functionailty. And this seems to be outright impossible all on SQLite.

Contributor

DaSourcerer commented Mar 21, 2012

A little support: The command builder seems to be fine with pgsql. Oracle will need a subquery for the same functionailty. And this seems to be outright impossible all on SQLite.

@DaSourcerer

This comment has been minimized.

Show comment
Hide comment
@DaSourcerer

DaSourcerer Mar 21, 2012

Contributor

w/o link: mssql seems to be content with what the unaltered command builder provides.

Contributor

DaSourcerer commented Mar 21, 2012

w/o link: mssql seems to be content with what the unaltered command builder provides.

@DaSourcerer

This comment has been minimized.

Show comment
Hide comment
@DaSourcerer

DaSourcerer Apr 27, 2012

Contributor

bump
Any news on this?

Contributor

DaSourcerer commented Apr 27, 2012

bump
Any news on this?

@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Apr 27, 2012

Member

nope, sorry have not much time at the moment.

Member

cebe commented Apr 27, 2012

nope, sorry have not much time at the moment.

@DaSourcerer

This comment has been minimized.

Show comment
Hide comment
@DaSourcerer

DaSourcerer Apr 27, 2012

Contributor

That's okay. But this has been left withoug a notice on Google Code for almost half a year before, so I just want to make sure it's not forgotten.

Contributor

DaSourcerer commented Apr 27, 2012

That's okay. But this has been left withoug a notice on Google Code for almost half a year before, so I just want to make sure it's not forgotten.

@ghost ghost assigned cebe Aug 2, 2012

cebe added some commits Aug 28, 2012

Merge branch 'master' of https://github.com/yiisoft/yii into issue-124
* 'master' of https://github.com/yiisoft/yii: (651 commits)
  Updated guide Gii Model Generator page screenshot.
  adjusted CHANGELOG
  Requirements checking slightly improved.
  Enhanced CHANGELOG description
  Requirements checker: added support of the Oracle database (pdo_oci extension)
  MSSQL driver types refinements.
  Requirements checker: added support of the MSSQL (pdo_dblib and pdo_sqlsrv extensions).
  Language fixes.
  Preparations for merging fixes of the #556 into 1.1.13 codebase.
  [docs][blog] 'yiic' to 'Gii'
  ajaxUpdate is never false in jquery.yiilistview.js
  Added /docs/guide/fr/caching.overview.twt
  Added /docs/guide/fr/basics.controller.txt
  Added /docs/guide/fr/basics.mvc.txt
  Update framework/web/filters/CHttpCacheFilter.php
  Update docs/guide/fr/toc.txt
  [NL] Dutch messages translation updated
  added history.js license
  prepare for next release.
  prepare for 1.1.12 release.
  ...

Conflicts:
	CHANGELOG
better fix for issues #124
changed position of JOIN in UPDATE by overwriting applyJoin()
@cebe

This comment has been minimized.

Show comment
Hide comment
@cebe

cebe Aug 28, 2012

Member

Found a better solution by overwriting applyJoin() and insert JOIN part before SET.

Member

cebe commented Aug 28, 2012

Found a better solution by overwriting applyJoin() and insert JOIN part before SET.

@DaSourcerer

This comment has been minimized.

Show comment
Hide comment
@DaSourcerer

DaSourcerer Aug 28, 2012

Contributor

Nice. No idea why this didn't occur to me in the first place.

Contributor

DaSourcerer commented Aug 28, 2012

Nice. No idea why this didn't occur to me in the first place.

cebe added a commit that referenced this pull request Aug 28, 2012

Merge pull request #525, branch 'issue-124'
* issue-124:
  better fix for issues #124
  added @since annotation
  added docblocks to CMysqlCommandBuilder+CHANGELOG
  added some unit tests mysql
  Add CMysqlCommandBuilder to handle joins on update

@cebe cebe merged commit 266df50 into yiisoft:master Aug 28, 2012

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