RFC: New unexpected method call behaviour

[ciaranmcnulty]

There will likely be errors in this, corrections/comments are welcome

inspired by this comment https://github.com/phpspec/prophecy/issues/527#issuecomment-802103309

Motivation

441 changed the behaviour in a way that seemed sensible at the time, but has caused DX issues. To fix I seems to require a major version bump, so let's take the chance to properly think through the behaviour and work out what's best for the next major.

Historical context

Prophecy is an opinionated tool, and we have tried to be guided not by what makes testing easy, but by what pushes developers in the right direction.

Back in the dawn of time, in the PHP 5.3 days we did not have return types. It was decided that Prophecy would essentially have two modes:

1. for objects that haven't got any expectations, all method calls return null
2. for objects with expectations, unexpected method calls throw an immediate exception

Spy problems

This caused issues when using a spy and also stubbing a method, cases like this would typically throw:

$stub->foo()->willReturn('bar');

$this->bar($stub);

$stub->baz()->shouldHaveBeenCalled();

This is why #441 was implemented, but this now means that the call stack for an unexpected message is messed up, and null returns then being passed on to other methods cause an error down the line before the unexpected call error would have been displayed.

Return type problems

Modern PHP is at the stage where return types can and should be added to nearly all methods. This makes the fallback behaviour far less useful as in most cases it's a type error.

New Behaviour

In a new major version, Prophecy doubles will become strict by default.

That is: even if no expectations are set they will throw an Exception at call time for any unexpected method calls with one exception: void-returning methods.

This will allow the Spy behaviour for command-like methods, as long as the baz example returns void:

$stub->foo()->willReturn('bar');

$this->bar($stub);

$stub->baz()->shouldHaveBeenCalled();

This will disallow some previous Dummy-like use of doubles

BC Impacts

Tests that don't specify return values for doubles will break unless those doubles only have void method calls.

PhpSpec and the PHPUnit-bridge depend on prophecy 1, so can choose when to expose this breaking change to their users.

[stof]

This will disallow some previous Dummy-like use of doubles

if it is really a dummy that you need, you won't have any issue though, as no calls would be made on it. And this could be prepared by triggered a deprecation warning for such cases in 1.x

[Jean85]

The deprecations would be a nice touch!

I agree on the rest of the RFC. I would advise to bump a major in phpunit-prophecy too, to avoid silent major bumps for end users. As for PHPUnit, they already deprecated the direct integration, so they could possibly ignore this and push to the trait further.

[stof]

yeah, phpunit-prophecy would get a major version bump to v3 to opt-in for prophecy 2. That's the plan.

[DonCallisto]

That is: even if no expectations are set they will throw an Exception at call time for any unexpected method calls with one exception: void-returning methods.

I'm not sure that's a understandable behaviour from developer POV. I mean, why a method that returns a certain type will throw immediatly, whereas a method that returns void does not? If I understand correctly, a spy with a "typed return type" can't be ever used as a spy, if a method invocation is involved in the exaple. Am I right?

[stof]

@DonCallisto it can be used as a spy, but only if you also configured it to tell Prophecy what should be returned by the call

[DonCallisto]

Yeah, right. Only not so intuitive behaviour is when you would like to assert shouldNotHaveBeenCalled but you are forced, somehow, to do something like

$stub->method(Argument::cetera())
 ->willReturn(...);
$this->sutMethod($stub);
$stub->method(Argument::cetera())
 ->shouldNotHaveBeenCalled();

Argument::cetera() is just a placeholder for this example, of course it's not important for what I'm trying to point out. But I think it's kind of acceptable in the short term to tackle and get rid of the issue.

[TimoBakx]

Since almost all of my doubles already throw an exception without the ->willReturn(), this all good for me. As long as ->willThrow() will also not create any issues without a ->willReturn() present.

[ciaranmcnulty]

@DonCallisto I don't think that would be necessary:

$this->sutMethod($stub);

$stub->method(Argument::cetera())->shouldNotHaveBeenCalled();

This will be fine, if the method is called the test will fail because the stub will throw an Exception and the matcher is just information for a reader

There are other cases where you might want to do both:

$stub->save(Argument::any())->willReturn(null);

$this->savePrimes(2,3,4,5);

$stub->method(4)->shouldNotHaveBeenCalled();

[DonCallisto]

The last case you mentioned is way too generic from my standpoint, because forces the user to be way too generic for the call, whereas I might want to assert that a stub will return something under certain input and want to be sure that a call with other input has not hitten the stub. Of course the shouldNotHaveBeenCalled would take place only under the circumstances provided in the latter example. Don't get me wrong, I'm not against this BC, but I want to point out that shouldNotHaveBeenCalled and shouldHaveBeenCalled will lost a lot of their expresiveness and become pretty much useless imho.

[stof]

@TimoBakx that will be fine. All the ->will* methods are defining expectations, not only willReturn.

@DonCallisto you don't assert that a stub returns something. You define it.

want to be sure that a call with other input has not hitten the stub

that's already done automatically by the UnexpectedCallException thrown by Prophecy (except that today, we have the special case where nothing defined as expected calls means everything is accepted, which is scheduled for removal in this RFC).

[DonCallisto]

@stof yes, sorry, my bad, I mean "define a canned method response" not assert, of course. BTW well, thanks for pointing those things out, now everything seems fine to me.

[roman-eonx]

Hi guys, Are there chances to see this behavior in the next major release? We are currently using a similar behavior but implementing it as a patch of Prophecy. It would be good to see it as a package feature.

[Jean85]

FYI #526 already implements this and we're using that fork since April with no issues.

[Jean85]

Reporting back again: I had to cherry pick #527, but apart from that I'm still using #526 since April on a sensitive project without any issues.

[BladeMF]

Guys, any chance of something solving this? Why is this stalled? It is absurdly difficult to debug tests for methods with return values and expected calls when the expectation is not met - you get a return value exception rather than an unexpected call exception.