Dokuwiki: Broken Tests on PHPUnit 8

[phy25]

PHPUnit recently releases version 8 (which is now the default on Travis CI PHP 7.2 environment). It introduces "Return Type of Template Methods", which is failing DokuWiki's tests on PHP 7.2.

Looks like it will be very difficult to make tests work on both versions, since return type hinting is not compatible with PHP 5 (and we still need to support that). If we cannot find a way to support both, we have to make our Travis CI environment stick to PHPUnit 7, or tests on PHP 7.2 will always fail.

[phy25]

I happen to read Laravel's PHPUnit 8 issue (but they don't support PHP5.x anymore)... I guess the only thing we can do for now is to postpone support to PHPUnit 8, and configure Travis CI accordingly.

[Klap-in]

Btw: How long do we intend to support php 5.x?
Was there already a plan for deprecating it? Or maybe practical to think about it? This before to start creating difficult solutions...

[phy25]

Honestly we have a lot of enterprise users, who are somewhat unwilling to upgrade to PHP 7.x. But it's really only a matter of time...

[micgro42]

I thought the plan was to drop PHP5 support with the upcoming release? Maybe even 7.0 support and to require 7.1, since that is the oldest version that is still getting any security fixes.

Enterprises should have workflows to not run PHP5 anymore, as I sometimes feel like DokuWiki is one of the last PHP projects still supporting it.

It might be more of some vocal lone admins that still cling to an old Debian or CentOS installation without desire to upgrade? But then they might be fine with an older DokuWiki version as well :grin: 🤷

Nonetheless, we should fix Travis for now, these failures are annoying.

[phy25]

Still trying to find the best way to force the version (composer or just drop a phpunit.phar).

[micgro42]

The solution that Symfony found for this is to physically edit the phpunit TestCase files and to remove the : void typehints:

https://github.com/symfony/phpunit-bridge/blob/master/bin/simple-phpunit.php

    // Mutate TestCase code
    $alteredCode = file_get_contents($alteredFile = './src/Framework/TestCase.php');
    if ($PHPUNIT_REMOVE_RETURN_TYPEHINT) {
        $alteredCode = preg_replace('/^    ((?:protected|public)(?: static)? function \w+\(\)): void/m', '    $1', $alteredCode);
    }
    $alteredCode = preg_replace('/abstract class (?:TestCase|PHPUnit_Framework_TestCase)[^\{]+\{/', '$0 '.PHP_EOL."    use \Symfony\Bridge\PhpUnit\Legacy\PolyfillTestCaseTrait;", $alteredCode, 1);
    file_put_contents($alteredFile, $alteredCode);

[phy25]

@micgro42 I would use it only when older PHPUnit does not work with new PHP versions anymore... Looks good though. Thanks for posting it here!

[micgro42]

I agree.

Though I would like to see a way forward as well. I was toying with the idea of declaring the current DokuWikiTest class as legacy and creating a new DokuWikiIntegrationTest class that is compatible with PHPUnit 8.
This new class would allow Plugins that already dropped support for end-of-life PHP versions to update their tests. This is in particular relevant as the default travis.sh script downloads PHPUnit 8 for PHP 7.4, which means that plugins have to either employ hacks (e.g. testing on 7.4snapshot) or not testing on PHP 7.4 at all.

That new class would even allow, but not require, us to also think about creating a DokuWikiUnitTest class for true unit-tests that disallows accessing filesystem or network or globals(?). These should run _much_ fast than the current tests as they require no setup.

[phy25]

This new class would allow Plugins that already dropped support for end-of-life PHP versions to update their tests. This is in particular relevant as the default travis.sh script downloads PHPUnit 8 for PHP 7.4, which means that plugins have to either employ hacks (e.g. testing on 7.4snapshot) or not testing on PHP 7.4 at all.

I am not sure if any plugin is dropping PHP5 support now indeed. I understand that we will eventually drop PHP5 someday but it may be a long time (like Python 2).

Looking at https://github.com/travis-ci/php-src-builder/blob/f75181cabdbacc0517803b3c8251928aa99c69fe/bin/compile#L86-L93 they always use latest PHPUnit on PHP 7+ in a Travis CI environment.

Maybe I should add PHPUnit version requirements in composer.json by using require-dev to make this less troublesome - since not everyone is using our fetchphpunit.php.

That new class would even allow, but not require, us to also think about creating a DokuWikiUnitTest class for true unit-tests that disallows accessing filesystem or network or globals(?). These should run _much_ fast than the current tests as they require no setup.

This sounds cool - a great chance to refactor. May this is worth a new issue or may I reopen this?

[splitbrain]

I reopened this because we need to find a solution here.

Current State

To recap: this is the PHP version support of PHP unit:

| | PHPUnit 5 | PHPUnit 6 | PHPUnit 7 | PHPUnit 8 | PHPUnit 9 |
|---------|-----------|-----------|-----------|-----------|-----------|
| PHP 5.6 | ✔ | | | | |
| PHP 7.0 | ✔ | ✔ | | | |
| PHP 7.1 | ✔ | ✔ | ✔ | | |
| PHP 7.2 | | ✔ | ✔ | ✔ | |
| PHP 7.3 | | | ✔ | ✔ | ✔ |
| PHP 7.4 | | | | ✔ | ✔ |

The problem is that from PHPUnit 8 onwards, typehints are used that are not compatible with a) older PHP versions and b) older PHPUnit version. Up until now, we simply used the PHPUnit version that was suitable for the tested PHP release.

This is no longer possible, because there is no way around PHP doing strict type checks and failing fatally on mismatches.

How to fix it

The officially proper way forward would be to switch to PHPUnit 8 by adjusting all method signatures in our tests and stop testing on PHP < 7.2. This would also break lots of tests for all plugins until they adjust their their code accordingly.

A hacky way would be to dynamically adjust the original PHPUnit files as Symfony does it. This however does not work when using the PHPunit phar files for testing and is a hack bound to break any time.

Alternative

However if we're looking at breaking compatibility for everyone anyway, I believe it would be worth to look at alternatives to phpunit. Their approach to to always chase the newest PHP release and drop support for older releases seems not to be very much in line of how we do things.

Atoum seems to be pretty popular and they state:

despite constant changes to atoum, backward compatibility is one of the priorities of its developers.

Their composer.json lists them as compatible from 5.6 upwards.

Atoum's way to write tests is fundamentally different to PHPUnit though and might be weird to get into. However they do have a PHPUnit Extension that promises to simply run your phpunit tests as is. That might be worth a try.

So what do you think? How should we fix this?

[phy25]

I think PHPUnit 7 might be able to work with PHP 7.4+ (and we can simply use PHPUnit <7 for some time)? However keeping something that is not officially supported might not be the best solution.

Using some other test engine with PHPUnit compat layer sounds good to me, but I am not sure how well Atoum's compat layer is when running with our existing tests (http://extensions.atoum.org/extensions/phpunit listed what they support and what they don't yet). And yes I feel their test writing style is weird as well.

[splitbrain]

Ohhh, you're right. PHPUnit 7 seems to work on 7.4 :open_mouth: I thought I had tested that!?