Relational icon indicating copy to clipboard operation
Relational copied to clipboard

Return the number of affected rows when run Mapper remove method

Open ricardofontanelli opened this issue 8 years ago • 9 comments

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?

ricardofontanelli avatar Mar 30 '16 14:03 ricardofontanelli

What are your thoughts @tonicospinelli?

augustohp avatar Mar 30 '16 19:03 augustohp

it is interesting!

I new from here, but will give my 5 cents :smile:

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?

tonicospinelli avatar Mar 30 '16 20:03 tonicospinelli

@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!

ricardofontanelli avatar Mar 31 '16 04:03 ricardofontanelli

@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?

tonicospinelli avatar Apr 01 '16 11:04 tonicospinelli

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).

ricardofontanelli avatar Apr 01 '16 13:04 ricardofontanelli

ping @henriquemoody @augustohp @alganet

tonicospinelli avatar Apr 04 '16 11:04 tonicospinelli

@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).

augustohp avatar Apr 04 '16 21:04 augustohp

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

tonicospinelli avatar Apr 05 '16 17:04 tonicospinelli

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!

ricardofontanelli avatar Apr 07 '16 02:04 ricardofontanelli