Added INSERT support to dbal QueryBuilder #184

Closed
wants to merge 1 commit into
from

Projects

None yet

9 participants

@Tim-Erwin

See also http://www.doctrine-project.org/jira/browse/DBAL-320
Please review and commit as fits.

@travisbot

This pull request passes (merged 1f9406d into f9af44c).

@travisbot

This pull request passes (merged 1f9406d into f9af44c).

@stof stof commented on an outdated diff Aug 20, 2012
lib/Doctrine/DBAL/Query/QueryBuilder.php
case self::DELETE:
$sql = $this->getSQLForDelete();
break;
-
+
@stof
stof Aug 20, 2012 Member

please don't add trailing whitespaces in empty lines (they should be really empty)

@travisbot

This pull request passes (merged 866a7ca into f9af44c).

@tony
tony commented Aug 28, 2012

@Tim-Erwin alias key missing from $this->sqlParts['from']['alias']. I think we should fix the above alone accordingly. I was able to get this working successfully in a dev env.

I don't believe table aliases are not supported with INSERT on SQL databases.

T

@guilhermeblanco
Member

No CS issues from my side. =)

@Tim-Erwin

@tony What do you mean with "I think we should fix the above alone accordingly."? Aliases are not supported, right? I can't even think of a scenario where they whould be useful. Maybe an INSERT from a SELECT, but mixing that would be bad style anyways. So I'd just remove the alias that I missed in getSQLForInsert().

@tony
tony commented Aug 30, 2012

@Tim-Erwin: The code comments that I had posted contradicted, there was originally a key error as we were not passing alias. Since we're not using or passing it, we shouldn't check for a key that doesn't exist.

Does this clarify things?

@tony
tony commented Mar 11, 2013

@doctrine Any intent on having this merged?

@beberlei beberlei commented on the diff Mar 11, 2013
lib/Doctrine/DBAL/Query/QueryBuilder.php
@@ -996,6 +1028,20 @@ private function getSQLForUpdate()
}
/**
+ * Converts this instance into an INSERT string in SQL.
+ *
+ * @return string
+ */
+ private function getSQLForInsert()
+ {
+ $table = $this->sqlParts['from']['table'] . ($this->sqlParts['from']['alias'] ? ' ' . $this->sqlParts['from']['alias'] : '');
+ $query = 'INSERT INTO ' . $table
+ . ' SET ' . implode(", ", $this->sqlParts['set']);
@beberlei
beberlei Mar 11, 2013 Member

please change this to an INSERT INTO table (columns) VALUES (parts) statement

@Tim-Erwin
Tim-Erwin Jun 4, 2013

Well, I can do that, however, I'd either have to explode(" = ", $sqlParts) which is really not elegant or I'd have to change the way $sqlParts['set'] are stored which would also affect UPDATE or we would need to introduce a new sqlPartName and helper method.

I'd prefer the second option, it's the cleanest (would move the assembling code from set() to getSQL()). Ok?

@ghost
ghost commented Mar 31, 2013

@Tim-Erwin ping

@Tim-Erwin

I need some clarification before I can improve on the code (see above).

@ghost
ghost commented Jun 6, 2013

@beberlei - can you please give some direction for @Tim-Erwin

@Tim-Erwin

I cleaned up the commits a little which will hopefully not affect anybody. If this gets accepted soon it doesn't matter anyway.

@Tim-Erwin

@beberlei Well, if QueryBuilder::add('set', "field = value") should still be allowed (instead of QueryBuilder::add('set', array("field", "value")) ) we might have two different kinds in sqlParts['set'], which needs to be checked when assembling the SQL. It's not hard or whatever, just saying.

Why do you wanna have the INSERT INTO table (columns) VALUES (parts) syntax anyway? It's now the same as the UPDATE which seems to be fine.

@mvrhov
Contributor
mvrhov commented Jun 14, 2013

Regarding the syntax, because that's the syntax defined by the SQL standard.

@beberlei
Member

Merged @deeky666 PR #420

@beberlei beberlei closed this Dec 20, 2013
@ghost
ghost commented Dec 20, 2013

@Tim-Erwin can you make sure this gets documented properly in the doctrine docs?

@Tim-Erwin

@drak , I don't know if there is a misunderstanding. This issue here is not the one that has been merged, but #420. If you still think I'm the one to update the docs, I might not be the best choice as I don't even know where to start :)

@deeky666
Member

@drak @Tim-Erwin Added documentation with PR #454.

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