phpspec / prophecy

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

Interface inheritance can cause unevaluatable doubles #192

Open ciaranmcnulty opened 9 years ago

ciaranmcnulty commented 9 years ago

Reproducible case:

interface Foo{}
interface Bar extends Foo{}

// works fine and generates a double
(new Prophet())->prophesize()
  ->willImplement(Bar::class)
  ->willImplement(Foo::class)
  ->reveal();

// fatal error 'cannot implement previously implemented interface'
(new Prophet())->prophesize()
  ->willImplement(Foo::class)
  ->willImplement(Bar::class)
  ->reveal();

Root cause is that the following code is trying to be eval()ed:

class P1 extends \stdClass implements \Prophecy\Prophecy\ProphecySubjectInterface, \Prophecy\Doubler\Generator\ReflectionInterface, \spec\PhpSpec\Bar, \spec\PhpSpec\Foo {
 // ...
}

Which triggers this PHP bug (or rather, 'documented behaviour'): https://bugs.php.net/bug.php?id=63816

stof commented 9 years ago

hmm, we should indeed sort interfaces when generating the code to avoid this issue. Do you want to work on a fix ?

ciaranmcnulty commented 9 years ago

I might, I have a short list of PhpSpec things to do but if I can see a fix I'll implement,

I triggered this by the following typo in PhpSpec:

function let(Interface $double) {...}

function it_does_something(MoreSpecificInterface $double) { ... }

The matching argument names actually makes the double implement both interfaces, which might also be an error case.

ciaranmcnulty commented 9 years ago

@stof The sorting algorithm is non-trivial I suspect.

stof commented 9 years ago

@ciaranmcnulty parent before children (and any order for non-related interfaces).

You can see https://github.com/symfony/symfony/blob/4133aadd0c0be199b21ce5e171038e5d03bc818b/src/Symfony/Component/ClassLoader/ClassCollectionLoader.php#L242 for some code doing such sorting in a much more complex case (it needs to take parent classes and traits into account as well, and collect them to add them in the list if they were not there yet, which is not necessary here)

ciaranmcnulty commented 9 years ago

You'd also need to take into account what interfaces the classes implement, I think?

The other issue I'd have is lack of unit tests - I guess that's something you'd be open to me introducing to the projects?

stof commented 9 years ago

lack of unit tests ? Couldn't this be covered by the specs of the classes we already have ?

ciaranmcnulty commented 9 years ago

Ignore me, I was mixing it up with another project (behat) :-)