Problem/Motivation

In order for our tests to run, we need to specify a minimum version of phpspec/prophecy. We rely on a bugfix introduced in 1.4: https://github.com/phpspec/prophecy/issues/139

See #2887000-57: composer.json does not constrain Symfony components to minor and patch versions that are compatible with Drupal

See comment #5 for analysis and solution.

Steps to repro:

Checkout 8.4.x branch of core.

Assume we've made the changes from #2887000: composer.json does not constrain Symfony components to minor and patch versions that are compatible with Drupal or apply the patch in #66.

$ composer update --prefer-lowest --prefer-dist
[ stuff happens]
$ composer show *prophecy
phpspec/prophecy v1.3.1 Highly opinionated mocking framework for PHP 5.3+
$ ./vendor/bin/phpunit -c core/ --testsuite unit

Somewhere about 75% the test run will fatal out. You'll see this in your log file:

PHP Fatal error:  Declaration of Double\FormStateInterface\P1510::getCompleteForm() must be compatible with & Drupal\Core\Form\FormStateInterface::getCompleteForm() in /Users/paul/pj2/drupal/vendor/phpspec/prophecy/src/Prophecy/Doubler/Generator/ClassCreator.php(49) : eval()'d code on line 2

This tells us that we have some kind of incompatibility with prophecy.

We increase the minimum requirement of phpspec/prophecy to ^1.4 and this error no longer happens, and tests finish normally.

Proposed resolution

Limit our minimum prophecy version to ^1.4

Remaining tasks

DrupalCI could catch these real actual constraints if we test them: #2874198: Create and run dependency regression tests for core

User interface changes

API changes

Data model changes

Problem/Motivation

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mpdonadio created an issue. See original summary.

Mile23’s picture

Issue summary: View changes
mpdonadio’s picture

OK, let's show the failing run to see if we get more info.

Status: Needs review » Needs work

The last submitted patch, 3: prophecy13-test-only.patch, failed testing. View results

Mile23’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
426 bytes

Running the tests manually using --testsuite unit, I eventually see this log info:

[10-Aug-2017 02:36:41 Australia/Sydney] PHP Fatal error:  Declaration of Double\FormStateInterface\P1510::getCompleteForm() must be compatible with & Drupal\Core\Form\FormStateInterface::getCompleteForm() in /Users/paul/pj2/drupal/vendor/phpspec/prophecy/src/Prophecy/Doubler/Generator/ClassCreator.php(49) : eval()'d code on line 2

[..]

[10-Aug-2017 02:36:41 Australia/Sydney] PHP  11. PHPUnit_Framework_TestCase->runBare() /Users/paul/pj2/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:612
[10-Aug-2017 02:36:41 Australia/Sydney] PHP  12. Drupal\Tests\Core\Form\FormStateDecoratorBaseTest->setUp() /Users/paul/pj2/drupal/vendor/phpunit/phpunit/src/Framework/TestCase.php:764
[10-Aug-2017 02:36:41 Australia/Sydney] PHP  13. Prophecy\Prophecy\ObjectProphecy->reveal() /Users/paul/pj2/drupal/core/tests/Drupal/Tests/Core/Form/FormStateDecoratorBaseTest.php:42
[10-Aug-2017 02:36:41 Australia/Sydney] PHP  14. Prophecy\Doubler\LazyDouble->getInstance() /Users/paul/pj2/drupal/vendor/phpspec/prophecy/src/Prophecy/Prophecy/ObjectProphecy.php:105

So that tells us that from Drupal perspective, the error comes about in FormStateDecoratorBaseTest::setUp(). Does that method do anything weird? No:

  /**
   * {@inheritdoc}
   */
  public function setUp() {
    parent::setUp();

    $this->decoratedFormState = $this->prophesize(FormStateInterface::class);

    $this->formStateDecoratorBase = new NonAbstractFormStateDecoratorBase($this->decoratedFormState->reveal());
  }

Since the error happens during setUp() and not during a test, we know that the problem has to do with prophecy not fully implementing FormStateInterface in some way, and that our test method is not mucking with the mock.

The interface method that's generating the error is getCompleteForm() which returns a reference to an array.

And behold: https://github.com/phpspec/prophecy/issues/139 which is titled "Fatal error when mocking reference returning interface methods"

We consult the changelog for 1.4.0: https://github.com/phpspec/prophecy/blob/v1.4.0/CHANGES.md And there it is: "Support for mocking classes with methods that return references (thanks @edsonmedina)"

So: We either need to change the test (and probably others) so it uses phpunit mocks, or we need to specify a minimum version for prophecy of 1.4. I vote for the latter.

The patch is a re-upload of the original one so it's last in line.

Mile23’s picture

Title: Specify minimum version in core/composer.json for prophecy » Specify minimum phpspec/prophecy version in core/composer.json
Issue summary: View changes
Mile23’s picture

Issue summary: View changes
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I totally agree on not changing the tests but rather rely on the version number for the fix.

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2900800_5.patch, failed testing. View results

Mile23’s picture

Status: Needs work » Needs review
FileSize
430 bytes

Reroll.

Mile23’s picture

Issue tags: +Composer
Anonymous’s picture

Status: Needs review » Reviewed & tested by the community

Straight reroll. Back to RTBC per #8.

  • effulgentsia committed 5f83362 on 8.5.x
    Issue #2900800 by Mile23, mpdonadio: Specify minimum phpspec/prophecy...

  • effulgentsia committed 32b6b10 on 8.4.x
    Issue #2900800 by Mile23, mpdonadio: Specify minimum phpspec/prophecy...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, this is great. Pushed to 8.5.x and cherry picked to 8.4.x.

effulgentsia’s picture

Issue tags: +8.4.0 release notes

Tagging for release notes just in case, but I don't know if we really need to mention it. The installed version in our composer.lock file is 1.7.0, so this issue does not change what someone gets from doing a composer install, it only changes what someone would get from doing a composer update --prefer-lowest. And it's also only a require-dev dependency.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.