Allow PHPUnit 6+ support for object mocking

Sadly createMock doesn't exist in PHPUnit 4.8, so we would have to convert them all to use getMockBuilder or just go with prophecy directly.

For me its not a nice movement that phpunit has such a high PHP dependency. This causes problems for people out there which have some more restrictive version constrains, as you see ...

The only possible issue is people using PHP 5.5.9 which is beyond end of life.

Well, its the version supported by most linux distros out there.

#2524432: Suggest PHP 5.6 as the recommended version and linked issues are related to this topic.

Simply regex replace all existing getMock() into createMock(), for both 8.3.x and 8.4.x.

In between, since composer update in 8.4.x is now broken due to symfony 3.3.x released (see #2874909: Update Symfony components to 3.3.*), so here also give an additional change to all symfony/* and JUST depend on 3.2.x, statically.

Another version that without changes to composer.lock, for distribution (e.g. https://github.com/drustack/drustack-standard) to apply patches.

Individual patches create for drupal/core.

Only change $this->getMock() into $this->createMock().

Revoke #14 patches via latest dev

Recreate drupal/core patches via https://github.com/drupal/core

We obviously can't do this kind of change as long we require php 5.5 as lowest version. Here is an alternative suggestion, we could make "createMock" available in our version of phpunit, convert all of the cases ...

What are your thoughts about that?

The minimal dependency on PHP >=5.5.9 for Drupal 8.x looks a bit crazy from today point of view, especial as PHP 5.5.x already EOL on 2016-07-21 which was one years ago...

Even PHP 7.0.x will go into Security Update Only since 2017-12-03, which maybe just 1 - 2 months after initial scheduled Drupal 8.4.x released on October...

Moreover, when I am running with composer update on Drupal 8.4.x, Doctrine Annotations will be upgraded to 1.5.0 with our current composer.json, which also depends on PHP 7.1.x...

IMHO, as mentioned on original request of this issue:

Drupal 8 should fully support PHP 7. PHP 7 includes the ability to type hint primitives, and explicitly declare the return type of methods:

public function foo(string $bar) : string {
  // Implementation here.
}

Therefore upgrading our Drupal minimal PHP requirement to 7.1.x and so using PHPUnit 5.x (or even PHPUnit 6.x for the best PHP 7.x support and functionality) whenever our next major/minor release on coming October should be a good idea for answering #22.

The minimal dependency on PHP >=5.5.9 for Drupal 8.x looks a bit crazy from today point of view, especial as PHP 5.5.x already EOL on 2016-07-21 which was one years ago...

Even PHP 7.0.x will go into Security Update Only since 2017-12-03, which maybe just 1 - 2 months after initial scheduled Drupal 8.4.x released on October...

Well, I simply believe this doesn't matter here :) We have dedicated discussions for that, see #2670966: Warn users of old PHP versions. Just complaining here isn't necessarily constructive.

Moreover, when I am running with composer update on Drupal 8.4.x, Doctrine Annotations will be upgraded to 1.5.0 with our current composer.json, which also depends on PHP 7.1.x...

Indeed, see #2898119: Doctrine/Common 2.8 requires PHP ~7.1

Making big changes isn't easy.

I'm sorry, I just realized I haven't uploaded the full patch before.

Regarding the bulk of the coding standard fails ...

We are at the interface between two coding standards. So I have no problem with the phpcs errors with regard to method arguments
as they are a clear copy of something from another project.

I am ONLY correcting the remaining little coding standard err.

I agree with dawehner's train of through line from #25 ...

Speaking for myself.. I regard any project not using php7 as broken.. but I want to solve our phpunit issue now.

I would like to see this issue get resolved as soon as possible.

Creating a forward compatibility layer sounds like a good idea here. It will allow us to get existing tests ready before we move to PHPUnit 5.

Updated the trait to have a proper docblock and added some more explanation to the trait. Also converted one instance of getMock to createMock as a proof of concept/test case.

The IS needs an update to reflect the current fix. And we might want to do a follow up to convert getMock to createMock everywhere.

Bleh

+++ b/core/tests/Drupal/Tests/Phpunit5FCTrait.php
@@ -0,0 +1,50 @@
+   *
+   * @param string $originalClassName
+   *   Name of the class to mock.
+   * @param array|null $methods
+   *   When provided, only methods whose names are in the array are replaced
+   *   with a configurable test double. The behavior of the other methods is not
+   *   changed. Providing null means that no methods will be replaced.
+   * @param array $arguments
+   *   Parameters to pass to the original class' constructor.
+   * @param string $mockClassName
+   *   Class name for the generated test double class.
+   * @param bool $callOriginalConstructor
+   *   Can be used to disable the call to the original class' constructor.
+   * @param bool $callOriginalClone
+   *   Can be used to disable the call to the original class' clone constructor.
+   * @param bool $callAutoload
+   *   Can be used to disable __autoload() during the generation of the test double class.
+   * @param bool $cloneArguments
+   * @param bool $callOriginalMethods
+   * @param object $proxyTarget
+   *
+   * @return PHPUnit_Framework_MockObject_MockObject

Let's maybe put an @see on here, so its clear that this is just a wrapper for another function?

+++ b/core/tests/Drupal/Tests/Phpunit5FCTrait.php
@@ -0,0 +1,50 @@
+ * This trait makes tests forward compatible with PHPUnit 5 and above.
+ *
+ * The getMock method is deprecated in PHPUnit 5 but the createMock method is
+ * not available in PHPUnit 4. To provide forward compatibility this trait
+ * provides the createMock method and uses getMock if this method is not
+ * available on the parent class.
+ */
+trait Phpunit5FCTrait {

I could imagine it would be helpful to link from here to the official phpunit docs ... sadly they have splitted up the docs into one changelog per release.

Moved the explanation from the trait docblock to the method docblock since we might want to add more method to this trait in the future and added the @see

Also had a thought, that it would make more sense to wrap getMock and not createMock, this way all existing tests don't need to be updated to be compatible with PHPUnit 5, they just are. And then we can clean them up at our convenience. Or will this not work?
Took out the test case because all test will now run through the Trait.

Don't you want to be able to call to createMock in phpunit 4 environments?

As discussed on Slack:
lendude [10:50 AM]
maybe we want to do both, getMock wrapper for BC, createMock wrapper for FC?
dawehner [10:50 AM]
@lendude Oh yeah that makes sense!

so something like this?

1. +++ b/core/tests/Drupal/Tests/Phpunit5FCTrait.php
   @@ -0,0 +1,93 @@
   +   *
   +   * @see \PHPUnit_Framework_TestCase::getMock
   +   *
   +   * @return PHPUnit_Framework_MockObject_MockObject
   +   */
   +  public function getMock($originalClassName, $methods = array(), array $arguments = array(), $mockClassName = '', $callOriginalConstructor = true, $callOriginalClone = true, $callAutoload = true, $cloneArguments = false, $callOriginalMethods = false, $proxyTarget = null) {
   

   Should we deprecate getMock here already and point to createMock?

2. +++ b/core/tests/Drupal/Tests/UnitTestCase.php
   @@ -18,6 +18,8 @@
    
   +  use Phpunit5FCTrait;
   +
   

   Maybe its also useful in the kernel test ... what do you think?

#35.1 yeah was thinking about that too, lets do it, added a CR for that https://www.drupal.org/node/2907725
#35.2 Less mocking being done there, but yeah we should add it there too.

Delayed action on #31, added a @see to the phpunit release announcement https://github.com/sebastianbergmann/phpunit/wiki/Release-Announcement-f... so people can see why this is needed at all.

Nice! I guess this patch will fail due to the deprecation error message. I fear we can't trigger the error for now, given that so many contrib/custom tests will break.

Yeah ok, the trigger_error was excessive. Took it out, and put the test case for createMock back in.

+++ b/core/tests/Drupal/Tests/Phpunit5FCTrait.php
@@ -0,0 +1,99 @@
+  public function createMock($originalClassName, $methods = array(), array $arguments = array(), $mockClassName = '', $callOriginalConstructor = true, $callOriginalClone = true, $callAutoload = true, $cloneArguments = false, $callOriginalMethods = false, $proxyTarget = null) {

Can we create a dedicated test so we can test this trait. Maybe we want to add more later?

Corrected coding standards errors.

I like tests.
And I also like to use PHPUnit 5 in my testsuite.

Attached some changes:
* Test for PHPUnit 5 is changed to test for the parent class and not the literal phpunit class.
* invoke the createMock on the parent instead of calling the method statically.
* Added tests.

Thank you @bircher!

+++ b/core/tests/Drupal/Tests/Phpunit5FCTrait.php
@@ -89,10 +84,25 @@ public function getMock($originalClassName, $methods = array(), array $arguments
+  private function isPhpunit5() {
+    $parent = get_parent_class($this);
+    // Loop over the parents to find the parent of where the trait is used.
+    while (method_exists($parent, 'isPhpunit5')) {
+      $parent = get_parent_class($parent);
+    }
+    // Finally we are not checking the method defined in this trait.
+    return method_exists($parent, 'createMock');
+  }

I think we should add some documentation why this is needed: Subclassing of test classes

rerolled the patch with more comments.

no magic...

One nice thing we could do is to upload a test patch which updates phpunit to 5.x and then run the testbot against that version. With that we would kinda prove that the trait actually works as expected.

good idea!
I was testing it in my contrib project development environment and it worked there.
I am not sure I didn't just break all sorts of things with the attached patch.

LOL, I'd just rolled this too, lets see if mine sticks.

checkForUnintentionallyCoveredCode has been deprecated in PHPUnit 5, see https://github.com/sebastianbergmann/phpunit/wiki/Release-Announcement-f...

So that needs to be changed to beStrictAboutCoversAnnotation apparently. Any way to do that in a patch? No idea how the bot builds its phpunit.xml

Reroll patch from #54, remove changes to composer.lock, for distribution (e.g. https://github.com/drustack/drustack-standard) to apply patches.

Well I think #54 proves that our BC/FC layer works, none of the fails there have anything to do with that. It just means we need to do some additional work when we actually go to PHPUnit 5, but that's fine and out of scope here.

So I'd say #49 is the patch that needs review, reupping @birchers patch here to have the latest patch in the issue be the one that is under review.

Reroll patch from #49, also update composer.json with "phpunit/phpunit": "^5.0"

Reroll patch from #49, also update composer.json with "phpunit/phpunit": "^5.0"

Note: The point of this issue is NOT to update phpunit itself. Phpunit 5 requires PHP7, so we can't use that at this given point in time.

PHPUnit 5 requires PHP 5.6, not 7.

Also: #2927806: Use PHPUnit 6 for testing when PHP version >= 7.2

But core declares PHP 5.5 compatibility https://github.com/sebastianbergmann/phpunit/wiki/Release-Announcement-f...

@andypost: Yes, but we should be as close as possible to dropping PHP 5.5 at any moment. As @dawehner points out, this issue does not change PHPUnit versions.

I think this becomes critical in light of #2927806: Use PHPUnit 6 for testing when PHP version >= 7.2

Just a reroll of the right patch :)

Reroll looks good, this Iooks ready to me.

Another little one to help us on the PHPUnit upgrade path - #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes

1. +++ b/core/tests/Drupal/Tests/Phpunit5FCTrait.php
   @@ -0,0 +1,114 @@
   +  public function getMock($originalClassName, $methods = array(), array $arguments = array(), $mockClassName = '', $callOriginalConstructor = TRUE, $callOriginalClone = TRUE, $callAutoload = TRUE, $cloneArguments = FALSE, $callOriginalMethods = FALSE, $proxyTarget = NULL) {
   +    return $this->createMock($originalClassName, $methods, $arguments, $mockClassName, $callOriginalConstructor, $callOriginalClone, $callAutoload, $cloneArguments, $callOriginalMethods, $proxyTarget);
   +  }
   

   This should trigger a silenced deprecated error.

2. +++ b/core/tests/Drupal/Tests/Phpunit5FCTrait.php
   @@ -0,0 +1,114 @@
   +    while (method_exists($parent, 'isPhpunit5')) {
   +      $parent = get_parent_class($parent);
   +    }
   

   This is not covered by the tests.

The approach looks sane and will help people move tests to whichever PHPUnit version we end up on.

I'm going to roll the patch from #67 and also merge in #2928249: Introduce a PHPUnit 6+ compatibility layer for Drupal\Tests\Listeners classes which will also get us ready for PHPUnit 6 as this issue is about 5+

On looking at all the work to make the listeners 6+ compatible I decided against rolling it into here. Although to get PHPUnit truly 7.2 compatible we need to do that I think might be better to have narrower scoped issues.

Addressed #67 - I made the loop a little bit easier to understand as well and have added tests that fail when the loop is removed.

Hmmm the deprecation is not going to work unless we put it in the skipped deprecations since PHPUnit calls it itself in \PHPUnit_Framework_MockObject_MockBuilder...

    /**
     * Creates a mock object using a fluent interface.
     *
     * @return PHPUnit_Framework_MockObject_MockObject
     */
    public function getMock()
    {
        return $this->testCase->getMock(
            $this->type,
            $this->methods,
            $this->constructorArgs,
            $this->mockClassName,
            $this->originalConstructor,
            $this->originalClone,
            $this->autoload,
            $this->cloneArguments,
            $this->callOriginalMethods,
            $this->proxyTarget
        );
    }

Hmmm and skipping doesn't work either because that logic only fires for KernelTestBase/WebTestBase/BrowserTestBase. So fire the deprecation here but instead open a follow issue to fire a deprecation once core and it's use of PHPUnit does not use it. At least we tried :)

+++ b/core/tests/Drupal/Tests/Phpunit5FCTrait.php
@@ -0,0 +1,110 @@
+    while (in_array(__TRAIT__, class_uses($parent), TRUE)) {
+      $parent = get_parent_class($parent);
+    }

This change was incorrect too. Because class_uses only return what the current class uses - not it and its parents. So I've reverted the code to #65. I've tried to improve the comment to be clearer about what is actually happening here.

@alexpott thanks for taking a look at this. The added test coverage and the updated comment address your feedback, so back to RTBC.

Added the purposefully not setting of the trigger_error() to the IS.

@alexpott in #2927806-33: Use PHPUnit 6 for testing when PHP version >= 7.2 is using a combined patch including the one here, and he spotted a bug. Since we are targetting a PHPUnit 6 compatibility layer there I think this needs to wait until the various pieces of the puzzle come together

@mondrake yep you're right...

    /**
     * Returns a test double for the specified class.
     *
     * @param string $originalClassName
     *
     * @return MockObject
     *
     * @throws Exception
     */
    protected function createMock($originalClassName)
    {
        return $this->getMockBuilder($originalClassName)
                    ->disableOriginalConstructor()
                    ->disableOriginalClone()
                    ->disableArgumentCloning()
                    ->disallowMockingUnknownTypes()
                    ->getMock();
    }

This is the createMock in PHPUnit 6 so we're going to need to do something a bit more complex with all the args to support that and use getMockBuilder() too.

So we need a separate code path for PHPUnit 5 and 6?

So here's a version that copes with the PHPUnit 6 changes to createMock and has been tested with it - see #2927806: Use PHPUnit 6 for testing when PHP version >= 7.2

@dawehner tbh I think we just skip PHPUnit 5 altogether.

Just a note if we skip PHPUnit 5 altogether -

phpunit.xml.dist has

<phpunit bootstrap="tests/bootstrap.php" colors="true"
         beStrictAboutTestsThatDoNotTestAnything="true"
         beStrictAboutOutputDuringTests="true"
         beStrictAboutChangesToGlobalState="true"
         checkForUnintentionallyCoveredCode="false">

checkForUnintentionallyCoveredCode is deprecated in favor of beStrictAboutCoversAnnotation in PHPUnit 5.2.0 and was eventually removed in PHPUnit 6.0.

Since we'll be jumping from PHPUnit 4.8.36 to 6.5.2, this setting will become stale with no warnings unless we remember to do something about it (add a @todo?)

+++ b/core/tests/Drupal/Tests/Phpunit5FCTrait.php
@@ -0,0 +1,147 @@
+trait Phpunit5FCTrait {

Let's rename it to Phpunit6FCTrait then?

@dawehner well I'm pretty sure that the code would actually still work on PHPUnit 5 because it has the same MockBuilder class as 4 and 6 but I think you're right that renaming the trait is in order. Maybe to something like PHPUnitCompatibilityTrait? Atm this trait is providing stuff for both PHPUnit 4 and 6 so that you could use createMock() in PHPUnit 4 if you like too. That's actually one of the things I'm kinda unsure about - I like getting getMock() to work on PHPUnit 6 but do we really need to make createMock() work on PHPUnit 4?

Also if we have to support other newer versions of PHPUnit we might not be able to do it in the same trait so I can imagine some funky class aliasing in the future.

but do we really need to make createMock() work on PHPUnit 4?

Well, it would have been a nice transition path for people switching over, but now that we seem to have made the decision to jump on a newer phpunit version anyway, this is indeed less helpful.

I've done more thinking about #81 - I think we need to provide a createMock() in PHPUnit 4 that works like PHPUnit 6 and not change the behaviour of createMock() in PHPUnit 6 which is what the previous patches are doing. Once we do #2927806: Use PHPUnit 6 for testing when PHP version >= 7.2 then we can improve the test because we actually will be doing testing on different PHP Versions.

Patch attached renames things, completes and fixes the docs and changes the trait to use a supports() method instead to allow for other forward compatibility methods and to make it easier to grok what's happening.

The interdiff is not that useful cause renames.

Renaming is good, keeps it open for the future (PHPUnit 7 is out in Feb 2018, and 8 planned for Feb 2019...).

I found supernits below, but in general this seems RTBC to me.

1. +++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTrait.php
   @@ -0,0 +1,142 @@
   +   * The getMock method is does not exist in PHPUnit 6. To provide backward
   

   'is' should go away

2. +++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTraitTest.php
   @@ -0,0 +1,113 @@
   + * Test class for \PHPUnit\Framework\TestCase in PHPUnit 5.
   

   PHPUnit 6

3. +++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTraitTest.php
   @@ -0,0 +1,113 @@
   + * Test class for \Drupal\Tests\UnitTestCase with PHPUnit 5.
   

   same

4. +++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTraitTest.php
   @@ -0,0 +1,113 @@
   + * Test class for \Drupal\Tests\UnitTestCase with PHPUnit 5.
   

   same

Updated CR.

Addressing #84

+++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTraitTest.php
@@ -0,0 +1,113 @@
+ * @group simpletest

is this correct?

@larowlan well we have lots of unit tests with this - like \Drupal\Tests\simpletest\Unit\TestDiscoveryTest and we have the other more generic Tests too.

+++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTraitTest.php
@@ -0,0 +1,113 @@
+ * @group simpletest
+ * @group Tests

Our test groups are a bit of a mess. There are also tests in the group Test... confusing.

@larowlan mind you the only one outside simpletest module itself is \Drupal\Tests\AssertHelperTraitTest - so perhaps you're onto something. Anyway there is no harm in removing it and this does have nothing to do with Simpletest so one less thing to remove when simpletest is removed.

+++ b/core/tests/Drupal/Tests/PhpunitCompatibilityTraitTest.php
@@ -0,0 +1,112 @@
+ * Test the PHPUnit forward compatibility trait.

Nit, but shouldn't this be 'Tests the', and if so that applies to method summaries for the rest of the file.

I really like the approach in general.

Here's some updated docs. Thanks for the reviews @catch and @larowlan.

I've updated the CR for the move to PHPUnit 6 and opened a followup #2929133: Replace all usages of getMock() to replace all our usages of getMock() so we can actually add an @trigger_error().

so we can actually add an @trigger_error()

I suggest that be an issue targeted to Drupal 9.1.0. :-)

Adding mondrake to commit credits for reviews that helped shape the patch

Queued testing on 5.5, 5.6 and 7.1.

Are we sure this a feature request?

A feature request can't be critical and a feature request can't go into the stable branch.

Well it started out as a feature request, but with the advent of php 7.2 and its incompatibilities this becomes a bug of course.

Looking back in the comments, what about #79? Anything to be done here, followup, nothing :)?

@mondrake I think that that is part of the issue that makes us use PHPUnit 6... that happens in #2927806: Use PHPUnit 6 for testing when PHP version >= 7.2

OK, copied #79 to #2927806-63: Use PHPUnit 6 for testing when PHP version >= 7.2.

Committed f320141 and pushed to 8.5.x.

As this is a critical bug, I cherry-picked as f9a4241 and pushed to 8.4.x.

Because we're not adding the trigger_error() here, this deprecation should have minimal disruption.

Published change record.

Updated the postponed issue to indicate this is in, see you there.

Thanks all