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 · 23 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 · 23 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

@musahaidarird
Copy link

Hi! Thanks everyone for all the hard works 🙇 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
Copy link

sfaujour commented Jul 5, 2024

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

MethorZ commented Aug 20, 2024

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

@Mariusz225
Copy link

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

@Mariusz225 I already created a fix for it (#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
Copy link

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

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

@stof
Copy link
Member

stof commented Sep 16, 2024

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

Ryangr0 commented Nov 14, 2024

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:

  • I don't need to change my actual code
  • I'm still able to use prophecy

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.