php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Doc Bug #74569 Return in a finally clause silently ignores an exception thrown in a try clause
Submitted: 2017-05-10 16:44 UTC Modified: 2020-11-22 22:25 UTC
From: trianman at gmail dot com Assigned:
Status: Open Package: Scripting Engine problem
PHP Version: 7.1.4 OS: Any
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: trianman at gmail dot com
New email:
PHP Version: OS:

 

 [2017-05-10 16:44 UTC] trianman at gmail dot com
Description:
------------
Return in a finally clause silently ignores an exception thrown in a try clause.
I understand that there is no way to handle an exception here, but it MUST NOT be silently dropped out.

The interpreter should trigger an E_NOTICE or something similar here.

Looks like any version of PHP from 5.6 to 7.1.4 behaves in the same manner: https://3v4l.org/C16rP

Here is a similar issue: https://bugs.php.net/bug.php?id=68270

Test script:
---------------
<?php

class MyException extends Exception
{
    public function __construct($message = "", $code = 0, Throwable $previous = null)
    {
        print "2. Exception constructor\n";
        parent::__construct($message, $code, $previous);
    }
}

function ololo() {
    try {
        print "1. Start\n";
        throw new MyException("4. Exception message\n");
    } finally {
        return "3. Return statement\n";
    }
}

try {
    print ololo();
} catch (Exception $e) {
    print $e->getMessage();
}

Expected result:
----------------
1. Start
2. Exception constructor
PHP NOTICE: Dropped exception due the return statement in a finally block
3. Return statement

Actual result:
--------------
1. Start
2. Exception constructor
3. Return statement

Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2017-05-11 07:41 UTC] olavisau at gmail dot com
The correct way of writing this would be to catch the exception and throw it again, however this also fails. If it is not thrown again, then the behavior is correct.

<?php

class MyException extends Exception
{
    public function __construct($message = "", $code = 0, Throwable $previous = null)
    {
        parent::__construct($message, $code, $previous);
        print "2. Exception constructed\n";
    }
}

function throw_exception() {
    try {
        throw new MyException("5. Exception message\n");
    } catch(Exception $exception) {
        print "3. Exception caught and thrown again\n";
        throw $exception;
    } finally {
        return "4. Return statement\n";
    }
}

try {
    print "1. Start\n";
    print throw_exception();
} catch (Exception $e) {
    print $e->getMessage();
}
The flow should be: exception is thrown, caught by the inner block, thrown again, caught by the outer block and then the finally block should execute, returning the flow to 
print throw_exception();
 [2017-05-11 13:30 UTC] danack@php.net
> but it MUST NOT be silently dropped out.

That's a conclusion without rigorous justification.

btw The behaviour in PHP is the same as that which happens in Java: https://web.archive.org/web/20070922061412/http://weblogs.java.net/blog/staufferjames/archive/2007/06/_dont_return_in.html

It'd be a lot easier to say "Don't put returns in finally blocks, yo".

> The interpreter should trigger an E_NOTICE or something similar here.

This would almost certainly be better done with code analysis tools, rather than giving warnings on valid (though highly surprising) code.
 [2017-05-11 13:42 UTC] danack@php.net
This behaviour is also that used in Javascript.

function foo() {
  try {
	throw 42; 
  }
  finally {
    return "edge cases are hard";
  }
};

alert(foo());
 [2017-05-11 14:21 UTC] trianman at gmail dot com
> btw The behaviour in PHP is the same as that which happens in Java
> This behaviour is also that used in Javascript.

Sorry to hear that, but AFAIK nor Java nor JavaScript (ECMAScript) has no conventional (standartized) way to produce non critical warnings at the run time.

So PHP is better in this way and why can't we employ it?

> "Don't put returns in finally"
I believe that the good tool must help its users not to "shoot themselves" as much as possible. So the user becomes free from a lot of "Don't do something somewhere". If you don't agree you are free to use an assembler to do anything you want and remember a lot of such restrictions.

I can imagine another more expectable an clear way to handle this case. But it will break a BC so can be implemented only in a new major release of PHP.
Triggering E_NOTICE is the most convenient way for now.
 [2017-05-11 14:28 UTC] olavisau at gmail dot com
BC is broken with any solution. The fact that javascript behaves in the same way worries me. It's probably very hard to implement. I agree with the E_NOTICE.
 [2017-05-12 18:24 UTC] trianman at gmail dot com
2 danack@php.net

Sorry, I don't want to be mean. My English skills are not very good and it is sometimes hard to express my thoughts in a clear way.

I've just realized that the finally statement is some-kind of antipattern for me. Because finally is a shortcut for catching and re-throwing an \Exception class eg.

<?php
try {
  throw new CustomException();
} catch (CustomException $ex) {
  handleException();
} catch (\Exception $ex) {
  doThingsInFinally();
  throw $ex;
}
doThingsInFinally();
?>

Or with finally:

<?php
try {
  throw new CustomException();
} catch (CustomException $ex) {
  handleException();
} finally {
  doThingsInFinally();
}
?>

Of cause if we have a return statement instead of a doTihingsInFinally() method, we will lose an exception. But we doing it in a clear way. On the other hand finally statement hides out from developer's eyes what actually is going.

So for my mind a little copy-paste is a lesser evil than an unclear behavior. For all other guys the E_NOTICE will be enough. (:
 [2017-05-14 19:09 UTC] cmb@php.net
-Type: Bug +Type: Documentation Problem
 [2017-05-14 19:09 UTC] cmb@php.net
It seems to me this is simply a documentation issue.
 [2017-05-16 21:16 UTC] danack@php.net
The Php Inspections (EA Extended) guy has added this as code smell to that tool.

You could consider using that tool and giving them some money for being so nice: https://www.indiegogo.com/projects/php-inspections-ea-extended-a-code-analyzer-security#/
 [2020-11-22 19:16 UTC] anhlephuoc at gmail dot com
This faulty behaviour of the return statement in the finally{} of the the try{} block extends beyond the catchable error/exceptions.
1%0;  // No warning or error.
unknown_function(); // not reported. Program not aborted
It took me days to locate a misspelled function, because no error is reported at all and program would normally abort, now continue with undesirable behaviour.
Example code:
<?php
error_reporting(E_ALL|E_STRICT);
function do_test(): int {
  try {
    1/0;                    // a warning in php 7 - reported as documented
    nonexistent_function(); // a fatal error  - not reported - program continues
    1%0;                    // a fatal error
    return 0;
  } catch (Exception $e) {
    printf("CATCH\n");
    return 1;
  } finally {
    printf("FINALLY\n");
    return 2;
  }
  return 3;
}
printf("%d\n", do_test());
 [2020-11-22 22:25 UTC] requinix@php.net
@anhlephuoc: The behavior you're seeing is because the "fatal error" from the undefined function call is actually an exception that extends Error, not an exception that extends Exception. You aren't catching it which means normally it would have been thrown - if not for the fact that your finally block overrides that behavior and returns a value instead.

As of PHP 7, "catch (Exception $e)" does not catch every single exception that could be thrown. Use the Throwable interface for that.
https://3v4l.org/6N4d9
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Tue Mar 19 09:01:30 2024 UTC