Contract propagation bug?

[panosru]

I'm not sure if I'm doing something wrong but if I modify this:

framework/demo/Demo/Account.php

Lines 42 to 51 in c508a19

	/**
	* Returns current balance
	*
	* @Contract\Ensure("$__result == $this->balance")
	* @return float
	*/
	public function getBalance()
	{
	return $this->balance;
	}

into this:

    /**
     * {@inheritdoc}
     */
    public function getBalance()
    {
        // return $this->balance;
        return 10;
    }

I should've receive the following error:

Fatal error: Uncaught DomainException: Invalid return value received from the assertion body, only boolean or void can be returned in deal/demo/Account.php on line 51

PhpDeal\Exception\ContractViolation: Contract $__result == $this->balance violated for Demo\Account->getBalance in deal/demo/Account.php on line 51

instead, I'm getting:

$ php demo.php
10

this:

    /**
     * Returns current balance
     *
     * @Contract\Ensure("$__result == $this->balance")
     * @return float
     */
    public function getBalance()
    {
        // return $this->balance;
        return 10;
    }

returns the above-mentioned error as expected.

[icanhazstring]

Hi @panosru thanks for reaching out. As far as I can see you removed the contract annotation. Without it the underlying aop framework can't predict where to check for violations.

I hope I could help you.

[icanhazstring]

@panosru did you expect the @inheritdoc to use the @Contract\Ensure from the AccountContractInterface? If so and this does not work this might be a bug.

@lisachenko is this supposed to work?

[panosru]

@icanhazstring I'm sorry for my late reply, I had a flight to catch, yes I expected the @inheritdoc to use the @Contract\Ensure from the parent.

from what I read here

class Foo extends FooParent
{
    /**
     * @param int $amount
     * @Contract\Verify("$amount != 1")
     * {@inheritdoc}
     */
    public function bar($amount)
    {
        ...
    }
}
    
class FooParent
{
    /**
     * @param int $amount
     * @Contract\Verify("$amount != 2")
     */
    public function bar($amount)
    {
        ...
    }
}

Foo::bar does not accept '1' and '2' literals as a parameter.

That tells me that Foo::bar inherits the @Contract\Verify from FooParent::bar because of @inheritdoc, based on the example on the docs that should work:

class Foo extends FooParent
{
    /**
     * @param int $amount
     * {@inheritdoc}
     */
    public function bar($amount)
    {
        ...
    }
}
    
class FooParent
{
    /**
     * @param int $amount
     * @Contract\Verify("$amount != 2")
     */
    public function bar($amount)
    {
        ...
    }
}

but it seems that @inheritdoc works only when you have a @Contract\* in the method you add the @inheritdoc, otherwise is ignored.

The "problem" here is that @inheritdoc works only when you have specified in that docblock a @Contract\*, but @inheritdoc is widely used and having the library detecting @inheritdoc would bring more issues rather than solving the problem.

I suppose that @inheritdoc could work there where you use @Contract\* but if in my case you just want to inherit the parent contracts a separate annotation should be introduced @Contract\Inherit for instance.

[icanhazstring]

@panosru thanks for the explanation. Seems like a real bummer 😉. Will try to look into that.

[icanhazstring]

Just cross referencing #30 with this, since I realized the exception you expect seems not correct.
This should be changed, and might be in the next major version.

Anyways, I dug into your problem and can confirm it. The problem with that is, the aspects are only working for methods with an registered annotation. I will try to figure out a good way to scan for inherited contracts.

---

Edit: Ok so now I only came up with one solution.

Problem is @inheritdoc is, as you mentioned, used in a wide range of files. If we make GoAop (the underlying framework) to make a proxy for all classes with this annotation, everything and everyone will freak out 😉

The only thing that came to my mind is a new annotation @InheritContracts on class level.
So you will need to add this annotation on the class you are writing, which will make sure every method with @inheritdoc will be checked for existing contracts in a parent class recursively. This might help you solve your problem.

Maybe @lisachenko has an opinion on this as well 😉

[panosru]

@icanhazstring So, to visualise it, based on the solution you described, it would look like this:

/**
 * @InheritContracts
 */
class Foo extends FooParent
{
    /**
     * @param int $amount
     * {@inheritdoc}
     */
    public function bar($amount)
    {
        ...
    }
}
    
class FooParent
{
    /**
     * @param int $amount
     * @Contract\Verify("$amount != 2")
     */
    public function bar($amount)
    {
        ...
    }
}

Tagging a class will then seek its methods for potential @inheritdoc in their dockblock, am I right?

[icanhazstring]

@panosru exactly. That's the plan 😉

[icanhazstring]

@panosru I have made a branch where I implemented an new annotation.
@Inherit as you proposed earlier.

You can try it out by using the dev branch:

Add this repo to your composer.json

{
    "repositories": [
        {"type":"vcs", "url":"https://github.com/php-deal/framework"}
    ]
}

Require the needed dev branch.
composer require phpdeal/framework:dev-feature/inherit-contracts

---

The functionality is as follows:
@Inherit works for both Class and Method annotation.

• since there is no @inheritdoc on class level, the Invariant annotation already has an auto inheritance
• for methods all existing contracts will work with @inheritdoc and look in the inheritance tree
• if you skip the @inheritdoc annotation and use Inherit annotation, you will get the same result
  • this also works without an existing annotation (Ensure, Verify) on the current method.

---

To illustrate:

Given this interface

interface A 
{
    /*
     * @Contract\Ensure(...)
     */
    public function fu();
}

class B implements A
{
    /*
     * @Contract\Inherit
     */
    public function fu() {}
}

If this basically works for you, let me now, and I will clean up the branch, add tests, as well update the readme.

[panosru]

@icanhazstring Thanks for the branch! I cloned it and I'm testing it but it seems to not working for me.

I have created a debug repo which you can clone and run composer install then php test.php will return 10 instead of error as it supposed to...

Also you had a typo on your previous message, it should be composer require php-deal/framework:dev-feature/inherit-contracts (with dash)

[icanhazstring]

@panosru will give it a try and see what is wrong.

---

And back. Ok found the problem.
There are two problems.

1. You need to avoid adding {} around annotations. If they are there, the annotation reader can't find them
2. Currently you need to supply the @inheritdoc annotation as well. I will change this, so you don't have to add this.

Also as a hint: If you need minimum-stability as dev, add prefer-stable: true as well. This will make sure that every other package that is not needed as dev, will be installed in stable version 😉

[icanhazstring]

@panosru as you can see I just provided a new PR solving your issue.
Feel free to look it up. Can you please try again if this implementation works for you.

If so I will merge this an tag a new release.