Skip to content

insertMulti implementation by using transactions #504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 22, 2016

Conversation

ricwein
Copy link
Contributor

@ricwein ricwein commented Jul 21, 2016

this is a first implementation for MysqliDB:: insertMulti() method to insert multiple datasets at once by utilizing mysql-transactions.

Please excuse the additional changed lines in the commits, where only spaces where deleted. My Editor does this by default.

This is only a proposal and therefor may be not the best or fastest solution.

The changes include:

  • added MysqliDB::insertMulti() method into MysqliDb.php
  • added tests for insertMulti implementation in tests/mysqliDbTests.php
  • added small documentation to readme.md

Please tell, if something should be changed or does not adapt the classes coding style.

@ricwein ricwein mentioned this pull request Jul 21, 2016
@avbdr
Copy link
Collaborator

avbdr commented Jul 21, 2016

Looks good, but can you please remove all spacing related changes please?

@avbdr
Copy link
Collaborator

avbdr commented Jul 21, 2016

as well, please make function smaller by remove double \n

@avbdr
Copy link
Collaborator

avbdr commented Jul 21, 2016

one more observation. insertMulti() will never return false. Seems after rollback you would need to return false. As well, Please check if getLastError and getLastErrId() are returning errors from an actual failed insert() and not from rollback()

@ricwein
Copy link
Contributor Author

ricwein commented Jul 21, 2016

Reverting the spacing changes might by a problem I look into tomorrow.

I just removed some empty lines and unnecessary comments to make the actual method smaller.

I'm not sure what you mean by "insertMulti() will never return false. Seems after rollback you would need to return false". The current behavior is to return an array in success, and false in error case, also after rollback.

Actual getLastError() isn't affected by a rollback(), so this should be no problem.

@avbdr
Copy link
Collaborator

avbdr commented Jul 21, 2016

you are correct, sorry

- MysqliDb::insertMulti() requires at least a table-name and
two-dimensional array containing the data-sets
- the third parameter $dataKeys can contain an array, setting|overriding
the table column names in the data-sets
- removes some unnecessary linebreaks and comments
- fixes typo
- adds insertMulti() to Readme
- adds MysqliDB::insertMulti() insertion tests
- removes whitespace changes
@ricwein
Copy link
Contributor Author

ricwein commented Jul 22, 2016

I've reverted all whitespace related changes with the last commit. Anything else to do?

wasn't that simple to remove that damned whitespace-changes without messing up the repo, I hope the master-branch is alright now

@avbdr
Copy link
Collaborator

avbdr commented Jul 22, 2016

awesome. As for whitespaces, you could just do git checkout and paste back needed code :)

@avbdr avbdr merged commit bd4fb1b into ThingEngineer:master Jul 22, 2016
@ricwein
Copy link
Contributor Author

ricwein commented Jul 22, 2016

Well, that would be to simple!

  • I soft-reseted to the last commit before my changes
  • created a diff with --ignore-all-space --ignore-blank-lines --ignore-space-change as a patch
  • actual checked out the old commit and applied the patch
  • reset the master HEAD to my new commit

Have a nice weekend, and thanks for merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants