Issue with exception display on command line

[lukenm]

Hi there, would appreciate a discussion regarding this issue, thanks. On master, ref a1e3bd9.

As a developer using php-deal I expect to see a ContractViolation exception on the command line when a contract fails when running a script through the cli. I expect the ContractViolation exception to have a message describing the contract condition that failed, and the file/line of the method that failed; with a stack trace of my program.

The issue is, when running through the cli (tested with PHP 5.5.9 and 7.1.10) I see a DomainException, which doesn't have a meaningful message, and has a stack trace of the internals of the phpdeal and goaop frameworks. This does not help me debug my program.

I have tried various versions of PHP with xdebug on/off with varying results.

Expected:

PHP Fatal error:  Uncaught exception 'PhpDeal\Exception\ContractViolation' with message 'Contract $this->balance > 0 violated for Demo\Account->deposit' in phpdeal/demo/Demo/Account.php:38
Stack trace:
#0 phpdeal/src/Aspect/InvariantCheckerAspect.php(57): PhpDeal\Aspect\AbstractContractAspect->ensureContracts(Object(Go\Aop\Framework\DynamicClosureMethodInvocation), Array, Object(Demo\Account), 'Demo\Account__A...', Array)
#1 phpdeal/vendor/goaop/framework/src/Aop/Framework/AroundInterceptor.php(34): PhpDeal\Aspect\InvariantCheckerAspect->invariantContract(Object(Go\Aop\Framework\DynamicClosureMethodInvocation))
#2 phpdeal/vendor/goaop/framework/src/Aop/Framework/DynamicClosureMethodInvocation.php(40): Go\Aop\Framework\AroundInterceptor->invoke(Object(Go\Aop\Framework\DynamicClosureMethodInvocation))
#3 phpdeal/src/Aspect/PostconditionChecke in phpdeal/demo/Demo/Account.php on line 38

Actual:

PHP Fatal error:  Uncaught exception 'DomainException' with message 'Invalid return value received from the assertion body, only boolean or void can be returned' in phpdeal/src/Aspect/AbstractContractAspect.php:86
Stack trace:
#0 phpdeal/src/Aspect/InvariantCheckerAspect.php(57): PhpDeal\Aspect\AbstractContractAspect->ensureContracts(Object(Go\Aop\Framework\DynamicClosureMethodInvocation), Array, Object(Demo\Account), 'Demo\Account__A...', Array)
#1 phpdeal/vendor/goaop/framework/src/Aop/Framework/AroundInterceptor.php(34): PhpDeal\Aspect\InvariantCheckerAspect->invariantContract(Object(Go\Aop\Framework\DynamicClosureMethodInvocation))
#2 phpdeal/vendor/goaop/framework/src/Aop/Framework/DynamicClosureMethodInvocation.php(40): Go\Aop\Framework\AroundInterceptor->invoke(Object(Go\Aop\Framework\DynamicClosureMethodInvocation))
#3 phpdeal/src/A in phpdeal/demo/Demo/Account.php on line 38

I've had some trouble trying to work this one out. I think it has something to do with the exception chaining. I've tried to create a simple test case to isolate the issue outside of the php-deal framework, but haven't been successful - my isolated tests with exception chaining always work as expected, with separate messages for each exception thrown. When running tests with the php-deal framework, the ContractViolation exception is always missing from the cli output.

When I step through the code, the ContractViolation appears to be created correctly, and I can catch it in the outer scope. It seems to be a problem with the way PHP is displaying the exceptions and stack trace.

To recreate the issue, apply the following patch, and run the demo:

diff --git a/demo/Demo/Account.php b/demo/Demo/Account.php
index 8116a03..1ea5a3f 100644
--- a/demo/Demo/Account.php
+++ b/demo/Demo/Account.php
@@ -36,7 +36,7 @@ class Account implements AccountContractInterface
      */
     public function deposit($amount)
     {
-        $this->balance += $amount;
+        $this->balance = 0;
     }
 
     /**

I can temporarily get things working as expected by disabling the exception chaining as follows:

diff --git a/src/Exception/ContractViolation.php b/src/Exception/ContractViolation.php
index a957d26..fb2fb05 100644
--- a/src/Exception/ContractViolation.php
+++ b/src/Exception/ContractViolation.php
@@ -32,6 +32,8 @@ class ContractViolation extends \LogicException
         $objName    = is_object($obj) ? get_class($obj) : $obj;
         $method     = $invocation->getMethod();
 
+        $previous = null;
+
         $message = "Contract {$contract} violated for {$objName}->{$method->name}";
         parent::__construct($message, 0, $previous);
 

Some questions:

1. I don't understand why the exception chaining isn't working as it currently stands. Can you repeat the issue? Can you explain it?
2. As a developer using php-deal, I would prefer to see framework errors separate to ContractViolation exceptions. Does it make sense to chain the exceptions in this way? To me it would seem better to catch the DomainException at Aspect/AbstractContractAspect.php:89, and throw a ContractViolation exception without chaining (the Domain Exception doesn't appear to convey any useful information to the developer).
3. Should other Error or Exception types should be handled differently? It might make sense to have a different exception type in the case the syntax/parsing of the contract failed.
4. On code review, it appears that catching \Error $internalError at Aspect/AbstractContractAspect.php:89 and chaining it in Aspect/AbstractContractAspect.php:91 will fail due to the type hint in Exception/ContractViolation.php:29. I didn't see a test case for that - Aspect/AbstractContractAspect.php:91 isn't included in the code coverage.

[icanhazstring]

@lukenm I looked into that issue and figured out why you only get the topmost exception.
The exception chaining is working out fine. The problem is your exception handler.

If you run your script from cli, most likely your xdebug exception handler will kick in for unhandled exceptions. Per default the stacktrace for errors or exceptions is disabled in xdebug.

To properly debug your script try to change your xdebug configurations as follows (don't forget to restart fpm or whatever you use):

xdebug.show_error_trace=1
xdebug.show_exception_trace=1

Then you should get something similar to this:

PHP DomainException:  Invalid return value received from the assertion body, only boolean or void can be returned in /vagrant/src/Aspect/AbstractContractAspect.php on line 96
PHP Stack trace:
PHP   1. {main}() /vagrant/demo/demo.php:0
PHP   2. Demo\Account->deposit() /vagrant/demo/demo.php:18
PHP   3. Go\Aop\Framework\DynamicClosureMethodInvocation->__invoke() /vagrant/demo/cache/_proxies/Demo/Account.php:27
PHP   4. Go\Aop\Framework\DynamicClosureMethodInvocation->proceed() /vagrant/vendor/goaop/framework/src/Aop/Framework/AbstractMethodInvocation.php:89
PHP   5. Go\Aop\Framework\BeforeInterceptor->invoke() /vagrant/vendor/goaop/framework/src/Aop/Framework/DynamicClosureMethodInvocation.php:43
PHP   6. Go\Aop\Framework\DynamicClosureMethodInvocation->proceed() /vagrant/vendor/goaop/framework/src/Aop/Framework/BeforeInterceptor.php:35
PHP   7. Go\Aop\Framework\AroundInterceptor->invoke() /vagrant/vendor/goaop/framework/src/Aop/Framework/DynamicClosureMethodInvocation.php:43
PHP   8. PhpDeal\Aspect\InvariantCheckerAspect->invariantContract() /vagrant/vendor/goaop/framework/src/Aop/Framework/AroundInterceptor.php:34
PHP   9. Go\Aop\Framework\DynamicClosureMethodInvocation->proceed() /vagrant/src/Aspect/InvariantCheckerAspect.php:53
PHP  10. Go\Aop\Framework\AroundInterceptor->invoke() /vagrant/vendor/goaop/framework/src/Aop/Framework/DynamicClosureMethodInvocation.php:43
PHP  11. PhpDeal\Aspect\PostconditionCheckerAspect->postConditionContract() /vagrant/vendor/goaop/framework/src/Aop/Framework/AroundInterceptor.php:34
PHP  12. PhpDeal\Aspect\PostconditionCheckerAspect->ensureContracts() /vagrant/src/Aspect/PostconditionCheckerAspect.php:56
PHP PhpDeal\Exception\ContractViolation:  Contract $this->balance == $__old->balance+$amount violated for Demo\Account->deposit in /vagrant/demo/Demo/Account.php on line 38
PHP Stack trace:
PHP   1. {main}() /vagrant/demo/demo.php:0
PHP   2. Demo\Account->deposit() /vagrant/demo/demo.php:18
PHP   3. Go\Aop\Framework\DynamicClosureMethodInvocation->__invoke() /vagrant/demo/cache/_proxies/Demo/Account.php:27
PHP   4. Go\Aop\Framework\DynamicClosureMethodInvocation->proceed() /vagrant/vendor/goaop/framework/src/Aop/Framework/AbstractMethodInvocation.php:89
PHP   5. Go\Aop\Framework\BeforeInterceptor->invoke() /vagrant/vendor/goaop/framework/src/Aop/Framework/DynamicClosureMethodInvocation.php:43
PHP   6. Go\Aop\Framework\DynamicClosureMethodInvocation->proceed() /vagrant/vendor/goaop/framework/src/Aop/Framework/BeforeInterceptor.php:35
PHP   7. Go\Aop\Framework\AroundInterceptor->invoke() /vagrant/vendor/goaop/framework/src/Aop/Framework/DynamicClosureMethodInvocation.php:43
PHP   8. PhpDeal\Aspect\InvariantCheckerAspect->invariantContract() /vagrant/vendor/goaop/framework/src/Aop/Framework/AroundInterceptor.php:34
PHP   9. Go\Aop\Framework\DynamicClosureMethodInvocation->proceed() /vagrant/src/Aspect/InvariantCheckerAspect.php:53
PHP  10. Go\Aop\Framework\AroundInterceptor->invoke() /vagrant/vendor/goaop/framework/src/Aop/Framework/DynamicClosureMethodInvocation.php:43
PHP  11. PhpDeal\Aspect\PostconditionCheckerAspect->postConditionContract() /vagrant/vendor/goaop/framework/src/Aop/Framework/AroundInterceptor.php:34
PHP  12. PhpDeal\Aspect\PostconditionCheckerAspect->ensureContracts() /vagrant/src/Aspect/PostconditionCheckerAspect.php:56

Which is the correct exception chain from top > down.
Hope I could help you :)

As for the questions:

1. xdebug on cli or rather the global exception handler is your enemy The chaining works as intended.
2. The DomainException is catched and wrapped in a ContractViolation thus the exception chain.
3. That is a good point - maybe @lisachenko could say smt about this :)
4. If I am correct, there is currently not special >=php7 test to see if \Error cases are handled - this should probably be added.

[icanhazstring]

Hi @lukenm do you need any further information on this?
Otherwise I would like to close this issue.

[icanhazstring]

@lukenm ping. Closing this on Wednesday

[icanhazstring]

Closed due to inactivity.
If you need any further help. Feel free to open again.