phpspec / prophecy

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

PHP 8 | 'static' is an invalid class name #527

Closed AlexandruGG closed 2 years ago

AlexandruGG commented 3 years ago

Hello, I'm seeing a fatal error trying to use Prophecy with PHPUnit in a Symfony 5 project running PHP 8 with the new static return type. Here is the error:

PHP Fatal error:  '\static' is an invalid class name in /Users/alex/dev/project/vendor/bin/.phpunit/phpunit-9.5-0/vendor/phpspec/prophecy/src/Prophecy/Doubler/Generator/ClassCreator.php(49) : eval()'d code on line 11

And a simplified version of the Entity I'm trying to create a prophecy for:

class MyEntity {
    /**
     * @ORM\Column(name="id", type="integer", nullable=false, options={"unsigned"=true})
     * @ORM\Id
     * @ORM\GeneratedValue(strategy="IDENTITY")
     */
    private int $id;

    /**
     * @ORM\Column(name="name", type="string", length=50, nullable=false)
     */
    private string $name;

    public function getId(): int
    {
        return $this->id;
    }

    public function getName(): string
    {
        return $this->name;
    }

    public function setName(string $name): static // <--- if I replace this with 'self' it works
    {
        $this->name = $name;

        return $this;
    }
}

Is this a known issue with PHP 8? Apologies if so but I couldn't find any mention of it.

Or am I doing something wrong with this usage?

Thanks

stof commented 3 years ago

Support for this new return type has not been added yet in Prophecy.

However, I'm not quite sure how to implement that properly regarding the value actually returned by the double (when Prophecy guesses the returned value because it is not set explicitly).

ciaranmcnulty commented 3 years ago

I think it only needs to be handled the same way void is here: https://github.com/phpspec/prophecy/blob/master/src/Prophecy/Doubler/Generator/Node/ReturnTypeNode.php#L11-L13

There's no need to resolve the class, as this is valid:


class A {
    function foo() : static {}
}
class B extends A {
    function foo() : static {}
}
ciaranmcnulty commented 3 years ago

@AlexandruGG do you want to make this change? We'd add some tests too https://github.com/phpspec/prophecy/blob/master/spec/Prophecy/Doubler/Generator/Node/ReturnTypeNodeSpec.php#L108-L120

ciaranmcnulty commented 3 years ago

Oh hm you're right @stof that is tricky

stof commented 3 years ago

@ciaranmcnulty generating the right signature is easy. But generating the right behavior is not.

My own opinion is that Prophecy should drop all that logic, and expect all calls to be explicitly configured (which solves that case for static too, as it is then the responsibility of the dev to specify either ->willReturnSelf() or another valid return value returning another double instance). #526 is already bringing back the immediate UnexpectedCallException. My suggestion would then only mean removing the special case of an ObjectProphecy without anything configured in it. Today, it accepts any call (which stops as soon as you configure one). In my proposal, it would expect having no calls being done on it. I think that this is much less surprising, as Prophecy cannot properly determine what the call should return anyway.

ciaranmcnulty commented 3 years ago

In the short term (+ in prophecy 1.x) we can have a special case for static and throw an UnexpectedCallException maybe

RobinHoutevelts commented 3 years ago

I'm hitting this as well..

Any workaround besides dropping the use of the static return type in my code?

AlexandruGG commented 3 years ago

I'm hitting this as well..

Any workaround besides dropping the use of the static return type in my code?

@RobinHoutevelts this is the workaround I'm using for now. Works with static analysers like PHPStan and Psalm:



/**
* @return static
*/
public function setName(string $name): self
    {
        $this->name = $name;

        return $this;
    }
RobinHoutevelts commented 3 years ago

https://github.com/irontec/prophecy/commit/2d17db5545a52cbfdcddcb7fed0952d8d2dc086f kinda did it for me as wel.

Not sure what impact it has but at least my tests are green.

Thanks for the help!

alsar commented 3 years ago

The fix from irontec@2d17db5 works. Could this be merged?

stof commented 3 years ago

Well, nobody has submitted a PR with a fix yet

ciaranmcnulty commented 3 years ago

Yeah we don't scour forks for fixes; if someone can open a PR and write a test to go with it we can consider merging

dzianis-hrynko commented 3 years ago

Here is fix of the issue I've checked on my project https://github.com/phpspec/prophecy/pull/545

stof commented 2 years ago

Fixed in https://github.com/phpspec/prophecy/pull/545