Prevent doubling of __destruct()

[Daimona]

Q	A
PHPUnit version	9.5.4 (but dev-master is also affected)
PHP version	7.4.1
Installation Method	Composer

Summary

If a test uses $this->mock->expects( $this->never() )->method( '__destruct' );, under certain circumstances (unsure which ones exactly), if the garbage collector kicks in and __destruct is called, the test suite will abort immediately.

Current behavior

If you run the faulty test class, PHPUnit runs the test as normal, but then the following message is printed:

MyTest\MockedClass::__destruct() was not expected to be called.

and PHPUnit will exit with exit code 2 without any additional output.

I'm not entirely sure about the cause of this error, as it might be PHPUnit or PHP itself (e.g. a GC bug), so I am providing a complete reproduction environment below.

How to reproduce

Here is all you need to reproduce this bug. The test case is derived from MediaWiki, where I've seen the issue happen in the wild.

composer.json

{
	"require-dev": {
		"phpunit/phpunit": "^9.5"
	},
	"autoload": {
		"psr-4": { "MyTest\\": "src" }
	}
}

phpunit.xml.dist

<?xml version="1.0" encoding="UTF-8"?>
<phpunit>
	<extensions>
		<extension class="MyTest\GarbageCollectExtension" />
	</extensions>
</phpunit>

src/GarbageCollectExtension.php

<?php

namespace MyTest;

use PHPUnit\Runner\AfterTestHook;

/** We use this extension to trigger the garbage collector on purpose. In real-world usage, if there are many tests, the GC might trigger on its own */
class GarbageCollectExtension implements AfterTestHook {
	public function executeAfterTest( string $test, float $time ): void {
		gc_collect_cycles();
	}
}

src/MockedClass.php

<?php

namespace MyTest;

class MockedClass {
	public function __destruct() {
	}
}

src/MockWrapper.php

<?php

namespace MyTest;

class MockWrapper {
	private $closureProp;
	private $mockObj;

	public function __construct( $mockObj ) {
		$this->mockObj = $mockObj;
		$this->closureProp = function () {}; // Setting a closure here is fundamental.
	}
}

tests/CrashTest.php

<?php

use MyTest\MockedClass;
use MyTest\MockWrapper;

class CrashTest extends \PHPUnit\Framework\TestCase {
	protected $mockedClass;
	protected $mockWrapper;

	protected function tearDown() : void {
		unset( $this->mockWrapper ); // Both `unset()` here are necessary; the order doesn't matter
		unset( $this->mockedClass );
	}

	public function testSomething() : void {
		$this->mockedClass = $this->createMock( MockedClass::class );
		$this->mockedClass->expects( $this->never() )->method( '__destruct' );
		$this->mockWrapper = new MockWrapper( $this->mockedClass );
		$this->assertSame( 'y', 'x' ); // This assertion is also necessary, but the suite will die before printing that it fails.
	}
}

Once you've created all the above, run the following from the project root.

$ composer install
$ vendor/bin/phpunit tests/CrashTest.php

Expected behavior

The soft assertion (of __destruct never being called) should fail as it currently does [1], but then the execution should continue, and the final summary should be printed (i.e. runtime, memory, failures, assertions etc.).

[1] - Although this is not necessary for my use case.

[sebastianbergmann]

Sorry to say this, but there is nothing I want to do about that. Destructors should not be doubled, IMO.

[Daimona]

Sorry to say this, but there is nothing I want to do about that. Destructors should not be doubled, IMO.

Yes, this makes sense. I should note that the original issue happens because the test uses:

$this->mockedClass->expects( $this->never() )->method( $this->anythingBut( 'method1', 'method2' ) );

where anythingBut is defined as

protected function anythingBut( ...$values ) {
    return $this->logicalNot( $this->logicalOr(
        ...array_map( [ $this, 'identicalTo' ], $values )
    ) );
}

Whoever wrote this code didn't think of __destruct being defined in the mocked class. So the issue reported here happens as a side effect. This is why I said that reporting a violated soft assertion for __destruct might be fine, but is not needed for my use case.

[Daimona]

Also, FTR, the issue still happens with PHP 8.0.1, so if it's a GC bug, it wasn't fixed in PHP 8.

[sebastianbergmann]

anythingBut() is not something PHPUnit provides.

[Daimona]

anythingBut() is not something PHPUnit provides.

I know, and I've pushed a fix so that our implementation skips __destruct. But this specific anythingBut method is just an example of how it's possible to end up doubling a destructor (unintentionally).

Perhaps, if doubling destructors is not officially supported, PHPUnit should explicitly refuse to do that (e.g. by showing a warning or throwing an exception)? Or maybe even just state in the documentation that it might have unwanted effects as reported here?