DDC-1336: em::transactional() returns TRUE if I return FALSE in the closure #1673

New Issue

admin · 2026-01-22T13:21:47+01:00

admin commented

2026-01-22 13:21:47 +01:00

Originally created by @doctrinebot on GitHub (Aug 17, 2011).

Originally assigned to: @beberlei on GitHub.

Jira issue originally created by user ambis:

In short:

$return = $entityManager->transactional(function() {
    return false;
});

var_dump($return); // returns true, should be false

$return = $entityManager->transactional(function() {
    return 'foobar';
});

var_dump($return); // returns 'foobar'

Best fix I think would be just to return whatever the closure returns, be it null, boolean or otherwise.

Originally created by @doctrinebot on GitHub (Aug 17, 2011). Originally assigned to: @beberlei on GitHub. Jira issue originally created by user ambis: In short: ``` $return = $entityManager->transactional(function() { return false; }); var_dump($return); // returns true, should be false $return = $entityManager->transactional(function() { return 'foobar'; }); var_dump($return); // returns 'foobar' ``` Best fix I think would be just to return whatever the closure returns, be it null, boolean or otherwise.

admin added the Bug label 2026-01-22 13:21:47 +01:00

admin closed this issue

2026-01-22 13:21:49 +01:00

admin commented

2026-01-22 13:21:50 +01:00

@doctrinebot commented on GitHub (Aug 17, 2011):

Comment created by ambis:

While this might be a bug in it's own right, I just realized that I did this a bit wrong in the first place.

I shouldn't return false in the closure when something goes wrong, but to throw an exception so that the transaction is rolled ba... No wait... But then it'll re-throw the exception and I need to wrap the ::transactional() in try catch... Huh!

What if:


        try {
            $return = $func($this);

            if (false === $return) {
                $this->close();
                $this->rollback();
                return $return;
            }

            $this->flush();
            $this->conn->commit();

            return $return;
        } catch (Exception $e) {
            $this->close();
            $this->conn->rollback();

            throw $e;
        }

This way you could silently abort the whole thing by returning false in the closure.

@doctrinebot commented on GitHub (Aug 17, 2011): Comment created by ambis: While this might be a bug in it's own right, I just realized that I did this a bit wrong in the first place. I shouldn't return false in the closure when something goes wrong, but to throw an exception so that the transaction is rolled ba... No wait... But then it'll re-throw the exception and I need to wrap the ::transactional() in try catch... Huh! What if: ``` try { $return = $func($this); if (false === $return) { $this->close(); $this->rollback(); return $return; } $this->flush(); $this->conn->commit(); return $return; } catch (Exception $e) { $this->close(); $this->conn->rollback(); throw $e; } ``` This way you could silently abort the whole thing by returning false in the closure.

admin commented

2026-01-22 13:21:51 +01:00

@doctrinebot commented on GitHub (Nov 19, 2011):

Comment created by @beberlei:

The way out this method in case of error is rollback + exception. You will need to catch this exception again, but at a higher level of your application when errors are logged and error pages shown.

If you need another behavior you should implement your own method.

Since the method has no @return docblock we cant assume what the correct behavior is, since none is defined. The code has to stay this way now because of Backwards compatibility reasons.

@doctrinebot commented on GitHub (Nov 19, 2011): Comment created by @beberlei: The way out this method in case of error is rollback + exception. You will need to catch this exception again, but at a higher level of your application when errors are logged and error pages shown. If you need another behavior you should implement your own method. Since the method has no @return docblock we cant assume what the correct behavior is, since none is defined. The code has to stay this way now because of Backwards compatibility reasons.

admin commented

2026-01-22 13:21:52 +01:00

@doctrinebot commented on GitHub (Nov 19, 2011):

Issue was closed with resolution "Invalid"

@doctrinebot commented on GitHub (Nov 19, 2011): Issue was closed with resolution "Invalid"

Sign in to join this conversation.

Branches Tags

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: doctrine/archived-orm#1673