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

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

Closed
gytsen opened this issue Aug 8, 2019 · 3 comments

Comments

@gytsen
Copy link

commented Aug 8, 2019

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

@gytsen gytsen changed the title Resultset's returned value is always true regardless of transaction failure [Resultset::update] returned value is always true regardless of transaction failure Aug 8, 2019

ruudboon added a commit to ruudboon/cphalcon that referenced this issue Aug 8, 2019
ruudboon added a commit to ruudboon/cphalcon that referenced this issue Aug 8, 2019
@ruudboon ruudboon referenced this issue Aug 8, 2019
4 of 5 tasks complete
@ruudboon

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@gytsen Thank you for reporting Joost. If pull get's accepted it should be fixed in the beta.2 release.

@gytsen

This comment has been minimized.

Copy link
Author

commented Aug 8, 2019

That's awesome! Thank you very much for fixing this!

@sergeyklay

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Fixed in the 4.0.x branch. Thank you for the bug report.

@sergeyklay sergeyklay closed this Aug 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.