Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
vuryss opened this issue Jan 25, 2023 · 14 comments · May be fixed by #623
Open

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

vuryss opened this issue Jan 25, 2023 · 14 comments · May be fixed by #623
Labels

Comments

@vuryss
Copy link

vuryss commented Jan 25, 2023

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?

@vuryss vuryss changed the title Mocking PHP 8.2 readonly classes generated readonly double with untyped property and fails Mocking PHP 8.2 readonly classes generates readonly double with untyped property and fails Jan 25, 2023
@ciaranmcnulty
Copy link
Member

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
Copy link
Author

vuryss commented Jan 25, 2023

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
Copy link

BladeMF commented Mar 24, 2023

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.

@stof stof added the PHP8.2 label Aug 22, 2023
@NicolasJourdan
Copy link

Any news about this problem ?

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

@WalterWoshid
Copy link

Bump, need this, too.

@WalterWoshid

This comment was marked as outdated.

@jdreesen
Copy link
Contributor

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

@WalterWoshid
Copy link

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
Copy link

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
Copy link

Ryangr0 commented Apr 25, 2024

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
Copy link

jgxvx commented Apr 25, 2024

@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 WalterWoshid linked a pull request Apr 25, 2024 that will close this issue
@WalterWoshid
Copy link

I have created a pull request #623

@Ryangr0
Copy link

Ryangr0 commented May 4, 2024

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
Copy link

@Ryangr0 Fully agree with you here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants