phpspec / prophecy

Highly opinionated mocking framework for PHP 5.3+
MIT License
8.53k stars 242 forks source link

Mocking PHP 8.2 readonly classes generates readonly double with untyped property and fails #586

Open vuryss opened 1 year ago

vuryss commented 1 year ago

When creating a double of a readonly class I ger the following error:

PHP Fatal error: Readonly property Double\Path\To\ClassName\P1::$objectProphecyClosure must have type in /var/www/html/vendor/phpspec/prophecy/src/Prophecy/Doubler/Generator/ClassCreator.php(49) : eval()'d code on line 3

This comes from eval of the generated code likes this:

namespace Double\Path\To\ClassName {
    readonly class P1 extends \Path\To\ClassName implements \Prophecy\Prophecy\ProphecySubjectInterface, \Prophecy\Doubler\Generator\ReflectionInterface {
        private $objectProphecyClosure;

        public  function __construct(...

Readonly classes make all class properties readonly. And readonly properties must have a type. So this generated property objectProphecyClosure fails that.

It's generated in ProphecySubjectPatch::apply

May be the double class doesn't need to be readonly or we can make the generated fields typed?

ciaranmcnulty commented 1 year ago

I'll be honest with you, I don't really remember how readonly and inheritance work together (and there's some readonly classes coming that'll be tricky to deal with)

If we can make the doubled properties be readonly too that's a good solution, otherwise prophecy should fail early and refuse to double the class with an appropriate message

The latter would be a good stopgap if nobody has time to look at this in detail

vuryss commented 1 year ago

You can use an object instead to hold stuff that has to be initialized and modified later. Even if the property referencing the object is readonly - it's own data can be mutable.

BladeMF commented 1 year ago

Same here. An argument that is set to Argument::exact(null) where the expected type is ?Token is producing a double inheriting Token, which I don't understand why is necessary. This works if the argument is not null, but an instance of the Token class.

UPDATE: Mine was caused by having a MethodProphecy on a method which returns Token, but has no specified return promise. Prophecy then tries to generate a default return value and fails.

NicolasJourdan commented 9 months ago

Any news about this problem ?

@vuryss How can you use an object in order to mock the readonly class ?

WalterWoshid commented 8 months ago

Bump, need this, too.

WalterWoshid commented 8 months ago

For anyone needing a temporary solution:

Use it like this:

class MyTest
{
    use ProphecyTrait;
    use ProphecyTraitReadonlyHack {
        ProphecyTraitReadonlyHack::prophesize insteadof ProphecyTrait;
    }

    // ...
}

Solution:

// ProphecyTraitReadonlyHack.php

use MyApp\Prophecy\Prophet;
use MyApp\Prophecy\ClassCodeGenerator;
use Prophecy\Doubler\Generator\ClassCodeGenerator as OriginalClassCodeGenerator;
use Prophecy\PhpUnit\ProphecyTrait;
use Prophecy\Prophecy\ObjectProphecy;

/**
 * Read-only classes throw an error when using Prophecy. This trait overrides
 * Prophecy's {@link OriginalClassCodeGenerator ClassCodeGenerator} with its own
 * {@link ClassCodeGenerator}, which fixes the issue.
 *
 * @mixin ProphecyTrait
 */
trait ProphecyTraitReadonlyHack
{
    private ?Prophet $modifiedProphet = null;

    protected function prophesize(?string $classOrInterface = null): ObjectProphecy
    {
        return $this->getModifiedProphet()->prophesize($classOrInterface);
    }

    private function getModifiedProphet(): Prophet
    {
        if (!$this->modifiedProphet) {
            $this->prophet = new Prophet();
        }

        return $this->prophet;
    }
}
// Prophet.php

use MyApp\Prophecy\ClassCodeGenerator;
use MyApp\Utils\ClassUtils;
use Prophecy\Doubler\Doubler;
use Prophecy\Doubler\Generator\ClassCreator;
use Prophecy\Prophecy\RevealerInterface;
use Prophecy\Prophet as OriginalProphet;
use Prophecy\Util\StringUtil;

class Prophet extends OriginalProphet
{
    public function __construct(
        Doubler $doubler = null,
        RevealerInterface $revealer = null,
        StringUtil $util = null,
    ) {
        parent::__construct($doubler, $revealer, $util);

        // ClassUtils = Reflection helper

        $doubler = ClassUtils::getPropertyValue($this, 'doubler');

        ClassUtils::setPropertyValue(
            $this,
            'creator',
            new ClassCreator(new ClassCodeGenerator()),
        );

        ClassUtils::setPropertyValue($this, 'doubler', $doubler);
    }
}
// ClassCodeGenerator.php

use Prophecy\Doubler\Generator\ClassCodeGenerator as OriginalClassCodeGenerator;
use Prophecy\Doubler\Generator\Node;

class ClassCodeGenerator extends OriginalClassCodeGenerator
{
    public function generate($classname, Node\ClassNode $class): string
    {
        $code = parent::generate($classname, $class);

        if (str_contains($code, 'readonly class')
            && str_contains($code, 'private $objectProphecyClosure;')
        ) {
            $code = str_replace(
                '->objectProphecyClosure',
                '->objectProphecyClosureContainer->closure',
                $code,
            );

            $code = str_replace(
                'private $objectProphecyClosure;',
                'private \MyApp\Prophecy\ObjectProphecyClosureContainer $objectProphecyClosureContainer;',
                $code,
            );

            // Replace: __construct(...) { ... }
            // With: __construct(...) { $this->objectProphecyClosure = new ObjectProphecyClosureContainer; ... }
            $code = preg_replace(
                '/__construct\([^)]*\) \{/',
                '__construct($1) { $this->objectProphecyClosureContainer = new \Bite\Test\App\TestBundle\Prophecy\ObjectProphecyClosureContainer; ',
                $code
            );
        }

        return $code;
    }
}
// ObjectProphecyClosureContainer.php

class ObjectProphecyClosureContainer
{
    public mixed $closure = null;
}
// ClassUtils.php

use ReflectionClass;
use ReflectionException;
use ReflectionProperty;

class ClassUtils
{
    public static function getPropertyValue(
        object $subject,
        string $propertyName,
    ): mixed {
        $property = self::findProperty($subject, $propertyName);

        return $property->getValue($subject);
    }

    public static function setPropertyValue(
        object $subject,
        string $propertyName,
        mixed $value,
    ): void {
        $property = self::findProperty($subject, $propertyName);

        $property->setValue($subject, $value);
    }

    private static function findProperty(
        object $subject,
        string $propertyName,
    ): ReflectionProperty {
        $class = new ReflectionClass($subject);
        $iterateClass = $class;

        while ($iterateClass) {
            if ($iterateClass->hasProperty($propertyName)) {
                return $iterateClass->getProperty($propertyName);
            }

            $iterateClass = $iterateClass->getParentClass();
        }

        throw new ReflectionException(sprintf(
            'Property %s::$%s does not exist',
            $class->getName(),
            $propertyName
        ));
    }
}
jdreesen commented 8 months ago

Why don't you create a PR if you have a solution?

WalterWoshid commented 8 months ago

My bad @jdreesen, I just realized how stupid my solution was. I didn't know you can't modify readonly properties, which makes it a lot harder.

WalterWoshid commented 8 months ago

Nevermind, I have found the solution, but this is only a hack and I don't have time to do it right and create a PR, because my company needs this.

For anyone needing a temporary solution:

Use it like this:

class MyTest
{
    use ProphecyTrait;
    use ProphecyTraitReadonlyHack {
        ProphecyTraitReadonlyHack::prophesize insteadof ProphecyTrait;
    }

    // ...
}

Solution:

// ProphecyTraitReadonlyHack.php

use MyApp\Prophecy\Prophet;
use MyApp\Prophecy\ClassCodeGenerator;
use Prophecy\Doubler\Generator\ClassCodeGenerator as OriginalClassCodeGenerator;
use Prophecy\PhpUnit\ProphecyTrait;
use Prophecy\Prophecy\ObjectProphecy;

/**
 * Read-only classes throw an error when using Prophecy. This trait overrides
 * Prophecy's {@link OriginalClassCodeGenerator ClassCodeGenerator} with its own
 * {@link ClassCodeGenerator}, which fixes the issue.
 *
 * @mixin ProphecyTrait
 */
trait ProphecyTraitReadonlyHack
{
    private ?Prophet $modifiedProphet = null;

    protected function prophesize(?string $classOrInterface = null): ObjectProphecy
    {
        return $this->getModifiedProphet()->prophesize($classOrInterface);
    }

    private function getModifiedProphet(): Prophet
    {
        if (!$this->modifiedProphet) {
            $this->prophet = new Prophet();
        }

        return $this->prophet;
    }
}
// Prophet.php

use MyApp\Prophecy\ClassCodeGenerator;
use MyApp\Utils\ClassUtils;
use Prophecy\Doubler\Doubler;
use Prophecy\Doubler\Generator\ClassCreator;
use Prophecy\Prophecy\RevealerInterface;
use Prophecy\Prophet as OriginalProphet;
use Prophecy\Util\StringUtil;

class Prophet extends OriginalProphet
{
    public function __construct(
        Doubler $doubler = null,
        RevealerInterface $revealer = null,
        StringUtil $util = null,
    ) {
        parent::__construct($doubler, $revealer, $util);

        // ClassUtils = Reflection helper

        $doubler = ClassUtils::getPropertyValue($this, 'doubler');

        ClassUtils::setPropertyValue(
            $this,
            'creator',
            new ClassCreator(new ClassCodeGenerator()),
        );

        ClassUtils::setPropertyValue($this, 'doubler', $doubler);
    }
}
// ClassCodeGenerator.php

use Prophecy\Doubler\Generator\ClassCodeGenerator as OriginalClassCodeGenerator;
use Prophecy\Doubler\Generator\Node;

class ClassCodeGenerator extends OriginalClassCodeGenerator
{
    public function generate($classname, Node\ClassNode $class): string
    {
        $code = parent::generate($classname, $class);

        if (str_contains($code, 'readonly class')
            && str_contains($code, 'private $objectProphecyClosure;')
        ) {
            $code = str_replace(
                '->objectProphecyClosure',
                '->objectProphecyClosureContainer->closure',
                $code,
            );

            $code = str_replace(
                'private $objectProphecyClosure;',
                'private \MyApp\Prophecy\ObjectProphecyClosureContainer $objectProphecyClosureContainer;',
                $code,
            );

            // Replace: __construct(...) { ... }
            // With: __construct(...) { $this->objectProphecyClosure = new ObjectProphecyClosureContainer; ... }
            $code = preg_replace(
                '/__construct\([^)]*\) \{/',
                '__construct($1) { $this->objectProphecyClosureContainer = new \Bite\Test\App\TestBundle\Prophecy\ObjectProphecyClosureContainer; ',
                $code
            );
        }

        return $code;
    }
}
// ObjectProphecyClosureContainer.php

class ObjectProphecyClosureContainer
{
    public mixed $closure = null;
}
// ClassUtils.php

use ReflectionClass;
use ReflectionException;
use ReflectionProperty;

class ClassUtils
{
    public static function getPropertyValue(
        object $subject,
        string $propertyName,
    ): mixed {
        $property = self::findProperty($subject, $propertyName);

        return $property->getValue($subject);
    }

    public static function setPropertyValue(
        object $subject,
        string $propertyName,
        mixed $value,
    ): void {
        $property = self::findProperty($subject, $propertyName);

        $property->setValue($subject, $value);
    }

    private static function findProperty(
        object $subject,
        string $propertyName,
    ): ReflectionProperty {
        $class = new ReflectionClass($subject);
        $iterateClass = $class;

        while ($iterateClass) {
            if ($iterateClass->hasProperty($propertyName)) {
                return $iterateClass->getProperty($propertyName);
            }

            $iterateClass = $iterateClass->getParentClass();
        }

        throw new ReflectionException(sprintf(
            'Property %s::$%s does not exist',
            $class->getName(),
            $propertyName
        ));
    }
}
Ryangr0 commented 7 months ago

Is anybody working on a real solution to this problem in a PR or something? I'd like to help contribute if possible. Just ran into this and I really don't want to use the hacky solution above. It makes me wonder about the support for newer versions of php. Readonly was introduced 3 years ago...

jgxvx commented 7 months ago

@Ryangr0 I'm just a user like you, but I think this issue is hard to solve. mockery/mockery#1317 is the same problem and they commented that it may ultimately not be possible to work around the restrictions.

On our team, we work around this by not marking entire classes as readonly if we intend to mock them, and just mark all their properties as readonly instead. Far from ideal, I know...

WalterWoshid commented 7 months ago

I have created a pull request https://github.com/phpspec/prophecy/pull/623

Ryangr0 commented 6 months ago

A testing framework definitely should not be dictating how you structure your code in this way. I hope I'm not coming over as hostile. Thanks for making the PR. Love to see open source work.

WalterWoshid commented 6 months ago

@Ryangr0 Fully agree with you here

musahaidarird commented 5 months ago

Hi! Thanks everyone for all the hard works :bow: And I do understand the busy life of developers who contribute to the open source projects.

Yet, I would like to have the courage to ask, when we could expect this to be merged and released, as the commit is from April. And our team is also waiting for this feature too.

Most probably there are some procedures to be followed, but I have very low knowledge on how features get their way to production in open source project, so my apologies in advance.

sfaujour commented 4 months ago

I don't have a solution either, but maybe it makes sense to look at the Code of PHPUnit, where there is also a createMock method, which CAN mock readonly classes. I am not familiar with the mockery code, so I can only make this contribution :/

MethorZ commented 3 months ago

Any ETA on when this topic can be merged so that we actually can use prophecies once again with latest PHP Code Bases?

Mariusz225 commented 2 months ago

Prophecy currently struggles with readonly properties, which can cause issues during mocking. Instead, mock the interface that does not use readonly properties and configure the return values of individual methods as needed.

WalterWoshid commented 2 months ago

@Mariusz225 I already created a fix for it (https://github.com/phpspec/prophecy/pull/623), but still waiting for it to get approved. It takes so long, that I guess that this project is as good as dead or not maintained anymore. The monkey-patch I posted above is based on that and works and is currently in use for my company.

Mariusz225 commented 2 months ago

Super :) Will this solve the problem with final class as well? The interface method might be helpful if someone wants to use native mocking with phpunit, because I noticed the same problem there.

WalterWoshid commented 2 months ago

@Mariusz225 I am not sure, I might have to test that.

stof commented 2 months ago

@Mariusz225 mocking final classes will never be supported, as by definition, the generated mock class has to extend the class you want to mock (so that it can pass type checks), and a final class cannot be extended. Hack supports mocking final class thanks to a language-level feature where a special attribute added on a class allows it to bypass the final keyword of the parent class, but PHP does not have such feature.

Ryangr0 commented 1 week ago

Mocking final classes in PHPUnit is supported, if you use https://github.com/dg/bypass-finals. And so is readonly. It works with prophecy as well.

Though I suppose you aren't actually mocking classes that are "still" final when the test is run, when using that solution. It doesn't matter to me. It fits my requirements perfectly: