Callcenter returns null on non registered method call

[jeanlucc]

Hello,

First I do not often submit issues, so please accept my apologies if it is far from perfect.
This comment https://github.com/phpspec/prophecy/pull/441/files#r388191626 is perfectly true (as often with Stof) and thus creates an exception with an imprecise message.

Where I would like to see that there was no configured methodCall for these arguments with the list of provided arguments, I have a message about type mismatch.
This means to correct the error I have to look at the arguments with a debugger for instance.

I wonder if it is good to authorise a call that must return a value to fail later in case of shouldHaveBeenCalled() because it is not the job of prophecy to know/guess what to return.

I think an option is to return the good type thanks to reflection. Another option would be to check if the type of return is not void and throw the exception immediately which would mean users could not expect a shouldHaveBeenCalled to work for function with a return value without defining the correct return value themselves. There may be a lot of other better options but I only thought of these for now.

Could you please tell me where I made mistakes in my understandings. And if I'm correct about the problem how should we proceed to make this better?

PS : @everzet I'm a regular user of Prophecy and I want to thank you for this piece of software.

[sstok]

I actually encountered this as well (and wanted to open an issue/pr about 😅 sorry).

A possible solution (suggested in Slack): In the case of a TypeError it would be possible to catch this and execute checkUnexpectedCalls so that at least reports why it failed?

[jeanlucc]

Hello,

Thanks for your reply and no need to be sorry. I opened the issue because I thought it was a problem but I indeed implemented a work around that replace the TypeError Exception with the Exception of checkUnexpectedCalls so that I have a clear message.

But this needs to be implemented by everyone with this need (I think it might not be so many people) thus I raised the issue to know if it was an option to implement it directly in prophecy.

If your suggestion is for an implementation in Prophecy I do not see right now how to do it in a clean way and the advantages over the options I provided. (I do not know the implementation of Prophecy very well so it may be why I cannot see it right now).

[ScreamingDev]

If I understood correctly, what you are trying to fulfill is an interface / return-type, which can already be done using Prophecy ...

use Prophecy\Argument;

$proph = $this->prophesize(Some::class);
$proph->THE_METHOD()->yourRules(); // ...
$proph->THE_METHOD()->withArguments([Argument::cetera()])->willReturn('some-correct-type-here');

This allows you to define a "default return" value (tested with version 1.10.3) so you can fulfill the return type there.
But it internally only reaches a score of 2 and gets overridden when you use multiple ::any() like this:

// imagine a function with two arguments

...->withArguments([ Argument::any(), Argument::any() ])->...
// Score = 3 + 3 = 6

...->withArguments([ Argument::any(), Argument::cetera() ])->...
// Score = 3 + 2 = 5

...->withArguments([ Argument::cetera() ])->...
// Score = 2

Resolving which "prophecy-rules" matches will look for the biggest score and follow its definition / return value.

And you need the ::cetera() when you define less arguments than your function has, because otherwise your rule won't match at all due to ...

https://github.com/phpspec/prophecy/blame/8ce87516be71aae9b956f81906aaf0338e0d8a2d/src/Prophecy/Argument/ArgumentsWildcard.php#L72
(::cetera() marks itself as isLast() = true and stops a few lines earlier)

Even though the scoring of ::cetera() is a bit low the whole system works very well IMHO.

[Jean85]

@ScreamingDev your solution is technically correct, but it's not feasible.
I was pointing out the same problem in #508 (referencing the same comment from Stof) and it's basically a DX (developer experience) issue: if I'm in the middle of writing a test, I may get something wrong, like have an Argument declaration that doesn't match exactly, and I would waste minutes tracking down the issue because I would get a fatal \TypeError instead of an unexpected call one.

The root of the issue is still the same: stubs and spies can have undefined behaviour, 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.

Bottom line, Prophecy's behaviour is unclear when the subject of the prophecy has return types declared on its methods.

We should intercept early when we would get a \TypeError leveraging reflection and either

1. bail out with a specific exception
2. try to guess a good return type

IMHO we should bail out, because trying to guess a returnable value is very tricky:

• for scalars you would have to return arbitrary values ([], '', 0, true or false?)
• for objects, you could still encounter classes which can't be prophesized (final, \Generator, \Throwable...)
• what about resource?

...and you would still need to have a bail out as a fallback. Also, the DX issue would still be there, because the code under test could start to behave in unpredictable ways, possibly delaying the test failure, leading to different unexpected calls down the line and derailing the user.

@ciaranmcnulty WDYT?

[stof]

even without return type in the mix, I still see an issue:
fir a non registered method call, Prophecy returns null (since #441), which might violate the phpdoc of the method and then be passed as an argument to another method forbidding null, which would also be a TypeError (on something totally unrelated to this prophecy double).

@ciaranmcnulty to me, #441 was a mistake. It creates more issues that what it solves IMO.

[ciaranmcnulty]

@stof I agree, the question is how to proceed without more disruption

[Jean85]

If we agree that reverting #441 is the way to go, it would certainly wreak havoc on users' test suites; many tests would start breaking just by changing that, and I'm not counting the fact that reverting #441 would force us to change a couple of other behaviours that were leveraging that.

IMHO we should lump that into a 2.0 release, so that the upgrade wouldn't be automatic for end users, and mark this as a breaking change.

[stof]

no idea. the issue with spies is that they are configured in the MethodProphecy after using the double. So we have an issue here.

[Jean85]

@stof do you think that something along the lines of #509 would be a solution for spies? Basically, you would have to enable (or disable, depending on which default we want) the "bail out" on hitting a non registered method call, so that you could still define spies without having the double blow up on usage.

[ciaranmcnulty]

Prophecy is 'opinionated' so I'd rather have a single behaviour, even if it loses some users.

[stof]

@ciaranmcnulty the current behavior is bad for anyone not using spies. Before #441, the UnexpectedCallException had a stack trace pointing where the unexpected call happened, making it dead easy to fix it. After #441, you have no helpful info.
I would say that Prophecy should have a way to opt-in into the spying rather than opting out (as it cannot guess that spying assertions will be used later).
Either that, or it should still require you to stub any method on which you want to spy so that their behavior is defined (which is basically what happened before #441).

[ciaranmcnulty]

I'm happy to go back to the world before #441, but what's the least disruptive way of doing that?

[stof]

The less disruptive way is a major version, so that projects have to opt-in for the behavior change.
#441 was about allowing to write tests in a way that was not supported before, and which is basically incompatible with any codebase not allowing null everywhere. Projects relying on that new way will need to be updated. All other projects will get back a much saner failure mode for broken code.

[Jean85]

I drafted #526 to this effect. It seems a pretty straightforward change; I would just take the occasion to bake in other breaking changes if needed.

[stof]

See #528 for the discussion about the solution