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

[Db] Add update in database #4385

Merged
merged 9 commits into from Jul 9, 2017

Conversation

Projects
None yet
5 participants
@eXorus
Contributor

eXorus commented Jul 4, 2017

Hello,

For my acceptance I need sometimes to update record in the database, so I added a new function to be able to do that in the module Db:

$I->updateInDatabase('users', ['banned' => 'true'], ['id' => '123']);

What do you think ? I know it's better to start test with a clean database but I have a complex database that is coming from the production (anonymized) and I test it like a blackblox from the web. I can't populate all my database with only testing data.

eXorus added some commits Jul 4, 2017

Show outdated Hide outdated src/Codeception/Module/Db.php
@@ -534,4 +534,15 @@ public function grabNumRecords($table, array $criteria = [])
{
return $this->countInDatabase($table, $criteria);
}
public function updateInDatabase($table, array $data, array $criteria = []){

This comment has been minimized.

@Nitpick-CI

Nitpick-CI Jul 4, 2017

Opening brace should be on a new line

@Nitpick-CI

Nitpick-CI Jul 4, 2017

Opening brace should be on a new line

@eXorus eXorus changed the title from Add update in database to [Db] Add update in database Jul 4, 2017

Show outdated Hide outdated src/Codeception/Lib/Driver/Db.php
$set = [];
foreach ($data as $column => $value) {
$set[] = "{$column} = {$value}";

This comment has been minimized.

@Naktibalda

Naktibalda Jul 4, 2017

Member

I think that it would be better to quote column names witrh $this->getQuotedName($column), because column name can be a reserved keyword or contain some illegal characters.

@Naktibalda

Naktibalda Jul 4, 2017

Member

I think that it would be better to quote column names witrh $this->getQuotedName($column), because column name can be a reserved keyword or contain some illegal characters.

This comment has been minimized.

@eXorus

eXorus Jul 5, 2017

Contributor

Fixed

@eXorus

eXorus Jul 5, 2017

Contributor

Fixed

Show outdated Hide outdated src/Codeception/Lib/Driver/Db.php
public function update($table, array $data, array $criteria)
{
$set = [];

This comment has been minimized.

@Naktibalda

Naktibalda Jul 4, 2017

Member

Please throw exception if $data is empty.

@Naktibalda

Naktibalda Jul 4, 2017

Member

Please throw exception if $data is empty.

This comment has been minimized.

@eXorus

eXorus Jul 5, 2017

Contributor

Fixed

@eXorus

eXorus Jul 5, 2017

Contributor

Fixed

eXorus added some commits Jul 5, 2017

Show outdated Hide outdated src/Codeception/Lib/Driver/Db.php
$set = [];
foreach ($data as $column => $value) {
$set[] = $this->getQuotedName($column)." = '{$value}'";

This comment has been minimized.

@Naktibalda

Naktibalda Jul 5, 2017

Member

It would be better to use placeholders ? instead of injecting $value.

@Naktibalda

Naktibalda Jul 5, 2017

Member

It would be better to use placeholders ? instead of injecting $value.

Show outdated Hide outdated src/Codeception/Module/Db.php
{
$query = $this->driver->update($table, $data, $criteria);
$parameters = array_values($criteria);

This comment has been minimized.

@Naktibalda

Naktibalda Jul 5, 2017

Member

If placeholders are used in SET, $parameters will have to contain $data and $criteria.

$parameters = array_merge(array_values($data), array_values($criteria));
@Naktibalda

Naktibalda Jul 5, 2017

Member

If placeholders are used in SET, $parameters will have to contain $data and $criteria.

$parameters = array_merge(array_values($data), array_values($criteria));

eXorus added some commits Jul 5, 2017

@DavertMik

This comment has been minimized.

Show comment
Hide comment
Member

DavertMik commented Jul 6, 2017

@eXorus

This comment has been minimized.

Show comment
Hide comment
@eXorus

eXorus Jul 7, 2017

Contributor

@DavertMik @Naktibalda
Test added

Contributor

eXorus commented Jul 7, 2017

@DavertMik @Naktibalda
Test added

$query = $this->driver->update($table, $data, $criteria);
$parameters = array_merge(array_values($data), array_values($criteria));
$this->debugSection('Query', $query);
if (!empty($parameters)) {

This comment has been minimized.

@Naktibalda

Naktibalda Jul 7, 2017

Member

This condition isn't necessary, because $data can't be empty.

@Naktibalda

Naktibalda Jul 7, 2017

Member

This condition isn't necessary, because $data can't be empty.

public function update($table, array $data, array $criteria)
{
if (empty($data)) {
throw new \Exception(

This comment has been minimized.

@Naktibalda

Naktibalda Jul 7, 2017

Member

A more specific exception would be better. Codeception ModuleException is a good choice IMO.

@Naktibalda

Naktibalda Jul 7, 2017

Member

A more specific exception would be better. Codeception ModuleException is a good choice IMO.

This comment has been minimized.

@tiger-seo

tiger-seo Jul 7, 2017

Member

consider to throw InvalidArgumentException, which is more appropriate in this case

@tiger-seo

tiger-seo Jul 7, 2017

Member

consider to throw InvalidArgumentException, which is more appropriate in this case

This comment has been minimized.

@DavertMik

DavertMik Jul 8, 2017

Member

Agree on InvalidArgumentException

@DavertMik

DavertMik Jul 8, 2017

Member

Agree on InvalidArgumentException

@DavertMik

This comment has been minimized.

Show comment
Hide comment
@DavertMik

DavertMik Jul 9, 2017

Member

I will merge and update the requested changes.
Thanks @eXorus

Member

DavertMik commented Jul 9, 2017

I will merge and update the requested changes.
Thanks @eXorus

@DavertMik DavertMik merged commit 8a00379 into Codeception:2.3 Jul 9, 2017

3 of 4 checks passed

semaphoreci The build failed on Semaphore.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
wercker/build Wercker pipeline passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment