Skip to content

Remove not longer required stubs#309

Closed
alexander-schranz wants to merge 1 commit intoJan0707:masterfrom
alexander-schranz:feature/remove-not-longer-required-stubs
Closed

Remove not longer required stubs#309
alexander-schranz wants to merge 1 commit intoJan0707:masterfrom
alexander-schranz:feature/remove-not-longer-required-stubs

Conversation

@alexander-schranz
Copy link
Copy Markdown
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.

],
"require": {
"php": "^7.1 || ^8.0",
"php": "^7.2 || ^8.0",
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@alexander-schranz

Would you be interested in #285?

@alexander-schranz
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

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