Remove unnecessary code from Connection::insert #314

Merged
merged 1 commit into from May 14, 2013

Conversation

Projects
None yet
4 participants
@deeky666
Member

deeky666 commented May 13, 2013

This is a tiny optimization in the Connection::insert method that remove an unnecessary foreach loop an some unneeded variable assignments.
If this is not desired, just close this PR ;)

guilhermeblanco added a commit that referenced this pull request May 14, 2013

Merge pull request #314 from deeky666/microoptimize-connection-insert
Remove unnecessary code from Connection::insert

@guilhermeblanco guilhermeblanco merged commit 5dd0adf into doctrine:master May 14, 2013

1 check passed

default The Travis CI build passed
Details
@lastzero

This comment has been minimized.

Show comment Hide comment
@lastzero

lastzero Sep 17, 2013

This code breaks insert() calls with $data = array(), since array_fill() expects the second parameter to be positive (not 0). In MySQL, there can actually exist tables which have an automatic primary key and all other fields with default.

In case you do not want to accept empty $data for insert() anymore, you should 1) document that and 2) throw an exception which explains what's wrong (the warning triggered by array_fill() is not enough).

This code breaks insert() calls with $data = array(), since array_fill() expects the second parameter to be positive (not 0). In MySQL, there can actually exist tables which have an automatic primary key and all other fields with default.

In case you do not want to accept empty $data for insert() anymore, you should 1) document that and 2) throw an exception which explains what's wrong (the warning triggered by array_fill() is not enough).

This comment has been minimized.

Show comment Hide comment
@lastzero

lastzero Sep 17, 2013

Why is there no automatic test for insert()? :)

Why is there no automatic test for insert()? :)

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Sep 17, 2013

Member

@lastzero consider writing one and opening an issue or reporting the issue on http://www.doctrine-project.org/jira/browse/DBAL

Member

Ocramius replied Sep 17, 2013

@lastzero consider writing one and opening an issue or reporting the issue on http://www.doctrine-project.org/jira/browse/DBAL

This comment has been minimized.

Show comment Hide comment
@lastzero

lastzero Sep 17, 2013

Already cloned the repository... however, it won't be easy to write a test, if there is no existing approach for testing database methods like insert, update, delete, fetch,...? I usually do this with static file fixtures recorded from a database server during the first test run. Would this be ok?

The fix btw would just reverting this particular change, since the code obviously was not "unnecessary".

Already cloned the repository... however, it won't be easy to write a test, if there is no existing approach for testing database methods like insert, update, delete, fetch,...? I usually do this with static file fixtures recorded from a database server during the first test run. Would this be ok?

The fix btw would just reverting this particular change, since the code obviously was not "unnecessary".

This comment has been minimized.

Show comment Hide comment
@lastzero

lastzero Sep 17, 2013

Thank you :)

Thank you :)

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