Cphalcon: [Resultset::update] returned value is always true regardless of transaction failure

Created on 8 Aug 2019 · 3Comments · Source: phalcon/cphalcon

Questions should go to https://forum.phalconphp.com
Documentation issues should go to https://github.com/phalcon/docs/issues

Expected and Actual Behavior

Describe what you are trying to achieve and what goes wrong.

As described in this forum post I have a ResultSet returned from a simple Model::find().
I then attempt to update the returned records as recommended by the docs when operating on ResultSets:

$robots = Robots::find([
    'conditions' => 'category = "toy"',
]);

$result = $robots->update([
    'category' => 'industrial',
]);

I had a database schema violation due to some old, non-conforming rows in the test database.
That meant that the update of the resultset did not succeed. However, $result always returned true, even on a failed update.

When looking at the source code for the update method, it seems that ResultSet::update returns true unconditionally.

I believe this behaviour to be a bug, since the regular way of detecting errors now doesn't work anymore:

if ($result === false) {
   // handle errors
   foreach($robots->getMessages() as $message) {
       echo $message;
   }
}

Right now I work around this issue by checking the message count of the resultset, but I feel that this is a bit hacky:

$messages = $robots->getMessages();
if (count($messages) > 0) {
    // handle errors
    foreach($robots->getMessages() as $message) {
        echo $message;
    }
}

Suggested Fix

I have not been able to work on a fix, but from my limited understanding of the code I have the following fix in mind.
The transaction variable in the update method seems to track failure so my proposed fix would simply be:

diff --git a/phalcon/Mvc/Model/Resultset.zep b/phalcon/Mvc/Model/Resultset.zep
index 2c77ca0..4e14180 100644
--- a/phalcon/Mvc/Model/Resultset.zep
+++ b/phalcon/Mvc/Model/Resultset.zep
@@ -637,7 +637,7 @@ abstract class Resultset
             connection->commit();
         }

-        return true;
+        return transaction;
     }

     /**

However, since I have only looked at the code at a glance, I am unsure if this is the right approach.

If there is anything else I can do to help, feel free to ask.

Details

Phalcon version: Observed with Phalcon 3.4.3, but confirmed to still be present in 4.0.x
PHP Version: 7.2.17
Operating System: Fedora 28 GNU/Linux 5.0.9-100.fc28.x86_64
Installation type: RPM based install
Server: nginx/1.12.1
Database: mysql Ver 15.1 Distrib 10.2.22-MariaDB, for Linux (x86_64) using readline 5.1