Skip to content
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

Return the number of affected rows when run Mapper remove method #86

Open
ricardofontanelli opened this issue Mar 30, 2016 · 9 comments

Comments

@ricardofontanelli
Copy link

Hi Guys,

How about return the number of affected rows when run Mapper remove method? Is it possible (currently) or interesting to everyone in a new PR?

@augustohp
Copy link
Member

What are your thoughts @tonicospinelli?

@tonicospinelli
Copy link
Member

it is interesting!

I new from here, but will give my 5 cents 😄

now, Mapper class will remove a specific "entity" and I'm not sure this behaviour should be happen on it.

Probably, Db class is the good way to execute a batch delete.

@ricardofontanelli could you give a example?

@ricardofontanelli
Copy link
Author

@tonicospinelli tks for your response. Well, I'm trying delete an single record in the database, but the flushmethod returns nullif the record was deleted or not (in case that the id doesn't exist, for example).

$file1 = new \stdClass;
$file1->id = 1248; //This record doesn't exist in the database, it could be null, false or a string, for example

$file2 = new \stdClass;
$file2->id = 3; // Real record that exist in the database

$mapper['local']->invoice_file->remove( $file1 );
var_dump( $mapper['local']->flush() );// Result 1 is = NULL

$mapper['local']->invoice_file->remove( $file2 );
var_dump( $mapper['local']->flush() );//Result 2 is  = NULL

Supposing that I changed my code and after that the Controller stop to get correctly the id to be removed. I'll execute the remove method thinking that everything is OK!

@tonicospinelli
Copy link
Member

@ricardofontanelli I got your point of view!

In your case, you will remove in a controlled environment and you know only remove method will be called in you application.

If you take a look inside of flushSingle method, it will call the methods rawDelete, rawInsert and rawUpdate and the result about affected rows could be weird or only about update or insert if it will be called together with remove.

Honestly, I don't think Mapper needs to do it when flush is called. My suggestion is implement in Db class a lastStatement property to get affected rows from last statement executed.

$db = new \Respect\Relational\Db(new PDO('connection', 'user', 'pass'));
$mapper = new Mapper($db);

$file = new \stdClass;
$file->id = 3;

$mapper->invoice_file->remove($file);
$mapper->flush();

var_dump($mapper->getConnection()->getLastStatement()->rowCount()); // result from last executed statement
// or
var_dump($db->getLastStatement()->rowCount()); // result from last executed statement
// or
var_dump($db->affectedRows()); // it encapsulate result from last executed statement

hey, core developers, @Respect/core do you have any different idea how to do it?

@ricardofontanelli
Copy link
Author

Tks again @tonicospinelli, I did some changes in my repo fork, something like that you said about get the last statement in each Dd::prepare and Db::executeStatement, then I can get each PDOStatement::rowCount and acumulate in the flushmethod to return the number of affected rows (deleted or updated).

@tonicospinelli
Copy link
Member

ping @henriquemoody @augustohp @alganet

@augustohp
Copy link
Member

@tonicospinelli I agree with your suggestion, specially because making flush return a result of the changes would be tricky and confusing to say the least.

The major problem I see is that the lastStatement would not be always the remove if other operations were made (as @tonicospinelli mentioned). We could address it in two ways AFAIK:

  1. Document the problem with that method.
  2. Implement a way to retrieve lastStatement(Db::STATEMENT_TYPE_DELETE).

@tonicospinelli
Copy link
Member

thanks, @augustohp this way is good approach for me! it could be encapsulated with lastRemoveStatement() method too.

@ricardofontanelli
Copy link
Author

Thanks @tonicospinelli and @augustohp, I've built some unit tests and getting each lastStatement it works fine with this sequence: select, insert, update, invalid delete and delete, it results in 2 affected rows, because I'm getting the rowCountin each loop inside flush. But as you think that change flush method would be tricky/confusing, I'll extend and change class. Tks!

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

No branches or pull requests

3 participants