Method with a class argument and default value can't be mocked

[bicpi]

Hi,
since 8.1, PHP supports the usage of the new operator in initializers (https://php.watch/rfcs/new_in_initializers).

Given this dummy class:

class DummyClass {}

And an interface with a method using a DummyClass instance as default argument of a method:

interface BarService
{
    public function doIt(DummyClass $myClass = new DummyClass()): void;
}

And a service using the interface as a dependency:

class FooService
{
    public function __construct(
        private readonly BarService $bar
    ) {
    }

    public function execute(): void
    {
        $this->bar->doIt();
    }
}

The following test fails with a fatal PHP error in the phpspec/prophecy code:

namespace Tests;

use PHPUnit\Framework\TestCase;
use Prophecy\PhpUnit\ProphecyTrait;

class DummyClass {...}
interface BarService {...}
class FooService {...}

class FooServiceTest extends TestCase
{
    use ProphecyTrait;

    /**
     * @test
     */
    public function it_fails(): void
    {
        $someService = $this->prophesize(BarService::class);
        $someService
            ->doIt()
            ->shouldBeCalled();

        $fooService = new FooService($someService->reveal());
        $fooService->execute();
    }
}

The error message is:

PHP Fatal error:  Constant expression contains invalid operations in /vendor/phpspec/prophecy/src/Prophecy/Doubler/Generator/ClassCreator.php(49) : eval()'d code on line 5

[Jean85]

I'm trying to debug this issue, and I cannot find any way to replicate the original behavior in the mock. The reflection gives you access only to getDefaultValue(), which returns the instantiated object, so it's impossible to produce written code that would get us the intended result. As of now, the generator uses var_export on it:

prophecy/src/Prophecy/Doubler/Generator/ClassCodeGenerator.php

Lines 112 to 114 in d4f454f

	if ($argument->isOptional() && !$argument->isVariadic()) {
	$php .= ' = '.var_export($argument->getDefault(), true);
	}

so this means calling Class::__set_state(array(...)), which generates the fatal. IMHO we should work around it for now and produce a null as a default value in place of the object.

[stof]

Producing a null as default value is also broken, as it would change the method signature by making the argument nullable.

[Jean85]

I proposed my change in #615

Producing a null as default value is also broken, as it would change the method signature by making the argument nullable.

Mh no, you can't still pass null there, you have to explicitly declare it: https://3v4l.org/p2vdM
It's an argument for a mocked method, so the only impact that I can think of is if you use willReturnArgument().

[stof]

@Jean85 the mock class being used in the test won't have the signature used in your code snippet. Your PR produces this code: https://3v4l.org/hNgNI

[Jean85]

Oh right my mistake! Would a manual throw (of a TypeError?) in the method body be an acceptable workaround?

[stof]

symfony/var-exporter implemented some magic to export the signature of a ReflectionFunctionAbstract: https://github.com/symfony/var-exporter/blob/345c62fefe92243c3a06fc0cc65f2ec1a47e0764/ProxyHelper.php#L336

We won't be able to reuse their ProxyHelper directly in the code generator due to the layer of abstraction between the Reflection API and the Prophecy class representation (see the ClassMirror), but we might copy this logic as part of a support in our architecture.

[Jean85]

The only effect that the ClassMirror has in this use case is for patches, as far as I can see: https://github.com/phpspec/prophecy/tree/master/src/Prophecy/Doubler/ClassPatch

IMO copying that code would be dangerous, is pretty obscure and hard to maintain; being able to use exportSignature would be great, granted that we would have to reimplement the patches logic to fit into a class that extends \ReflectionFunctionAbstract.

Also, that method is present only since 6.2... It doesn't have dependencies, but it requires PHP 8.1 :(

[stof]

we would have to reimplement the patches logic to fit into a class that extends \ReflectionFunctionAbstract.

Currently, the ClassMirror system is built so that the representation of classes is decoupled from Reflection. Patches could potentially introduce new methods if they want.
Creating our own classes extending \ReflectionFunctionAbstract to reuse the ProxyHelper directly would not help much: we would have to ensure that our own classes behave the same than native Reflection classes (and ProxyHelper relies on parsing the string representation of the ReflectionParameter because there is no other way to access the instantiation AST for the default value).

is pretty obscure and hard to maintain

This is because PHP Reflection does not provide any dedicated way of accessing this info