Fix return type error for unexpected method calls

[tkotosz]

Problem

This PR attempts to fix the issue of triggering a type error when an unexpected method call happens. Now it will return the correct type and let the code execution finish, so the predictions can be evaluated at the end of the test run.
See related comment here: #441 (comment)
PR which introduced the issue: #441
Relevant GitHub issues:

• Spies do not register as allowed method calls when stubbing is specified #120
• Callcenter returns null on non registered method call #472

Example - Classes to test

<?php

namespace Tkotosz\TestProject;

class Foo
{
    public function doStuff(Bar $bar)
    {
        $x = $bar->aaa();
        $bar->bbb($x+4);
    }
}

<?php

namespace Tkotosz\TestProject;

class Bar
{
    public function aaa(): int
    {
        return 42;
    }

    public function bbb(int $x): int
    {
        return $x+1;
    }
}

Example - spec 1

<?php

namespace spec\Tkotosz\TestProject;

use PhpSpec\ObjectBehavior;
use Tkotosz\TestProject\Foo;
use Tkotosz\TestProject\Bar;

class FooSpec extends ObjectBehavior
{
    function it_test_1(Bar $bar)
    {
        $bar->aaa()->willReturn(22);

        $this->doStuff($time);

        $bar->bbb(22+4)->shouldHaveBeenCalled();
    }
}

The test passes: The only expectation we have is that method bbb should have been called with 26 and that happened so everything is fine.
Note: This works with using shouldBeCalled before doStuff as well.

Example - spec 2

<?php

namespace spec\Tkotosz\TestProject;

use PhpSpec\ObjectBehavior;
use Tkotosz\TestProject\Foo;
use Tkotosz\TestProject\Bar;

class FooSpec extends ObjectBehavior
{
    function it_test_2(Bar $bar)
    {
        $bar->aaa()->willReturn(22);

        $this->doStuff($time);

        $bar->bbb(22+4)->shouldNotHaveBeenCalled();
    }
}

The test fails: The only expectation we have is that method bbb should NOT have been called with 26 and that happened so the test fails. (This also works using Argument::any() in the prediction)
Note: This works with using shouldNotBeCalled before doStuff as well.

Example - spec 3

<?php

namespace spec\Tkotosz\TestProject;

use PhpSpec\ObjectBehavior;
use Tkotosz\TestProject\Foo;
use Tkotosz\TestProject\Bar;

class FooSpec extends ObjectBehavior
{
    function it_test_3(Bar $bar)
    {
        $time->aaa()->willReturn(22);

        $this->doStuff($time);
    }
}

The test passes: There are no expectations so there is nothing that could fail here. So now the call to the method bbb is completely ignored, still not reported as an unexpected call. I am not sure if this is the desired behavior 🤔 since this basically eliminates the unexpected method call errors entirely. You will only see errors if you have a failed expectation.

[ciaranmcnulty]

Sorry for the late review; the code change seems fine

The change of behaviour is what worries me, not sure how to handle it! Possibly we should have a shouldNotBeCalled-type expectation for all methods that aren't otherwise stubbed?

In either case we should add some tests that capture the new behaviour explicitly

[tkotosz]

no prob @ciaranmcnulty

The change of behaviour is what worries me, not sure how to handle it! Possibly we should have a shouldNotBeCalled-type expectation for all methods that aren't otherwise stubbed?

Hmm interesting idea... I will probably have some time to play with this over the weekend, then I will get back to you. :)

[Jean85]

This seems to me a broken solution: in the third example, there is no behaviour defined for bbb(), which still needs to return an integer. Prophecy will instead try to return null, and the code will break with a fatal nonetheless.

I think that we're battling the same issue from different angles (I was getting into it with #508 + #509): stubs and spies can have undefined behavior, but that was defined in Prophecy long before PHP got return types. Now that we have them, returning null is no longer a valid fallback, so we have to do something else.

[tkotosz]

Prophecy will instead try to return null, and the code will break with a fatal nonetheless.

yep currently that is case, but it is changed in this PR, so prophecy returns an int instead of null to avoid the type error

[stof]

but it returns a generated value that might still break the following code if it does not match other invariants of the code.

[Jean85]

FTR, we're discussing reverting the whole #441 behaviour in #472.

[ciaranmcnulty]

I'd appreciate any comments on #528 as a way forward