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

Remove not longer required stubs #309

Conversation

alexander-schranz
Copy link
Collaborator

This pull request replaces #298 which did replacer @template with @template-covariant

Since Prophecy 1.17.0 the template are correctly added to prophecy itself and so the stubs are not longer required. Thx to @stof.

@@ -10,18 +10,18 @@
}
],
"require": {
"php": "^7.1 || ^8.0",
"php": "^7.2 || ^8.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the stubs are added in 1.17.0 which only supports version from ^7.2 so we not longer can test on 7.1

@localheinz
Copy link
Contributor

@alexander-schranz

Would you be interested in #285?

@alexander-schranz
Copy link
Collaborator Author

@localheinz Sure it looks like it is not a lot of work todo here and I'm if some issue appear it looks like I'm already the first one stumble over it as I use prophecy with the plugin in most projects.

"phpspec/prophecy": "^1.7.0",
"phpunit/phpunit": "^6.0.0 || ^7.0.0 || ^8.0.0 || ^9.0.0"
"phpspec/prophecy": "^1.17.0",
"phpunit/phpunit": "^8.0.0 || ^9.0.0"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prophecy 1.17 does only support PHPUnit 8, 9 (10 soon). So we can not longer test against 6 and 7.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpunit 10 is now supported

@localheinz localheinz assigned localheinz and unassigned localheinz Feb 23, 2024
@alexander-schranz alexander-schranz force-pushed the feature/remove-not-longer-required-stubs branch from edd0cac to b08af9e Compare March 13, 2024 08:24
@alexander-schranz alexander-schranz force-pushed the feature/remove-not-longer-required-stubs branch from b08af9e to 11a2a9c Compare March 13, 2024 08:25
Copy link
Contributor

@stof stof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually wondering whether this extension is still needed at all. Prophecy also has generic signatures for prophesize() methods

"phpspec/prophecy": "^1.7.0",
"phpunit/phpunit": "^6.0.0 || ^7.0.0 || ^8.0.0 || ^9.0.0"
"phpspec/prophecy": "^1.17.0",
"phpunit/phpunit": "^8.0.0 || ^9.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

phpunit 10 is now supported

@jseparovic1
Copy link
Contributor

jseparovic1 commented Mar 13, 2024

I'm actually wondering whether this extension is still needed at all. Prophecy also has generic signatures for prophesize() methods

I verified this and there is it still doesn't work without the extension (PHP 8.2, PhpUnit 10, phpstan 1.10).

Test case:

final class SimpleProphecyTestCase extends TestCase
{
    use ProphecyTrait;

    public function testCreatedViaTrait(): void
    {
        $foo = $this->prophesize(Foo::class);
        $foo->foo()->willReturn('foo');
        $foo = $foo->reveal();

        self::assertSame('foo', $foo->foo());
    }

     public function testCreatedInline(): void
    {
        $foo = (new Prophet())->prophesize(Foo::class);
        $foo->foo()->willReturn('foo');
        $foo = $foo->reveal();

        self::assertSame('foo', $foo->foo());
    }
}
class Foo
{
    public function foo(): string
    {
        return 'foo';
    }
}

Output:

 ------ -----------------------------------------------------------------------------------
  Line   SimpleProphecyTestCase.php
 ------ -----------------------------------------------------------------------------------
  20     Call to an undefined method Prophecy\Prophecy\ObjectProphecy::foo().
  27     Call to an undefined method Prophecy\Prophecy\ObjectProphecy<AppTest\Foo>::foo().
 ------ -----------------------------------------------------------------------------------

To make dynamic return types working prophesize method should return T instead of ObjectProphecy<T> but this won't be technically correct.

Note that method in ProphecyTrait does not specify generic types 🤔

It seems this extension is still required.

@InvisibleSmiley
Copy link

I'm actually wondering whether this extension is still needed at all. Prophecy also has generic signatures for prophesize() methods

Two answers to this question.

  1. When using the ProphecyTrait ($this->prophesize) instead of (new Prophet())->prophesize, the Prophecy generics are not used because the prophesize method override does not include them.
  2. Even when the ProphecyTrait override PHPDoc is amended or the Prophecy method is used directly, without this extension here I get lots of PHPStan errors like Call to an undefined method Prophecy\Prophecy\ObjectProphecy<SomeClass>::someMethod() in cases where I create a prophecy object and then use like $prophecy->someMethod()->willReturn('something');

@stof
Copy link
Contributor

stof commented Mar 13, 2024

ah indeed. I forgot the part covering the magic calls.

But I think WillExtendOrImplementDynamicReturnTypeExtension and ProphesizeDynamicReturnTypeExtension are not needed anymore.

Note that method in ProphecyTrait does not specify generic types

this should be solved by doing a PR on the prophecy-phpunit repo IMO.

@InvisibleSmiley
Copy link

Note that method in ProphecyTrait does not specify generic types

this should be solved by doing a PR on the prophecy-phpunit repo IMO.

Obviously. :)

@stof
Copy link
Contributor

stof commented Mar 13, 2024

And the TypeNodeResolverExtension should probably be deprecated because it is about transforming types like @var ObjectProphecy|Foo into ObjectProphecy<Foo> but there is no valid reason to write ObjectProphecy|Foo in phpdoc anymore as a hack (especially given that phpstan will complain about the missing generic type for ObjectProphecy)

@stof
Copy link
Contributor

stof commented Mar 13, 2024

Actually, I just did the PR for the ProphecyTrait myself: phpspec/prophecy-phpunit#63

@alexander-schranz alexander-schranz marked this pull request as draft March 13, 2024 11:15
@sasezaki sasezaki mentioned this pull request Nov 15, 2024
3 tasks
@alexander-schranz
Copy link
Collaborator Author

close in favor of #348 where we removed the not longer required stubs

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

Successfully merging this pull request may close these issues.

5 participants