Contract propagation bug?

[panosru]

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

https://github.com/php-deal/framework/blob/c508a19dd9b663dc195468854ed85effe151237f/demo/Demo/Account.php#L42-L51

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 :wink:. 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 :wink:

[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.

[panosru]

@icanhazstring Indeed you are correct, I by mistake replaced the @inheritdoc with @Contract\Inherit but forgot to remove the { }.

I run composer update and removed the { } around the @Contract\Inherit:

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

and now it seems to work:

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

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

Thanks for the hint! That debug repo was created only for debug purposes of current issue, it does not reflect the way I'm handling my production or development projects, but in any case thanks!

Have a great day and thanks for the implementation!

[icanhazstring]

@panosru just released a new version 0.5 with the new feature.

Closing this.